Re: Bypassing cursors in postgres_fdw to enable parallel plans

One of the patch required a rebase, hence attaching the rebased patches.

On Wed, 20 May 2026 at 12:40, Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com> wrote:

>
>
> On Tue, 12 May 2026 at 14:56, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Mon, May 11, 2026 at 7:03 AM Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
>> wrote:
>> > Thanks Robert for the detailed review and some great suggestions for
>> improving this patch. I went through the comments and the patch and here is
>> the revised version. To summarize the changes done in this patch,
>> > - as suggested, split it into two patches now, the first patch changes
>> the cursor_exists flag to scan_in_progress, the second patch is the one
>> with the rest of the changes to implement this fetch mechanism
>> > - changed the GUC to server/tables level option called streaming_fetch.
>> Still a boolean type. Open to better naming suggestions.
>> > - refactored postrgesReScanForeignScan for a more readable code
>> > Following the suggestions above, now, it handles the case for the
>> backward cursor separately and exits the function straight from there. In
>> case of close cursor, it checks if cursor or cursor-free mode is used, and
>> executes different actions as per the mode. In cursor-free mode we end_scan
>> and tuplestore. In the third case when we start scanning the tuples already
>> fetched, things remain the same in both modes.
>> > - added the changes to show the option in explain verbose output
>> > - Added a lot more test cases to check the option with other options
>> and also to cover more code-paths, particularly rescans and ending the
>> query in between, error cases etc.
>> >
>> > There is one problem that remains here, the use of ExprContext. We need
>> this when we are fetching the tuples for the active scan, but since this is
>> only available in node, we can not have the one which was there at the time
>> when active_scan was the fsstate, so at the moment it is using the econtext
>> from the node. This doesn't seem correct to me, but to have the correct
>> ExprContext we need the node, but it also doesn't seem totally right to
>> have the pointer to node in ScanState for this scenario. Please let me know
>> what could be a good way to handle this.
>>
>> The problem here is that save_to_tuplestore() has gotten itself into
>> the business of resending the query when a node is rescanned and we
>> haven't yet buffered the results anywhere. But it doesn't need to do
>> that in the first place. If the query needs to be resent, it's not
>> currently active on the connection, and then there's no need to do
>> anything to free up the connection, so save_to_tuplestore() doesn't
>> need to be called in the first place. The reason why you're having
>> this problem is that postgresReScanForeignScan calls end_scan but
>> end_scan does not clear the active_scan pointer. So then
>> save_to_tuplestore() gets called if something else tries to use the
>> connection, and to make that work, you did this.
>>
>> But the right solution is to make sure that the active_scan pointer is
>> only non-NULL when there are actually results already on the wire that
>> need to be drained. If you do that, then save_to_tuplestore() won't
>> get called if rescan has already read all of the pending results off
>> the wire, and then you can delete the code that resends the query, and
>> then you won't need an econtext here in the first place.
>>
>> Thanks Robert for the quick follow up on this. I found this to be true
> and modified the patch accordingly.
> Please find the updated patches attached. In the first patch jnothing is
> changed, just adding it here for the sake of completeness.
>
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>

--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH