On Tue, 21 Jul 2026 at 02:02, Robert Haas <[email protected]> wrote:
> 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. > Yes you are right, this was based on the existing flow of pendingAreq, but also because we have access to conn_state in this function. In the functions pgfdw_cancel_query_end and pgfdw_get_cleanup_result we are only dealing with conn only, so clearing the conn_state there doesn't seem possible without significant changes. > > - 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). > > I realise, following these inputs we would not even need this function anymore and can just use save_to_tuplestore instead, that will simplify the code. Thanks. > (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.) > > Unfortunately, I keep on making this mistake because we can get active_state from current_state, so I just find this simple to start with current_fsstate and use active_fsstate when required. I would be more mindful of this. > - 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 > -- Regards, Rafia Sabih CYBERTEC PostgreSQL International GmbH
