Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Tue, 11 Aug 2026 at 02:13, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> 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.
>
> 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