Repository navigation
Conversation
dcd9357 to
1179700
Compare
tonio
left a comment
tonio
left a comment
There was a problem hiding this comment.
A few blocking issues in the transfer endpoints, see inline comments: the export holds a table lock for the whole download, export failures reach the client as a 200, and every restore failure is reported as a 400.
| with data_engine.connect() as sa_connection: | ||
| conn = _driver_connection(sa_connection) | ||
| # A re-ingestion replacing the table waits for the end of the dump | ||
| conn.execute(sql.SQL("LOCK TABLE {} IN ACCESS SHARE MODE").format(table)) |
There was a problem hiding this comment.
The lock, along with a pooled data_engine connection, stays held for as long as the client takes to read the response, not just for as long as the DB takes to produce it. With a slow or stalled client, a re-ingestion that runs DROP TABLE queues for ACCESS EXCLUSIVE, and every later SELECT on the table (GeoServer WMS/WFS) queues behind it, so the published layer hangs until the download ends. A few concurrent exports can also exhaust the pool.
Suggestion: write the dump to a tempfile.SpooledTemporaryFile inside the locked section, release the connection, then stream the file.
| """Gzip stream of the table, read in a single snapshot.""" | ||
| table = sql.Identifier(schema, table_name) | ||
| compressor = zlib.compressobj(wbits=GZIP_WBITS) | ||
| with data_engine.connect() as sa_connection: |
There was a problem hiding this comment.
Because this is a generator passed to StreamingResponse, none of this code runs until after the 200 status and headers are sent. Any failure here (the table moved or dropped between _locate_final_table and LOCK TABLE, a lock or statement timeout, a dropped connection) gives the client an HTTP 200 with an empty or truncated body. The client only finds out later, when PUT rejects the file as invalid gzip.
Opening the connection, taking the lock and reading the definition before returning the response would allow a proper 404 or 500. Spooling the dump as suggested in the comment on line 91 fixes this as well.
| except TableTransferError: | ||
| conn.rollback() | ||
| raise | ||
| except Exception as e: |
There was a problem hiding this comment.
This turns every exception into TableTransferError, so the endpoint returns 400 "stream is rejected" even for server-side problems:
- Two concurrent
PUT /data/{org}/{table}requests both pass the exists check, and the secondCREATE TABLEfails with "relation already exists". The client gets 400 instead of the documented 409. - A dropped connection, a full disk or a statement timeout is reported as a bad file (400) instead of a server error (500).
_check_definition (line 152) does the same thing and reports these failures as "Invalid type".
Suggestion: map only data errors from the COPY (psycopg.errors.DataError, InvalidTextRepresentation, ...) to TableTransferError, map DuplicateTable to a 409, and let everything else propagate as a 500.
| READ_SIZE = 1024 * 1024 | ||
| MAX_DEFINITION_SIZE = 1024 * 1024 | ||
| # Column of the final tables created with an id (see transform_staging_to_final) | ||
| DATAFEEDER_ID_COLUMN = "id_datafeeder" |
There was a problem hiding this comment.
This duplicates ID_COLUMN from data_manipulation.transformation.sql_transform, which is the constant that transform_staging_to_final actually uses. Importing it keeps the two from drifting apart, and the comment above becomes unnecessary.
No description provided.