Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Tue, 21 Jul 2026 at 02:02, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jul 15, 2026 at 7:50 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
> 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