Re: Bypassing cursors in postgres_fdw to enable parallel plans

On Fri, 21 Aug 2026 at 12:25, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:

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

CFBot informed me that the patches need a rebase. Please find the attached
files for the rebased patches.
--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH