On Wed, Jul 15, 2026 at 7:50 AM Rafia Sabih <[email protected]> wrote: > Please find the rebased patches attached.
Hi, I think you're making progress here, but I still see some problems. - I don't really like the fact that pgfdw_abort_cleanup just zeroes the active_scan pointer. When a transaction or subtransation is aborted, pgfdw_cancel_query_end calls pgfdw_get_cleanup_result to consume all the remaining tuples. It's at that point that we can say no active scan is in progress any more, so that's where I think the pointer should be getting cleared. If we ever arrived at one of these memset calls with an active scan still in progress, just clearing the pointer wouldn't fix anything -- we'd need to actually drain the tuples. The reason it works is because the tuples should always be getting drained earlier, before this logic is reached, so I think we should clear the pointer there rather than here. Now, on the other hand, that same logic probably applies to the pendingAreq stuff, about whose correctness I am also suspicious, and you could maybe argue you just followed the existing precedent. But I think it's a bad precedent upon which I would rather not double down. - A side benefit of the above change is that postgresReScanForeignScan would not need to set fsstate->conn_state->active_scan = NULL when it calls pgfdw_cancel_query. Instead, calling pgfdw_cancel_query would clear active_scan as a side-effect, which is more correct. - The explain output for streaming fetch mode now shows "streaming_fetch: true". That's nice, but the capitalization and punctuation is different from every other EXPLAIN property, which is less nice. - I think it would be a good idea for pgfdw_exec_query() to Assert(state == NULL || state->active_scan == NULL). That way, if we ever miss a drain_other_active_scan() call, it has a chance of tripping an Assert, if that code path is tested. - drain_other_active_scan's test for conn_state->active_scan != self does not do anything. postgresReScanForeignScan and postgresEndForeignScan don't call it when fsstate->streaming_fetch is true. In create_cursor and init_scan we are setting up a new active scan so the newly-created scan cannot already be in process. In execute_foreign_modify and execute_dml, there is no support for streaming fetch, so a streaming fetch scan for one of those cases cannot already be in progress. If you remove that test, then the "self" parameter for that function isn't needed any more. You don't need the conn parameter either, because it has to be the same as conn_state->active_scan->conn. So this can be simplified down to a one-parameter function: drain_other_active_scan(PgFdwConnState *conn_state). (This last point is basically the same issue that we have exchanged many emails about already. You shouldn't need to pass anything from the current scan into the functions that clear the active scan. Everything should be done via the active scan.) - fetch_from_tuplestore fetches every single tuple out of the tuplestore and returns them all at once. This defeats the purpose of using a tuplestore. The purpose of the tuplestore is to keep you from needing to put all the tuples in memory at the same time. So, when the connection is needed by some other process, you correctly put all of the tuples into a tuplestore, which means it can spill to disk if there's too much data. But as soon as we read from the tuplestore, you read all of the tuples at once. This is different from what happens in the cursor mode, which is that we fetch tuples from the remote side in small batches. This probably needs to work similarly. - save_to_tuplestore doesn't handle unexpected result statuses cleanly. If PQresultStatus(res) is an unexpected value - i.e. not PGRES_FATAL_ERROR, PGRES_TUPLES_OK, or PGRES_TUPLES_CHUNK - like say if somehow managed to be PGRES_COMMAND_OK or PGRES_COPY_BOTH or PGRES_PIPELINE_SYNC, then something fairly random would happen. It's OK for the code to error out if it sees a result status it isn't expecting, but it shouldn't just fall through to code that isn't going to cope with that status nicely. -- Robert Haas EDB: http://www.enterprisedb.com
