Re: Bypassing cursors in postgres_fdw to enable parallel plans - Mailing list pgsql-hackers

On Tue, 11 Aug 2026 at 02:13, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 3, 2026 at 7:28 AM Rafia Sabih <rafia.pghackers@gmail.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.

You are indeed right in this observation. I tried to keep them similar in this version.

I chose to make  pgfdw_exec_query() self-draining rather than trusting every caller to remember. It no longer Asserts that state->active_scan == NULL. Instead it actively drains any pending streaming scan itself before sending the new query. state is now a required, non-NULL parameter.  

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 function is now changed to have only PgFdwScanState i.e. active_scan as the parameter and PGConn is now retrieved from this itself. Next, now there is a loop and within it there is only via calls to fetch_stream_result to retrieve chunks of tuples and handle all the other result status cases. This looks much cleaner and also is what you are asking for here. Also, I felt the name drain_other_active_scan as more appropriate for this function than save_to_tuplestore. But of course I am open to renaming anything else which makes more sense.

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.

Removed. 

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.

In the attached patch, fetch_stream_result handles all the result status and also it reports errors whenever required. save_to_tuplestore function now repeatedly calls fetch_stream_result to get the chunks as well as handling different result statuses, so more duplicate handling of TUPLES_OK, NULL, etc. In fetch_more_data, when eof is marked because of the partial chunk, then it calls save_to_tuplestore to save the tuples to tuplestore and handle the NULL state afterwards via call to fetch_stream_result. So things are more uniformly handled now. 

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.

I have now placed the increment block for fetch_ct_2 to the beginning of the function, so that it can be handled similarly in case of either mode. My understanding with respect to fetch_ct_2 is that it should mean the same in any of these modes, so I am using it similarly. 

Once again thank you for your support and patient review for this work.

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

--

Regards,
Rafia Sabih

CYBERTEC PostgreSQL International GmbH