Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Mon, Aug 3, 2026 at 7:28 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:
> Please find the reworked patch set attached.
> This version handles the concerns discussed above.

Hi,

I noticed while looking over this version of the patch set that there
are a lot of similarities between process_pending_request() and
save_to_tuplestore(). In one sense, actually, they're exactly the same
thing: they track some query that is actively using the connection and
whose results must be read before the connection can be used for
something else. Possibly they should share code, but for sure we
should be suspicious if one of them is called someplace that the other
is not. execute_foreign_modify() is one place where things diverge:
process_pending_request() is called at the top of the function, and
save_to_tuplestore() is called a bit further down. That's a problem,
because SQL queries can be executed in the middle, showing that you've
put the call to save_to_tuplestore() in the wrong place.

Another place where this comes up is pgfdw_exec_query(). There, async
requests are handled via process_pending_request(), but for an active
scan, we instead Assert that the caller took care of it. But in fact,
many callers of pgfdw_exec_query() don't do that (e.g.
postgresAnalyzeForeignTable, postgresGetAnalyzeInfoForForeignTable).
We either need to make them all do it, or perhaps we should think
about changing things so that pgfdw_exec_query() does
save_to_tuplestore() itself. That would require every caller to pass a
non-NULL PgFdwConnState, or else those that don't would need to be
safe for some other reason.

Yet another place where this comes up is in fetch_more_data_begin().
It sends a new query, but it doesn't call save_to_tuplestore() first.
And it comes up in GetConnection() too, which has pendingAreq handling
but no save_to_tuplestore(). It's not impossible to make all of this
logic correct if pendingAreq is handled in one way and streaming_fetch
is handled in a different way that is also correct but puts all the
code in different places. However, it might be easier if we make them
as similar as possible, instead of inventing a new way to do a very
similar thing.

The signature for save_to_tuplestore is still not correct. I don't
understand how this can still not be right after as much discussion as
we've had. As I said last time: "So this can be simplified down to a
one-parameter function: drain_other_active_scan(PgFdwConnState
*conn_state)." In the current patch, it has two parameters, which is
more than one. Once again, you don't need PGconn *conn. As I said last
time: "You don't need the conn parameter either, because it has to be
the same as conn_state->active_scan->conn." That's still true.

save_to_tuplestore() should be structured with a loop at the top
level, instead of entering a loop only after the first call to
pgfdw_get_next_result(). Right now, if the first call to
pgfdw_get_next_result() returns an unexpected PQresultStatus(), an
error will be reported (which is good), but if a later call returns an
unexpected PQresultStatus(), no error will be reported, which is bad.
Everything that gets done for the first call to
pgfdw_get_next_result() should be done for the subsequent ones too.
This function currently has three calls to pgfdw_get_next_result() and
you want to get that down to no more than two, or possibly just one.

The use of #ifdef LIBPQ_HAS_CHUNK_MODE is not correct. If this patch
were to be accepted, it would be part of a version of PostgreSQL that
definitely has chunk mode. We do not need to cater to the scenario of
that not being true.

In fetch_more_data(), the call to fetch_stream_result() to clear the
remains of the query from the connection is quite scary -- if any
tuples were returned there, unexpectedly, they'd be silently dropped.
The caller is expecting no tuples, but that's not checked. The bigger
picture here is that fetch_stream_result() is highly duplicative of
save_to_tuplestore(). The error handling isn't currently identical
between those two functions, but it should be. Either we should get
rid of fetch_stream_result(), or save_to_tuplestore() should use it as
a subroutine.

fetch_ct_2 needs to count the number of times that tuples[] has been
filled, to a maximum of 2, so that postgresReScanForeignScan can use
that information to know how to rewind. But it doesn't currently work
that way, because the streaming-tuplestore cases in fetch_more_data()
ignore it. If we enter the if (fsstate->tuplestore) block, then we
reload tuples[] inside fetch_from_tuplestore() but don't touch
fetch_ct_2. If we have no tuplestore, then we call
fetch_stream_result() which is fine except when res == NULL. In that
case, we return right away without bumping fetch_ct_2.

--
Robert Haas
EDB: http://www.enterprisedb.com