Thread

Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2025-01-06T08:52:10Z

Hello hackers,

At present, in postgres_fdw, if a query which is using a parallel plan is
fired from the remote end fails to use the parallel plan locally because of
the presence of CURSORS. Consider the following example,
Local server,
Table:
create table t ( i int, j int, k text);
insert into t values(generate_series(1,10000), generate_series(1, 10000),
'check_this_out');
Query
select * from t where i > 1000;
Query plan
Gather  (cost=0.00..116.08 rows=9000 width=23)
 Workers Planned: 2
 ->  Parallel Seq Scan on t  (cost=0.00..116.08 rows=3750 width=23)
       Filter: (i > 1000)

Foreign server
create extension postgres_fdw;
CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host '127.0.0.1', port '5432', dbname 'postgres');
CREATE USER MAPPING FOR user1 SERVER foreign_server OPTIONS (user 'user1');
Table
CREATE FOREIGN TABLE foreign_table ( i int, j int, k text ) SERVER
foreign_server OPTIONS (schema_name 'public', table_name 't');
Query
select * from t where i > 1000;
Query plan at the local server
Seq Scan on t  (cost=0.00..189.00 rows=9000 width=23)
Filter: (i > 1000)

I have used auto_explain extension to get the query plans at the local
server and also following settings in .conf to force the parallel plans for
the purpose of demonstration --
min_parallel_table_scan_size = 0
parallel_tuple_cost= 0
parallel_setup_cost = 0

with the patch:
set postgres_fdw.use_cursor = false;
Query plan at the local server
 Gather  (cost=0.00..116.08 rows=9000 width=23)
 Workers Planned: 2
 ->  Parallel Seq Scan on t  (cost=0.00..116.08 rows=3750 width=23)
       Filter: (i > 1000)

Now, to overcome this limitation, I have worked on this idea (suggested by
my colleague Bernd Helmle) of bypassing the cursors. The way it works is as
follows,
there is a new GUC introduced postgres_fdw.use_cursor, which when unset
uses the mode without the cursor. Now, it uses PQsetChunkedRowsMode in
create_cursor when non-cursor mode is used. The size of the chunk is the
same as the fetch_size. Now in fetch_more_data, when non-cursor mode is
used, pgfdw_get_next_result is used to get the chunk in PGresult and
processed in the same manner as before.

Now, the issue comes when there are simultaneous queries, which is the case
with the join queries where all the tables involved in the join are at the
local server. Because in that case we have multiple cursors opened at the
same time and without a cursor mechanism we do not have any information or
any other structure to know what to fetch from which query. To handle that
case, we have a flag only_query, which is unset as soon as we have assigned
the cursor_number >= 2, in postgresBeginForeignScan. Now, in fetch_more
data, when we find out that only_query is unset, then we fetch all the data
for the query and store it in a Tuplestore. These tuples are then
transferred to the fsstate->tuples and then processed as usual.

So yes there is a performance drawback in the case of simultaneous queries,
however, the ability to use parallel plans is really an added advantage for
the users. Plus, we can keep things as before by this new GUC --
use_cursor, in case we are losing more for some workloads.  So, in short I
feel hopeful that this could be a good idea and a good time to improve
postgres_fdw.

Looking forward to your reviews, comments, etc.
-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2025-01-14T17:33:33Z

On Mon, Jan 6, 2025 at 3:52 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Now, to overcome this limitation, I have worked on this idea (suggested by my colleague Bernd Helmle) of bypassing the cursors. The way it works is as follows,
> there is a new GUC introduced postgres_fdw.use_cursor, which when unset uses the mode without the cursor. Now, it uses PQsetChunkedRowsMode in create_cursor when non-cursor mode is used. The size of the chunk is the same as the fetch_size. Now in fetch_more_data, when non-cursor mode is used, pgfdw_get_next_result is used to get the chunk in PGresult and processed in the same manner as before.
>
> Now, the issue comes when there are simultaneous queries, which is the case with the join queries where all the tables involved in the join are at the local server. Because in that case we have multiple cursors opened at the same time and without a cursor mechanism we do not have any information or any other structure to know what to fetch from which query. To handle that case, we have a flag only_query, which is unset as soon as we have assigned the cursor_number >= 2, in postgresBeginForeignScan. Now, in fetch_more data, when we find out that only_query is unset, then we fetch all the data for the query and store it in a Tuplestore. These tuples are then transferred to the fsstate->tuples and then processed as usual.
>
> So yes there is a performance drawback in the case of simultaneous queries, however, the ability to use parallel plans is really an added advantage for the users. Plus, we can keep things as before by this new GUC -- use_cursor, in case we are losing more for some workloads.  So, in short I feel hopeful that this could be a good idea and a good time to improve postgres_fdw.

Hi,

I think it might have been nice to credit me in this post, since I
made some relevant suggestions here off-list, in particular the idea
of using a Tuplestore when there are multiple queries running. But I
don't think this patch quite implements what I suggested. Here, you
have a flag only_query which gets set to true at some point in time
and thereafter remains true for the lifetime of a session. That means,
I think, that all future queries will use the tuplestore even though
there might not be multiple queries running any more. which doesn't
seem like what we want. And, actually, this looks like it will be set
as soon as you reach the second query in the same transaction, even if
the two queries don't overlap. I think what you want to do is test
whether, at the point where we would need to issue a new query,
whether an existing query is already running. If not, move that
query's remaining results into a Tuplestore so you can issue the new
query.

I'm not sure what the best way to implement that is, exactly. Perhaps
fsstate->conn_state needs to store some more details about the
connection, but that's just a guess. I don't think a global variable
is what you want. Not only is that session-lifetime, but it applies
globally to every connection to every server. You want to test
something that is specific to one connection to one server, so it
needs to be part of a data structure that is scoped that way.

I think you'll want to figure out a good way to test this patch. I
don't know if we need or can reasonably have automated test cases for
this new functionality, but you at least want to have a good way to do
manual testing, so that you can show that the tuplestore is used in
cases where it's necessary and not otherwise. I'm not yet sure whether
this patch needs automated test cases or whether they can reasonably
be written, but you at least want to have a good procedure for manual
validation so that you can verify that the Tuplestore is used in all
the cases where it needs to be and, hopefully, no others.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2025-01-17T12:03:09Z

On Tue, 14 Jan 2025 at 18:33, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Jan 6, 2025 at 3:52 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > Now, to overcome this limitation, I have worked on this idea (suggested
> by my colleague Bernd Helmle) of bypassing the cursors. The way it works is
> as follows,
> > there is a new GUC introduced postgres_fdw.use_cursor, which when unset
> uses the mode without the cursor. Now, it uses PQsetChunkedRowsMode in
> create_cursor when non-cursor mode is used. The size of the chunk is the
> same as the fetch_size. Now in fetch_more_data, when non-cursor mode is
> used, pgfdw_get_next_result is used to get the chunk in PGresult and
> processed in the same manner as before.
> >
> > Now, the issue comes when there are simultaneous queries, which is the
> case with the join queries where all the tables involved in the join are at
> the local server. Because in that case we have multiple cursors opened at
> the same time and without a cursor mechanism we do not have any information
> or any other structure to know what to fetch from which query. To handle
> that case, we have a flag only_query, which is unset as soon as we have
> assigned the cursor_number >= 2, in postgresBeginForeignScan. Now, in
> fetch_more data, when we find out that only_query is unset, then we fetch
> all the data for the query and store it in a Tuplestore. These tuples are
> then transferred to the fsstate->tuples and then processed as usual.
> >
> > So yes there is a performance drawback in the case of simultaneous
> queries, however, the ability to use parallel plans is really an added
> advantage for the users. Plus, we can keep things as before by this new GUC
> -- use_cursor, in case we are losing more for some workloads.  So, in short
> I feel hopeful that this could be a good idea and a good time to improve
> postgres_fdw.
>
> Hi,
>
> I think it might have been nice to credit me in this post, since I
> made some relevant suggestions here off-list, in particular the idea
> of using a Tuplestore when there are multiple queries running. But I
> don't think this patch quite implements what I suggested. Here, you
> have a flag only_query which gets set to true at some point in time
> and thereafter remains true for the lifetime of a session. That means,
> I think, that all future queries will use the tuplestore even though
> there might not be multiple queries running any more. which doesn't
> seem like what we want. And, actually, this looks like it will be set
> as soon as you reach the second query in the same transaction, even if
> the two queries don't overlap. I think what you want to do is test
> whether, at the point where we would need to issue a new query,
> whether an existing query is already running. If not, move that
> query's remaining results into a Tuplestore so you can issue the new
> query.
>
> I'm not sure what the best way to implement that is, exactly. Perhaps
> fsstate->conn_state needs to store some more details about the
> connection, but that's just a guess. I don't think a global variable
> is what you want. Not only is that session-lifetime, but it applies
> globally to every connection to every server. You want to test
> something that is specific to one connection to one server, so it
> needs to be part of a data structure that is scoped that way.
>
> I think you'll want to figure out a good way to test this patch. I
> don't know if we need or can reasonably have automated test cases for
> this new functionality, but you at least want to have a good way to do
> manual testing, so that you can show that the tuplestore is used in
> cases where it's necessary and not otherwise. I'm not yet sure whether
> this patch needs automated test cases or whether they can reasonably
> be written, but you at least want to have a good procedure for manual
> validation so that you can verify that the Tuplestore is used in all
> the cases where it needs to be and, hopefully, no others.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>

Indeed you are right.
Firstly, accept my apologies for not mentioning you in credits for this
work. Thanks a lot for your efforts, discussions with you were helpful in
shaping this patch and bringing it to this level.

Next, yes the last version was using tuplestore for queries within the same
transaction after the second query. To overcome this, I came across this
method to identify if there is any other simultaneous query running with
the current query; now there is an int variable num_queries which is
incremented at every call of postgresBeginForeignScan and decremented at
every call of postgresEndForeignScan. This way, if there are simultaneous
queries running we get the value of num_queries greater than 1. Now, we
check the value of num_queries and use tuplestore only when num_queries is
greater than 1. So, basically the understanding here is that if
postgresBeginForeignScan is called and before the call of
postgresEndForeignScan if another call to postgresBeginForeignScan is made,
then these are simultaneous queries.

I couldn't really find any automated method of testing this, but did it
manually by debugging and/or printing log statements in
postgresBeginForeingScan, postgresEndForeignScan, and fetch_more_data to
confirm indeed there are simultaneous queries, and only they are using
tuplestore. So, the case of simultaneous queries I found was the join
query. Because, there it creates the cursor for one side of the join and
retrieves the first tuples for it and then creates the next cursor for the
other side of join and keeps on reading all the tuples for that query and
then it comes back to first cursor and retrieves all the tuples for that
one. Similarly, it works for the queries with n number of tables in join,
basically what I found is if there are n tables in the join there will be n
open cursors at a time and then they will be closed one by one in the
descending order of the cursor_number. I will think more on the topic of
testing this and will try to come up with a script (in the best case) to
confirm the use of tuplestore in required cases only, or atleast with a set
of steps to do so.

For the regular testing of this feature, I think a regression test with
this new GUC postgres_fdw.use_cursor set to false and running all the
existing tests of postgres_fdw should suffice. What do you think? However,
at the moment when non-cursor mode is used, regression tests are failing.
Some queries require order by because order is changed in non-cursor mode,
but some require more complex changes, I am working on them.

In this version of the patch I have added only the changes mentioned above
and not the regression test modification.

-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2025-01-20T17:21:57Z

On Fri, Jan 17, 2025 at 7:03 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Indeed you are right.
> Firstly, accept my apologies for not mentioning you in credits for this work. Thanks a lot for your efforts, discussions with you were helpful in shaping this patch and bringing it to this level.
>
> Next, yes the last version was using tuplestore for queries within the same transaction after the second query. To overcome this, I came across this method to identify if there is any other simultaneous query running with the current query; now there is an int variable num_queries which is incremented at every call of postgresBeginForeignScan and decremented at every call of postgresEndForeignScan. This way, if there are simultaneous queries running we get the value of num_queries greater than 1. Now, we check the value of num_queries and use tuplestore only when num_queries is greater than 1. So, basically the understanding here is that if postgresBeginForeignScan is called and before the call of postgresEndForeignScan if another call to postgresBeginForeignScan is made, then these are simultaneous queries.

This wouldn't be true in case of error, I believe.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

KENAN YILMAZ <kenan.yilmaz@localus.com.tr> — 2025-02-17T14:09:36Z

Hi Rafia,

Based on our previous private discussion, thanks for the update and for
clarifying the current state of the patch.
I understand that more substantial changes are on the way, so I’ll focus on
relevant test scenarios rather than performance testing at this stage.

I will proceed with expanding the scenarios, including:

Multiple postgres_fdw servers are active at the same time
Multiple connections from the same postgres_fdw are active concurrently
Multiple transactions run simultaneously on a single connection
Multiple sessions operate from a single active connection

I will submit the results of these tests to this mail thread so that they
can benefit the broader community as well. Additionally, once you publish
the updated version of the patch, I will rerun the tests with the latest
changes and share the updated results.

Best Regards,

Rafia Sabih <rafia.pghackers@gmail.com>, 17 Şub 2025 Pzt, 16:46 tarihinde
şunu yazdı:

> On Tue, 14 Jan 2025 at 18:33, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> On Mon, Jan 6, 2025 at 3:52 AM Rafia Sabih <rafia.pghackers@gmail.com>
>> wrote:
>> > Now, to overcome this limitation, I have worked on this idea (suggested
>> by my colleague Bernd Helmle) of bypassing the cursors. The way it works is
>> as follows,
>> > there is a new GUC introduced postgres_fdw.use_cursor, which when unset
>> uses the mode without the cursor. Now, it uses PQsetChunkedRowsMode in
>> create_cursor when non-cursor mode is used. The size of the chunk is the
>> same as the fetch_size. Now in fetch_more_data, when non-cursor mode is
>> used, pgfdw_get_next_result is used to get the chunk in PGresult and
>> processed in the same manner as before.
>> >
>> > Now, the issue comes when there are simultaneous queries, which is the
>> case with the join queries where all the tables involved in the join are at
>> the local server. Because in that case we have multiple cursors opened at
>> the same time and without a cursor mechanism we do not have any information
>> or any other structure to know what to fetch from which query. To handle
>> that case, we have a flag only_query, which is unset as soon as we have
>> assigned the cursor_number >= 2, in postgresBeginForeignScan. Now, in
>> fetch_more data, when we find out that only_query is unset, then we fetch
>> all the data for the query and store it in a Tuplestore. These tuples are
>> then transferred to the fsstate->tuples and then processed as usual.
>> >
>> > So yes there is a performance drawback in the case of simultaneous
>> queries, however, the ability to use parallel plans is really an added
>> advantage for the users. Plus, we can keep things as before by this new GUC
>> -- use_cursor, in case we are losing more for some workloads.  So, in short
>> I feel hopeful that this could be a good idea and a good time to improve
>> postgres_fdw.
>>
>> Hi,
>>
>> I think it might have been nice to credit me in this post, since I
>> made some relevant suggestions here off-list, in particular the idea
>> of using a Tuplestore when there are multiple queries running. But I
>> don't think this patch quite implements what I suggested. Here, you
>> have a flag only_query which gets set to true at some point in time
>> and thereafter remains true for the lifetime of a session. That means,
>> I think, that all future queries will use the tuplestore even though
>> there might not be multiple queries running any more. which doesn't
>> seem like what we want. And, actually, this looks like it will be set
>> as soon as you reach the second query in the same transaction, even if
>> the two queries don't overlap. I think what you want to do is test
>> whether, at the point where we would need to issue a new query,
>> whether an existing query is already running. If not, move that
>> query's remaining results into a Tuplestore so you can issue the new
>> query.
>>
>> I'm not sure what the best way to implement that is, exactly. Perhaps
>> fsstate->conn_state needs to store some more details about the
>> connection, but that's just a guess. I don't think a global variable
>> is what you want. Not only is that session-lifetime, but it applies
>> globally to every connection to every server. You want to test
>> something that is specific to one connection to one server, so it
>> needs to be part of a data structure that is scoped that way.
>>
>> I think you'll want to figure out a good way to test this patch. I
>> don't know if we need or can reasonably have automated test cases for
>> this new functionality, but you at least want to have a good way to do
>> manual testing, so that you can show that the tuplestore is used in
>> cases where it's necessary and not otherwise. I'm not yet sure whether
>> this patch needs automated test cases or whether they can reasonably
>> be written, but you at least want to have a good procedure for manual
>> validation so that you can verify that the Tuplestore is used in all
>> the cases where it needs to be and, hopefully, no others.
>>
>> --
>> Robert Haas
>> EDB: http://www.enterprisedb.com
>>
>
> Indeed you are right.
> Firstly, accept my apologies for not mentioning you in credits for this
> work. Thanks a lot for your efforts, discussions with you were helpful in
> shaping this patch and bringing it to this level.
>
> Next, yes the last version was using tuplestore for queries within the
> same transaction after the second query. To overcome this, I came across
> this method to identify if there is any other simultaneous query running
> with the current query; now there is an int variable num_queries which is
> incremented at every call of postgresBeginForeignScan and decremented at
> every call of postgresEndForeignScan. This way, if there are simultaneous
> queries running we get the value of num_queries greater than 1. Now, we
> check the value of num_queries and use tuplestore only when num_queries is
> greater than 1. So, basically the understanding here is that if
> postgresBeginForeignScan is called and before the call of
> postgresEndForeignScan if another call to postgresBeginForeignScan is made,
> then these are simultaneous queries.
>
> I couldn't really find any automated method of testing this, but did it
> manually by debugging and/or printing log statements in
> postgresBeginForeingScan, postgresEndForeignScan, and fetch_more_data to
> confirm indeed there are simultaneous queries, and only they are using
> tuplestore. So, the case of simultaneous queries I found was the join
> query. Because, there it creates the cursor for one side of the join and
> retrieves the first tuples for it and then creates the next cursor for the
> other side of join and keeps on reading all the tuples for that query and
> then it comes back to first cursor and retrieves all the tuples for that
> one. Similarly, it works for the queries with n number of tables in join,
> basically what I found is if there are n tables in the join there will be n
> open cursors at a time and then they will be closed one by one in the
> descending order of the cursor_number. I will think more on the topic of
> testing this and will try to come up with a script (in the best case) to
> confirm the use of tuplestore in required cases only, or atleast with a set
> of steps to do so.
>
> For the regular testing of this feature, I think a regression test with
> this new GUC postgres_fdw.use_cursor set to false and running all the
> existing tests of postgres_fdw should suffice. What do you think? However,
> at the moment when non-cursor mode is used, regression tests are failing.
> Some queries require order by because order is changed in non-cursor mode,
> but some require more complex changes, I am working on them.
>
> In this version of the patch I have added only the changes mentioned above
> and not the regression test modification.
>
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>

Re: Bypassing cursors in postgres_fdw to enable parallel plans

KENAN YILMAZ <kenan.yilmaz@localus.com.tr> — 2025-03-04T16:14:29Z

Hi Rafia,
Sorry for the late response. I have completed my tests, and parallel
workers were successfully launched on the remote server. Below are the
details of my setup and test results.

# Machine Details
CPU: 4 cores
Memory: 8GB
PostgreSQL Version: 17.2 (compiled from source)
OS: Rocky Linux 8.10
Two VM instances

# PostgreSQL Configuration (For Demonstration)

logging_collector = on
log_truncate_on_rotation = on
log_rotation_size = 1GB
log_filename = 'postgresql-%a.log'
log_line_prefix = '%t [%p]: %q bg=%b, db=%d, usr=%u, client=%h, qryId=%Q,
txId=%x, app=%a, line=%l'
max_worker_processes = 4
max_parallel_workers_per_gather = 2
max_parallel_maintenance_workers = 2
max_parallel_workers = 4
debug_print_plan = on
track_functions = 'all'
log_statement = 'all'
postgres_fdw.use_cursor = false
shared_preload_libraries = 'pg_stat_statements,auto_explain'
compute_query_id = on
min_parallel_table_scan_size = 0
parallel_tuple_cost = 0
parallel_setup_cost = 0
auto_explain.log_min_duration = '0.00ms'
auto_explain.log_analyze = 'on'

# Memory Settings
effective_cache_size = '4GB'
shared_buffers = '1638MB'
work_mem = '100MB'

# Test Setup
# Creating pgbench Tables

$ pgbench -i -s 50 testdb

Running SQL Tests on Local Machine (192.168.1.68)

psql> CREATE EXTENSION postgres_fdw;
psql> CREATE SERVER fdwtest FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host
'192.168.1.69', dbname 'testdb');
psql> CREATE USER MAPPING FOR postgres SERVER fdwtest OPTIONS (user
'postgres');
psql> CREATE SCHEMA fdwtest;
psql> IMPORT FOREIGN SCHEMA public FROM SERVER fdwtest INTO fdwtest;

# Query 1: Counting Rows in a Foreign Table
testdb=# explain analyze select count(*) from fdwtest.pgbench_accounts
where aid> 1000;
                                            QUERY PLAN
──────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan  (cost=102.84..155.73 rows=1 width=8) (actual
time=457.965..457.967 rows=1 loops=1)
   Relations: Aggregate on (pgbench_accounts)
 Planning Time: 0.661 ms
 Execution Time: 458.802 ms
(4 rows)

Time: 461.944 ms

# Query 2: Fetching a Single Row from Foreign Table
testdb=# explain analyze select aid from fdwtest.pgbench_accounts where
aid> 1000 limit 1;
                                                    QUERY PLAN
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan on pgbench_accounts  (cost=100.00..100.24 rows=1 width=4)
(actual time=9.335..9.336 rows=1 loops=1)
 Planning Time: 0.452 ms
 Execution Time: 10.304 ms
(3 rows)

Time: 12.605 ms

## PostgreSQL Logs
## Local Machine Logs (192.168.1.68) – Cropped

bg=client backend, db=testdb, usr=postgres, client=[local] ,
qryId=-6000156370405137929 , txId=0, app=psql, line=14 LOG:  duration:
457.971 ms  plan:
        Query Text: explain analyze select count(*) from
fdwtest.pgbench_accounts where aid> 1000;
        Foreign Scan  (cost=102.84..155.73 rows=1 width=8) (actual
time=457.965..457.967 rows=1 loops=1)
          Relations: Aggregate on (pgbench_accounts)
..
STATEMENT:  explain analyze select aid from fdwtest.pgbench_accounts where
aid> 1000 limit 1;
bg=client backend, db=testdb, usr=postgres, client=[local] ,
qryId=5870070636604972000 , txId=0, app=psql, line=19 LOG:  duration: 9.339
ms  plan:
        Query Text: explain analyze select aid from
fdwtest.pgbench_accounts where aid> 1000 limit 1;
        Foreign Scan on pgbench_accounts  (cost=100.00..100.24 rows=1
width=4) (actual time=9.335..9.336 rows=1 loops=1)

## Remote Machine Logs (192.168.1.69) – Cropped
STATEMENT:  SELECT count(*) FROM public.pgbench_accounts WHERE ((aid >
1000))
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 ,
qryId=-7176633966431489392 , txId=0, app=postgres_fdw, line=22 LOG:
 execute <unnamed>: SELECT count(*) FROM public.pgbench_accounts WHERE
((aid > 1000))
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 , qryId=0 ,
txId=0, app=postgres_fdw, line=23 LOG:  statement: COMMIT TRANSACTION
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 ,
qryId=-2835399305386018931 , txId=0, app=postgres_fdw, line=24 LOG:
 duration: 455.461 ms  plan:
        Query Text: SELECT count(*) FROM public.pgbench_accounts WHERE
((aid > 1000))
        Finalize Aggregate  (cost=113216.98..113216.99 rows=1 width=8)
(actual time=454.321..455.452 rows=1 loops=1)
          ->  Gather  (cost=113216.97..113216.98 rows=2 width=8) (actual
time=454.173..455.443 rows=3 loops=1)
                Workers Planned: 2
                Workers Launched: 2
                ->  Partial Aggregate  (cost=113216.97..113216.98 rows=1
width=8) (actual time=449.393..449.394 rows=1 loops=3)
                      ->  Parallel Seq Scan on pgbench_accounts
 (cost=0.00..108009.67 rows=2082920 width=0) (actual time=0.255..343.378
rows=1666333 loops=3)
                            Filter: (aid > 1000)
                            Rows Removed by Filter: 333
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 , qryId=0 ,
txId=0, app=postgres_fdw, line=25 LOG:  statement: START TRANSACTION
ISOLATION LEVEL REPEATABLE READ
..
STATEMENT:  SELECT aid FROM public.pgbench_accounts WHERE ((aid > 1000))
LIMIT 1::bigint
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 ,
qryId=5994602644362067232 , txId=0, app=postgres_fdw, line=29 LOG:  execute
<unnamed>: SELECT aid FROM public.pgbench_accounts WHERE ((aid > 1000))
LIMIT 1::bigint
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 , qryId=0 ,
txId=0, app=postgres_fdw, line=30 LOG:  statement: COMMIT TRANSACTION
bg=client backend, db=testdb, usr=postgres, client=192.168.1.68 ,
qryId=-2835399305386018931 , txId=0, app=postgres_fdw, line=31 LOG:
 duration: 7.983 ms  plan:
        Query Text: SELECT aid FROM public.pgbench_accounts WHERE ((aid >
1000)) LIMIT 1::bigint
        Limit  (cost=0.00..0.02 rows=1 width=4) (actual time=0.836..7.974
rows=1 loops=1)
          ->  Gather  (cost=0.00..108009.67 rows=4999007 width=4) (actual
time=0.834..7.972 rows=1 loops=1)
                Workers Planned: 2
                Workers Launched: 2
                ->  Parallel Seq Scan on pgbench_accounts
 (cost=0.00..108009.67 rows=2082920 width=4) (actual time=0.270..0.271
rows=1 loops=3)
                      Filter: (aid > 1000)
                      Rows Removed by Filter: 333



if you would like me to conduct more complex tests, feel free to let me
know.
Best regards,
Kenan YILMAZ



KENAN YILMAZ <kenan.yilmaz@localus.com.tr>, 17 Şub 2025 Pzt, 17:09
tarihinde şunu yazdı:

> Hi Rafia,
>
> Based on our previous private discussion, thanks for the update and for
> clarifying the current state of the patch.
> I understand that more substantial changes are on the way, so I’ll focus
> on relevant test scenarios rather than performance testing at this stage.
>
> I will proceed with expanding the scenarios, including:
>
> Multiple postgres_fdw servers are active at the same time
> Multiple connections from the same postgres_fdw are active concurrently
> Multiple transactions run simultaneously on a single connection
> Multiple sessions operate from a single active connection
>
> I will submit the results of these tests to this mail thread so that they
> can benefit the broader community as well. Additionally, once you publish
> the updated version of the patch, I will rerun the tests with the latest
> changes and share the updated results.
>
> Best Regards,
>
> Rafia Sabih <rafia.pghackers@gmail.com>, 17 Şub 2025 Pzt, 16:46 tarihinde
> şunu yazdı:
>
>> On Tue, 14 Jan 2025 at 18:33, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>>> On Mon, Jan 6, 2025 at 3:52 AM Rafia Sabih <rafia.pghackers@gmail.com>
>>> wrote:
>>> > Now, to overcome this limitation, I have worked on this idea
>>> (suggested by my colleague Bernd Helmle) of bypassing the cursors. The way
>>> it works is as follows,
>>> > there is a new GUC introduced postgres_fdw.use_cursor, which when
>>> unset uses the mode without the cursor. Now, it uses PQsetChunkedRowsMode
>>> in create_cursor when non-cursor mode is used. The size of the chunk is the
>>> same as the fetch_size. Now in fetch_more_data, when non-cursor mode is
>>> used, pgfdw_get_next_result is used to get the chunk in PGresult and
>>> processed in the same manner as before.
>>> >
>>> > Now, the issue comes when there are simultaneous queries, which is the
>>> case with the join queries where all the tables involved in the join are at
>>> the local server. Because in that case we have multiple cursors opened at
>>> the same time and without a cursor mechanism we do not have any information
>>> or any other structure to know what to fetch from which query. To handle
>>> that case, we have a flag only_query, which is unset as soon as we have
>>> assigned the cursor_number >= 2, in postgresBeginForeignScan. Now, in
>>> fetch_more data, when we find out that only_query is unset, then we fetch
>>> all the data for the query and store it in a Tuplestore. These tuples are
>>> then transferred to the fsstate->tuples and then processed as usual.
>>> >
>>> > So yes there is a performance drawback in the case of simultaneous
>>> queries, however, the ability to use parallel plans is really an added
>>> advantage for the users. Plus, we can keep things as before by this new GUC
>>> -- use_cursor, in case we are losing more for some workloads.  So, in short
>>> I feel hopeful that this could be a good idea and a good time to improve
>>> postgres_fdw.
>>>
>>> Hi,
>>>
>>> I think it might have been nice to credit me in this post, since I
>>> made some relevant suggestions here off-list, in particular the idea
>>> of using a Tuplestore when there are multiple queries running. But I
>>> don't think this patch quite implements what I suggested. Here, you
>>> have a flag only_query which gets set to true at some point in time
>>> and thereafter remains true for the lifetime of a session. That means,
>>> I think, that all future queries will use the tuplestore even though
>>> there might not be multiple queries running any more. which doesn't
>>> seem like what we want. And, actually, this looks like it will be set
>>> as soon as you reach the second query in the same transaction, even if
>>> the two queries don't overlap. I think what you want to do is test
>>> whether, at the point where we would need to issue a new query,
>>> whether an existing query is already running. If not, move that
>>> query's remaining results into a Tuplestore so you can issue the new
>>> query.
>>>
>>> I'm not sure what the best way to implement that is, exactly. Perhaps
>>> fsstate->conn_state needs to store some more details about the
>>> connection, but that's just a guess. I don't think a global variable
>>> is what you want. Not only is that session-lifetime, but it applies
>>> globally to every connection to every server. You want to test
>>> something that is specific to one connection to one server, so it
>>> needs to be part of a data structure that is scoped that way.
>>>
>>> I think you'll want to figure out a good way to test this patch. I
>>> don't know if we need or can reasonably have automated test cases for
>>> this new functionality, but you at least want to have a good way to do
>>> manual testing, so that you can show that the tuplestore is used in
>>> cases where it's necessary and not otherwise. I'm not yet sure whether
>>> this patch needs automated test cases or whether they can reasonably
>>> be written, but you at least want to have a good procedure for manual
>>> validation so that you can verify that the Tuplestore is used in all
>>> the cases where it needs to be and, hopefully, no others.
>>>
>>> --
>>> Robert Haas
>>> EDB: http://www.enterprisedb.com
>>>
>>
>> Indeed you are right.
>> Firstly, accept my apologies for not mentioning you in credits for this
>> work. Thanks a lot for your efforts, discussions with you were helpful in
>> shaping this patch and bringing it to this level.
>>
>> Next, yes the last version was using tuplestore for queries within the
>> same transaction after the second query. To overcome this, I came across
>> this method to identify if there is any other simultaneous query running
>> with the current query; now there is an int variable num_queries which is
>> incremented at every call of postgresBeginForeignScan and decremented at
>> every call of postgresEndForeignScan. This way, if there are simultaneous
>> queries running we get the value of num_queries greater than 1. Now, we
>> check the value of num_queries and use tuplestore only when num_queries is
>> greater than 1. So, basically the understanding here is that if
>> postgresBeginForeignScan is called and before the call of
>> postgresEndForeignScan if another call to postgresBeginForeignScan is made,
>> then these are simultaneous queries.
>>
>> I couldn't really find any automated method of testing this, but did it
>> manually by debugging and/or printing log statements in
>> postgresBeginForeingScan, postgresEndForeignScan, and fetch_more_data to
>> confirm indeed there are simultaneous queries, and only they are using
>> tuplestore. So, the case of simultaneous queries I found was the join
>> query. Because, there it creates the cursor for one side of the join and
>> retrieves the first tuples for it and then creates the next cursor for the
>> other side of join and keeps on reading all the tuples for that query and
>> then it comes back to first cursor and retrieves all the tuples for that
>> one. Similarly, it works for the queries with n number of tables in join,
>> basically what I found is if there are n tables in the join there will be n
>> open cursors at a time and then they will be closed one by one in the
>> descending order of the cursor_number. I will think more on the topic of
>> testing this and will try to come up with a script (in the best case) to
>> confirm the use of tuplestore in required cases only, or atleast with a set
>> of steps to do so.
>>
>> For the regular testing of this feature, I think a regression test with
>> this new GUC postgres_fdw.use_cursor set to false and running all the
>> existing tests of postgres_fdw should suffice. What do you think? However,
>> at the moment when non-cursor mode is used, regression tests are failing.
>> Some queries require order by because order is changed in non-cursor mode,
>> but some require more complex changes, I am working on them.
>>
>> In this version of the patch I have added only the changes mentioned
>> above and not the regression test modification.
>>
>> --
>> Regards,
>> Rafia Sabih
>> CYBERTEC PostgreSQL International GmbH
>>
>

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Andy Fan <zhihuifan1213@163.com> — 2025-03-12T11:57:34Z

Rafia Sabih <rafia.pghackers@gmail.com> writes:


Hi,

>
> At present, in postgres_fdw, if a query which is using a parallel plan is fired from the remote end fails to use the
> parallel plan locally because of the presence of CURSORS. Consider the following example,
...
>
> Now, to overcome this limitation, I have worked on this idea (suggested by my colleague Bernd Helmle) of bypassing the
> cursors.

Do you know why we can't use parallel plan when cursor is used? Is It
related to this code in ExecutePlan?


	/*
	 * Set up parallel mode if appropriate.
	 *
	 * Parallel mode only supports complete execution of a plan.  If we've
	 * already partially executed it, or if the caller asks us to exit early,
	 * we must force the plan to run without parallelism.
	 */
	if (queryDesc->already_executed || numberTuples != 0)
     		use_parallel_mode = false;

Actually I can't understand the comment as well and I had this
confusion for a long time.

-- 
Best Regards
Andy Fan

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2025-09-29T14:51:13Z

Hello hackers,

I am back at this work with a rebased and revised patch. The new version is
rebased and has a change in approach.
Whenever we are using non-cursor mode, for the first cursor we are always
saving the tuples
in the tuplestore, this is because we do not have any means to know
beforehand how many cursors are required for the query.
And when we switch to the next query then we do not have a way to fetch the
tuples for the previous query.
So, the tuples retrieved earlier for the first query were lost if not saved.
I would highly appreciate your time and feedback for this.

On Wed, 12 Mar 2025 at 12:57, Andy Fan <zhihuifan1213@163.com> wrote:

> Rafia Sabih <rafia.pghackers@gmail.com> writes:
>
>
> Hi,
>
> >
> > At present, in postgres_fdw, if a query which is using a parallel plan
> is fired from the remote end fails to use the
> > parallel plan locally because of the presence of CURSORS. Consider the
> following example,
> ...
> >
> > Now, to overcome this limitation, I have worked on this idea (suggested
> by my colleague Bernd Helmle) of bypassing the
> > cursors.
>
> Do you know why we can't use parallel plan when cursor is used? Is It
> related to this code in ExecutePlan?
>
>
>         /*
>          * Set up parallel mode if appropriate.
>          *
>          * Parallel mode only supports complete execution of a plan.  If
> we've
>          * already partially executed it, or if the caller asks us to exit
> early,
>          * we must force the plan to run without parallelism.
>          */
>         if (queryDesc->already_executed || numberTuples != 0)
>                 use_parallel_mode = false;
>
> Actually I can't understand the comment as well and I had this
> confusion for a long time.
>
> --
> Best Regards
> Andy Fan
>
>

-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2025-10-10T19:35:16Z

On Wed, Mar 12, 2025 at 7:57 AM Andy Fan <zhihuifan1213@163.com> wrote:
> Do you know why we can't use parallel plan when cursor is used? Is It
> related to this code in ExecutePlan?

Yes. When a cursor is used, the whole query isn't executed all at
once, but rather the executor will be started and stopped for each
fetch from the cursor. We can't keep the parallel workers running for
that whole time, not just because it would be inefficient, but because
it would be incorrect. State changes would be possible in the leader
that were not reflected in the workers, leading to chaos.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2025-10-10T20:02:27Z

On Mon, Sep 29, 2025 at 10:51 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> I am back at this work with a rebased and revised patch. The new version is rebased and has a change in approach.
> Whenever we are using non-cursor mode, for the first cursor we are always saving the tuples
> in the tuplestore, this is because we do not have any means to know beforehand how many cursors are required for the query.

This might have the advantage of being simpler, but it's definitely
worse. If we're only fetching one result set, which will be common,
we'll buffer the whole thing in a tuplestore where that could
otherwise be avoided. Maybe it's still best to go with this approach;
not sure.

> And when we switch to the next query then we do not have a way to fetch the tuples for the previous query.
> So, the tuples retrieved earlier for the first query were lost if not saved.
> I would highly appreciate your time and feedback for this.

My suggestions are to work on the following areas:

1. Automated testing. The patch has no regression tests, and won't get
committed without those.

2. Manual testing. How does the performance with this new option
compare to the existing method? The answer might be different for
small result sets that fit in memory and large ones that spill to
disk, and will certainly also depend on how productive parallel query
can be on the remote side; but I think you want to do and post on this
list some measurements showing the best and worst case for the patch.

3. Patch clean-up. There are plenty of typos and whitespace-only
changes in the patch. It's best to clean those up. Running pgindent is
a good idea too. Some places could use comments.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2025-11-14T16:06:08Z

On Fri, 10 Oct 2025 at 22:02, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Sep 29, 2025 at 10:51 AM Rafia Sabih <rafia.pghackers@gmail.com>
wrote:
> > I am back at this work with a rebased and revised patch. The new
version is rebased and has a change in approach.
> > Whenever we are using non-cursor mode, for the first cursor we are
always saving the tuples
> > in the tuplestore, this is because we do not have any means to know
beforehand how many cursors are required for the query.
>
> This might have the advantage of being simpler, but it's definitely
> worse. If we're only fetching one result set, which will be common,
> we'll buffer the whole thing in a tuplestore where that could
> otherwise be avoided. Maybe it's still best to go with this approach;
> not sure.
>
> > And when we switch to the next query then we do not have a way to fetch
the tuples for the previous query.
> > So, the tuples retrieved earlier for the first query were lost if not
saved.
> > I would highly appreciate your time and feedback for this.
>
> My suggestions are to work on the following areas:
>
> 1. Automated testing. The patch has no regression tests, and won't get
> committed without those.
>
> 2. Manual testing. How does the performance with this new option
> compare to the existing method? The answer might be different for
> small result sets that fit in memory and large ones that spill to
> disk, and will certainly also depend on how productive parallel query
> can be on the remote side; but I think you want to do and post on this
> list some measurements showing the best and worst case for the patch.
>
> 3. Patch clean-up. There are plenty of typos and whitespace-only
> changes in the patch. It's best to clean those up. Running pgindent is
> a good idea too. Some places could use comments.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com

Thank you Robert, for reviewing this patch. On going through the patch
more, I realised this was not equipped to handle the cases when there are
more than two active cursors. So to accommodate such a case, I now modified
the new struct for saving the previous query to a list of such structs.
Also, it turns out we need not to save the tuples in case this is an active
cursor, so we only populate the associated tuplestore only when we need to
create a new cursor when the old cursor is not completely done.

In this version I have added the regression tests as well. I wanted to test
this patch for all the cases of postgres_fdw, the only way I could figure
out how to do this was to test the select statements with the new GUC.

I also did some tests for performance. I used the contrib_regression
database and populated the table "S1"."T1" with more tuples to understand
the impact of patch on higher scale. I also used auto_explain to get the
foreign plans.

contrib_regression=# select count(*) from ft1;
2025-11-14 14:40:35.825 CET [44338] LOG:  duration: 61.336 ms  plan:
Query Text: select count(*) from ft1;
Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
  Relations: Aggregate on (ft1)
2025-11-14 14:40:35.825 CET [44862] LOG:  duration: 60.575 ms  plan:
Query Text: DECLARE c1 CURSOR FOR
SELECT count(*) FROM "S 1"."T 1"
Aggregate  (cost=21888.22..21888.23 rows=1 width=8)
  ->  Seq Scan on "T 1"  (cost=0.00..19956.98 rows=772498 width=0)
 count
--------
 990821
(1 row)

Time: 62.728 ms
contrib_regression=# SET postgres_fdw.use_cursor = false;
SET
Time: 1.515 ms
contrib_regression=# select count(*) from ft1;
2025-11-14 14:40:46.260 CET [44862] LOG:  duration: 21.875 ms  plan:
Query Text: SELECT count(*) FROM "S 1"."T 1"
Finalize Aggregate  (cost=17255.64..17255.65 rows=1 width=8)
  ->  Gather  (cost=17255.43..17255.64 rows=2 width=8)
        Workers Planned: 2
        ->  Partial Aggregate  (cost=16255.43..16255.44 rows=1 width=8)
              ->  Parallel Seq Scan on "T 1"  (cost=0.00..15450.74
rows=321874 width=0)
2025-11-14 14:40:46.260 CET [44338] LOG:  duration: 22.623 ms  plan:
Query Text: select count(*) from ft1;
Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
  Relations: Aggregate on (ft1)
 count
--------
 990821
(1 row)

Time: 24.862 ms

So for this query, the advantage is coming from parallel query which was
otherwise not possible in this scenario.
To study more performance with this patch, I found another interesting
query and here are the results,

contrib_regression=# SET postgres_fdw.use_cursor = true;
SET
contrib_regression=# explain  (analyse, buffers)  SELECT t1."C 1" FROM "S
1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 =
t1."C 1");2025-11-14 15:57:46.893 CET [1946]
                                                              QUERY PLAN

---------------------------------------------------------------------------------------------------------------------------------------
 Hash Left Join  (cost=109013.50..131877.35 rows=772498 width=4) (actual
time=112311.578..112804.516 rows=990821.00 loops=1)
   Hash Cond: (t1."C 1" = t3.c1)
   Buffers: shared hit=12232, temp read=12754 written=12754
   ->  Seq Scan on "T 1" t1  (cost=0.00..19956.98 rows=772498 width=4)
(actual time=0.039..48.808 rows=990821.00 loops=1)
         Buffers: shared hit=12232
   ->  Hash  (cost=109001.00..109001.00 rows=1000 width=4) (actual
time=112310.386..112310.387 rows=990821.00 loops=1)
         Buckets: 262144 (originally 1024)  Batches: 8 (originally 1)
 Memory Usage: 6408kB
         Buffers: temp written=2537
         ->  Nested Loop  (cost=200.43..109001.00 rows=1000 width=4)
(actual time=0.728..112030.241 rows=990821.00 loops=1)
               ->  Foreign Scan on ft1 t2  (cost=100.00..331.00 rows=1000
width=4) (actual time=0.398..710.505 rows=990821.00 loops=1)
               ->  Foreign Scan on ft2 t3  (cost=100.43..108.66 rows=1
width=4) (actual time=0.082..0.082 rows=1.00 loops=990821)
 Planning:
   Buffers: shared hit=5
 Planning Time: 2.211 ms
 Execution Time: 112825.428 ms
(15 rows)

contrib_regression=# SET postgres_fdw.use_cursor = false;
SET
explain  (analyse, buffers)  SELECT t1."C 1" FROM "S 1"."T 1" t1 left join
ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1");
QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------
 Hash Left Join (cost=109013.50..131877.35 rows=772498 width=4) (actual
time=261.416..354.520 rows=990821.00 loops=1)
  Hash Cond: (t1."C 1" = t3.c1)
  Buffers: shared hit=12232, temp written=2660
  -> Seq Scan on "T 1" t1 (cost=0.00..19956.98 rows=772498 width=4) (actual
time=0.021..35.531 rows=990821.00 loops=1)
     Buffers: shared hit=12232
  -> Hash (cost=109001.00..109001.00 rows=1000 width=4) (actual
time=261.381..261.383 rows=100.00 loops=1)
     Buckets: 1024 Batches: 1 Memory Usage: 12kB
     Buffers: temp written=2660
     -> Nested Loop (cost=200.43..109001.00 rows=1000 width=4) (actual
time=255.563..261.356 rows=100.00 loops=1)
        Buffers: temp written=2660
        -> Foreign Scan on ft1 t2 (cost=100.00..331.00 rows=1000 width=4)
(actual time=0.433..0.443 rows=100.00 loops=1)
        -> Foreign Scan on ft2 t3 (cost=100.43..108.66 rows=1 width=4)
(actual time=2.609..2.609 rows=1.00 loops=100)
           Buffers: temp written=2660
 Planning:
  Buffers: shared hit=5
 Planning Time: 2.284 ms
 Execution Time: 389.358 ms
(17 rows)

So even in the case without a parallel plan, it is performing significantly
better. I investigated a bit more to find out why the query was so slow
with the cursors,
and came to understand that it is repeatedly abandoning and recreating the
cursor via the code path of postgresReScanForeignScan.
So, it looks like cursor management itself costs this much more time.

To understand the impact of the patch in case of tuples spilled to disk, I
tried following example,
contrib_regression=# SET postgres_fdw.use_cursor = true;
SET
contrib_regression=# set enable_hashjoin = off;
SET
contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
where a.c1 = b.c1 order by a.c2;
                                                               QUERY PLAN

----------------------------------------------------------------------------------------------------------------------------------------
 Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
time=4537.437..4598.483 rows=990821.00 loops=1)
   Sort Key: a.c2
   Sort Method: external merge  Disk: 137768kB
   Buffers: temp read=83156 written=83253
   ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
time=3748.488..4090.547 rows=990821.00 loops=1)
         Merge Cond: (a.c1 = b.c1)
         Buffers: temp read=48725 written=48779
         ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
time=1818.521..1865.792 rows=990821.00 loops=1)
               Sort Key: a.c1
               Sort Method: external merge  Disk: 75664kB
               Buffers: temp read=18910 written=18937
               ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
width=47) (actual time=1.302..1568.640 rows=990821.00 loops=1)
         ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
time=1929.955..1981.104 rows=990821.00 loops=1)
               Sort Key: b.c1
               Sort Method: external sort  Disk: 79520kB
               Buffers: temp read=29815 written=29842
               ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
width=47) (actual time=0.528..1553.249 rows=990821.00 loops=1)
 Planning Time: 0.479 ms
 Execution Time: 4661.872 ms
(19 rows)

contrib_regression=# SET postgres_fdw.use_cursor = false;
SET
contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
where a.c1 = b.c1 order by a.c2;
                                                              QUERY PLAN

---------------------------------------------------------------------------------------------------------------------------------------
 Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
time=3376.385..3435.406 rows=990821.00 loops=1)
   Sort Key: a.c2
   Sort Method: external merge  Disk: 137768kB
   Buffers: temp read=83156 written=83253
   ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
time=2565.517..2916.814 rows=990821.00 loops=1)
         Merge Cond: (a.c1 = b.c1)
         Buffers: temp read=48725 written=48779
         ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
time=1249.517..1300.132 rows=990821.00 loops=1)
               Sort Key: a.c1
               Sort Method: external merge  Disk: 75664kB
               Buffers: temp read=18910 written=18937
               ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
width=47) (actual time=1.651..980.740 rows=990821.00 loops=1)
         ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
time=1315.990..1369.576 rows=990821.00 loops=1)
               Sort Key: b.c1
               Sort Method: external sort  Disk: 79520kB
               Buffers: temp read=29815 written=29842
               ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
width=47) (actual time=0.426..970.728 rows=990821.00 loops=1)
 Planning Time: 0.491 ms
 Execution Time: 3527.457 ms
(19 rows)

So it doesn't hurt in this case either. But that is because not the
tuplestore which is added in this patch is spilling to disk here.
I am working on finding a case when there are two or more active cursors
and then when storing the tuples of one cursor,
they are spilled onto the disk. That might give a picture of the worst case
scenario of this patch.

Also, thanks to Kenan, a fellow hacker who finds this work interesting and
offered to do some performance analysis for this patch,
maybe he can also post more results here.

-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

KENAN YILMAZ <kenan.yilmaz@localus.com.tr> — 2025-11-25T14:24:14Z

Hello Hackers,

I have executed use_cursor = true/false with quick tests.

The test table 't1' has 20,000,000 rows (~2.8 GB in size).

The test query is `EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE
a > 1000;`

The result sums are;

** `postgres_fdw.use_cursor = true` --> 20090.782 ms
** `postgres_fdw.use_cursor = false` --> 24103.994 ms

If you wish to run more advanced and complex test queries, feel free to do
so.

All execution test scenarios are listed below;

--
-- restart with flush caches;
$ pg_ctl -D $PGDATA stop && sudo sync && echo 3 | sudo tee
/proc/sys/vm/drop_caches && rm $PGDATA/log/*.log && pg_ctl -D $PGDATA start
-- Data prep stage
DROP DATABASE IF EXISTS testdb;
CREATE DATABASE testdb;
\c testdb;
DROP TABLE IF EXISTS t1 CASCADE;
CREATE UNLOGGED TABLE t1 (a int, b int, c text, d timestamp);

-- Insert test datas
INSERT INTO t1 SELECT 10 + mod(i, 30), i, md5(i::text) ||
md5((i+1000000)::text) || md5((i+2000000)::text), '2025-01-01
10:00:00'::timestamp + (random() * 31536000) * INTERVAL '1 second' FROM
generate_series(1, 20000000) i;

-- Table maintenance
ALTER TABLE t1 SET LOGGED;
VACUUM (ANALYZE, FULL) t1;

-- FDW prep stage
DROP EXTENSION IF EXISTS postgres_fdw CASCADE;
CREATE EXTENSION postgres_fdw;
CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host '127.0.0.1', port '5432', dbname 'testdb');
CREATE USER MAPPING FOR postgres SERVER foreign_server OPTIONS (user
'postgres');
CREATE FOREIGN TABLE t1fdw (a int, b int, c text, d timestamp) SERVER
foreign_server OPTIONS (table_name 't1');

-- restart with flush caches are applied before all stage executions.

--
-- * Local t1 table EXPLAIN results
--

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1 WHERE a > 1000;
                                                         QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
time=23260.306..23261.591 rows=0.00 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   Buffers: shared read=363637
   I/O Timings: shared read=64590.910
   ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1 width=105)
(actual time=23242.279..23242.280 rows=0.00 loops=3)
         Filter: (a > 1000)
         Rows Removed by Filter: 6666667
         Buffers: shared read=363637
         I/O Timings: shared read=64590.910
 Planning:
   Buffers: shared hit=54 read=14 dirtied=1
   I/O Timings: shared read=23.281
 Planning Time: 38.734 ms
 Execution Time: 23269.677 ms
(15 rows)

Time: 23347.716 ms (00:23.348)

--
-- * use_cursor = true (Default) EXPLAIN results
--

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
                                                     QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
time=20074.746..20074.796 rows=0.00 loops=1)
 Planning:
   Buffers: shared hit=33 read=17 dirtied=1
   I/O Timings: shared read=10.696
 Planning Time: 43.852 ms
 Execution Time: 20090.782 ms
(6 rows)

Time: 20169.081 ms (00:20.169)

--> From Log Files
[153045]: line=11 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
TRANSACTION ISOLATION LEVEL REPEATABLE READ
[153045]: line=12 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[153045]: line=13 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: execute
<unnamed>: DECLARE c1 CURSOR FOR
        SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
[153045]: line=14 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
 exec_execute_message, postgres.c:2245
[153045]: line=15 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: FETCH
100 FROM c1
[153045]: line=16 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[153044]: line=5 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOG:  00000: duration: 20074.799 ms  plan:
        Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
WHERE a > 1000;
        Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
(actual time=20074.746..20074.796 rows=0.00 loops=1)
[153044]: line=6 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
[153045]: line=17 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: CLOSE
c1
[153045]: line=18 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[153045]: line=19 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: duration:
20057.543 ms  plan:
        Query Text: DECLARE c1 CURSOR FOR
        SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
        Seq Scan on t1  (cost=0.00..613637.45 rows=1 width=105) (actual
time=20057.541..20057.541 rows=0.00 loops=1)
          Filter: (a > 1000)
          Rows Removed by Filter: 20000000
[153045]: line=20 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
 explain_ExecutorEnd, auto_explain.c:437
[153045]: line=21 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: COMMIT
TRANSACTION

--
-- * use_cursor = false EXPLAIN results
--

testdb=# SET postgres_fdw.use_cursor = false;

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
                                                     QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
time=24080.945..24080.956 rows=0.00 loops=1)
 Planning:
   Buffers: shared hit=33 read=17
   I/O Timings: shared read=30.074
 Planning Time: 53.678 ms
 Execution Time: 24103.994 ms
(6 rows)

Time: 24230.548 ms (00:24.231)

--> From Log Files
[153121]: line=11 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
TRANSACTION ISOLATION LEVEL REPEATABLE READ
[153121]: line=12 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[153121]: line=13 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: execute <unnamed>: SELECT
a, b, c FROM public.t1 WHERE ((a > 1000))
[153121]: line=14 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_execute_message,
postgres.c:2245
[153113]: line=7 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOG:  00000: duration: 24080.958 ms  plan:
        Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
WHERE a > 1000;
        Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
(actual time=24080.945..24080.956 rows=0.00 loops=1)
[153113]: line=8 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
[153121]: line=15 sid=6923fc16.25621 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: COMMIT
TRANSACTION
[153121]: line=16 sid=6923fc16.25621 tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[153121]: line=17 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: duration: 24059.372 ms
 plan:
        Query Text: SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
        Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
time=24058.076..24059.367 rows=0.00 loops=1)
          Workers Planned: 2
          Workers Launched: 2
          ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1
width=105) (actual time=24051.406..24051.407 rows=0.00 loops=3)
                Filter: (a > 1000)
                Rows Removed by Filter: 6666667
[153121]: line=18 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  explain_ExecutorEnd,
auto_explain.c:437

---

Kenan YILMAZ



Rafia Sabih <rafia.pghackers@gmail.com>, 25 Kas 2025 Sal, 17:01 tarihinde
şunu yazdı:

>
>
> On Fri, 10 Oct 2025 at 22:02, Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Mon, Sep 29, 2025 at 10:51 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > > I am back at this work with a rebased and revised patch. The new
> version is rebased and has a change in approach.
> > > Whenever we are using non-cursor mode, for the first cursor we are
> always saving the tuples
> > > in the tuplestore, this is because we do not have any means to know
> beforehand how many cursors are required for the query.
> >
> > This might have the advantage of being simpler, but it's definitely
> > worse. If we're only fetching one result set, which will be common,
> > we'll buffer the whole thing in a tuplestore where that could
> > otherwise be avoided. Maybe it's still best to go with this approach;
> > not sure.
> >
> > > And when we switch to the next query then we do not have a way to
> fetch the tuples for the previous query.
> > > So, the tuples retrieved earlier for the first query were lost if not
> saved.
> > > I would highly appreciate your time and feedback for this.
> >
> > My suggestions are to work on the following areas:
> >
> > 1. Automated testing. The patch has no regression tests, and won't get
> > committed without those.
> >
> > 2. Manual testing. How does the performance with this new option
> > compare to the existing method? The answer might be different for
> > small result sets that fit in memory and large ones that spill to
> > disk, and will certainly also depend on how productive parallel query
> > can be on the remote side; but I think you want to do and post on this
> > list some measurements showing the best and worst case for the patch.
> >
> > 3. Patch clean-up. There are plenty of typos and whitespace-only
> > changes in the patch. It's best to clean those up. Running pgindent is
> > a good idea too. Some places could use comments.
> >
> > --
> > Robert Haas
> > EDB: http://www.enterprisedb.com
>
> Thank you Robert, for reviewing this patch. On going through the patch
> more, I realised this was not equipped to handle the cases when there are
> more than two active cursors. So to accommodate such a case, I now modified
> the new struct for saving the previous query to a list of such structs.
> Also, it turns out we need not to save the tuples in case this is an active
> cursor, so we only populate the associated tuplestore only when we need to
> create a new cursor when the old cursor is not completely done.
>
> In this version I have added the regression tests as well. I wanted to
> test this patch for all the cases of postgres_fdw, the only way I could
> figure out how to do this was to test the select statements with the new
> GUC.
>
> I also did some tests for performance. I used the contrib_regression
> database and populated the table "S1"."T1" with more tuples to understand
> the impact of patch on higher scale. I also used auto_explain to get the
> foreign plans.
>
> contrib_regression=# select count(*) from ft1;
> 2025-11-14 14:40:35.825 CET [44338] LOG:  duration: 61.336 ms  plan:
> Query Text: select count(*) from ft1;
> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>   Relations: Aggregate on (ft1)
> 2025-11-14 14:40:35.825 CET [44862] LOG:  duration: 60.575 ms  plan:
> Query Text: DECLARE c1 CURSOR FOR
> SELECT count(*) FROM "S 1"."T 1"
> Aggregate  (cost=21888.22..21888.23 rows=1 width=8)
>   ->  Seq Scan on "T 1"  (cost=0.00..19956.98 rows=772498 width=0)
>  count
> --------
>  990821
> (1 row)
>
> Time: 62.728 ms
> contrib_regression=# SET postgres_fdw.use_cursor = false;
> SET
> Time: 1.515 ms
> contrib_regression=# select count(*) from ft1;
> 2025-11-14 14:40:46.260 CET [44862] LOG:  duration: 21.875 ms  plan:
> Query Text: SELECT count(*) FROM "S 1"."T 1"
> Finalize Aggregate  (cost=17255.64..17255.65 rows=1 width=8)
>   ->  Gather  (cost=17255.43..17255.64 rows=2 width=8)
>         Workers Planned: 2
>         ->  Partial Aggregate  (cost=16255.43..16255.44 rows=1 width=8)
>               ->  Parallel Seq Scan on "T 1"  (cost=0.00..15450.74
> rows=321874 width=0)
> 2025-11-14 14:40:46.260 CET [44338] LOG:  duration: 22.623 ms  plan:
> Query Text: select count(*) from ft1;
> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>   Relations: Aggregate on (ft1)
>  count
> --------
>  990821
> (1 row)
>
> Time: 24.862 ms
>
> So for this query, the advantage is coming from parallel query which was
> otherwise not possible in this scenario.
> To study more performance with this patch, I found another interesting
> query and here are the results,
>
> contrib_regression=# SET postgres_fdw.use_cursor = true;
> SET
> contrib_regression=# explain  (analyse, buffers)  SELECT t1."C 1" FROM "S
> 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 =
> t1."C 1");2025-11-14 15:57:46.893 CET [1946]
>                                                               QUERY PLAN
>
>
> ---------------------------------------------------------------------------------------------------------------------------------------
>  Hash Left Join  (cost=109013.50..131877.35 rows=772498 width=4) (actual
> time=112311.578..112804.516 rows=990821.00 loops=1)
>    Hash Cond: (t1."C 1" = t3.c1)
>    Buffers: shared hit=12232, temp read=12754 written=12754
>    ->  Seq Scan on "T 1" t1  (cost=0.00..19956.98 rows=772498 width=4)
> (actual time=0.039..48.808 rows=990821.00 loops=1)
>          Buffers: shared hit=12232
>    ->  Hash  (cost=109001.00..109001.00 rows=1000 width=4) (actual
> time=112310.386..112310.387 rows=990821.00 loops=1)
>          Buckets: 262144 (originally 1024)  Batches: 8 (originally 1)
>  Memory Usage: 6408kB
>          Buffers: temp written=2537
>          ->  Nested Loop  (cost=200.43..109001.00 rows=1000 width=4)
> (actual time=0.728..112030.241 rows=990821.00 loops=1)
>                ->  Foreign Scan on ft1 t2  (cost=100.00..331.00 rows=1000
> width=4) (actual time=0.398..710.505 rows=990821.00 loops=1)
>                ->  Foreign Scan on ft2 t3  (cost=100.43..108.66 rows=1
> width=4) (actual time=0.082..0.082 rows=1.00 loops=990821)
>  Planning:
>    Buffers: shared hit=5
>  Planning Time: 2.211 ms
>  Execution Time: 112825.428 ms
> (15 rows)
>
> contrib_regression=# SET postgres_fdw.use_cursor = false;
> SET
> explain  (analyse, buffers)  SELECT t1."C 1" FROM "S 1"."T 1" t1 left join
> ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1");
> QUERY PLAN
>
> ----------------------------------------------------------------------------------------------------------------------------------
>  Hash Left Join (cost=109013.50..131877.35 rows=772498 width=4) (actual
> time=261.416..354.520 rows=990821.00 loops=1)
>   Hash Cond: (t1."C 1" = t3.c1)
>   Buffers: shared hit=12232, temp written=2660
>   -> Seq Scan on "T 1" t1 (cost=0.00..19956.98 rows=772498 width=4)
> (actual time=0.021..35.531 rows=990821.00 loops=1)
>      Buffers: shared hit=12232
>   -> Hash (cost=109001.00..109001.00 rows=1000 width=4) (actual
> time=261.381..261.383 rows=100.00 loops=1)
>      Buckets: 1024 Batches: 1 Memory Usage: 12kB
>      Buffers: temp written=2660
>      -> Nested Loop (cost=200.43..109001.00 rows=1000 width=4) (actual
> time=255.563..261.356 rows=100.00 loops=1)
>         Buffers: temp written=2660
>         -> Foreign Scan on ft1 t2 (cost=100.00..331.00 rows=1000 width=4)
> (actual time=0.433..0.443 rows=100.00 loops=1)
>         -> Foreign Scan on ft2 t3 (cost=100.43..108.66 rows=1 width=4)
> (actual time=2.609..2.609 rows=1.00 loops=100)
>            Buffers: temp written=2660
>  Planning:
>   Buffers: shared hit=5
>  Planning Time: 2.284 ms
>  Execution Time: 389.358 ms
> (17 rows)
>
> So even in the case without a parallel plan, it is performing
> significantly better. I investigated a bit more to find out why the query
> was so slow with the cursors,
> and came to understand that it is repeatedly abandoning and recreating the
> cursor via the code path of postgresReScanForeignScan.
> So, it looks like cursor management itself costs this much more time.
>
> To understand the impact of the patch in case of tuples spilled to disk, I
> tried following example,
> contrib_regression=# SET postgres_fdw.use_cursor = true;
> SET
> contrib_regression=# set enable_hashjoin = off;
> SET
> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
> where a.c1 = b.c1 order by a.c2;
>                                                                QUERY PLAN
>
>
> ----------------------------------------------------------------------------------------------------------------------------------------
>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
> time=4537.437..4598.483 rows=990821.00 loops=1)
>    Sort Key: a.c2
>    Sort Method: external merge  Disk: 137768kB
>    Buffers: temp read=83156 written=83253
>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
> time=3748.488..4090.547 rows=990821.00 loops=1)
>          Merge Cond: (a.c1 = b.c1)
>          Buffers: temp read=48725 written=48779
>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
> time=1818.521..1865.792 rows=990821.00 loops=1)
>                Sort Key: a.c1
>                Sort Method: external merge  Disk: 75664kB
>                Buffers: temp read=18910 written=18937
>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
> width=47) (actual time=1.302..1568.640 rows=990821.00 loops=1)
>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
> time=1929.955..1981.104 rows=990821.00 loops=1)
>                Sort Key: b.c1
>                Sort Method: external sort  Disk: 79520kB
>                Buffers: temp read=29815 written=29842
>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
> width=47) (actual time=0.528..1553.249 rows=990821.00 loops=1)
>  Planning Time: 0.479 ms
>  Execution Time: 4661.872 ms
> (19 rows)
>
> contrib_regression=# SET postgres_fdw.use_cursor = false;
> SET
> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
> where a.c1 = b.c1 order by a.c2;
>                                                               QUERY PLAN
>
>
> ---------------------------------------------------------------------------------------------------------------------------------------
>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
> time=3376.385..3435.406 rows=990821.00 loops=1)
>    Sort Key: a.c2
>    Sort Method: external merge  Disk: 137768kB
>    Buffers: temp read=83156 written=83253
>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
> time=2565.517..2916.814 rows=990821.00 loops=1)
>          Merge Cond: (a.c1 = b.c1)
>          Buffers: temp read=48725 written=48779
>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
> time=1249.517..1300.132 rows=990821.00 loops=1)
>                Sort Key: a.c1
>                Sort Method: external merge  Disk: 75664kB
>                Buffers: temp read=18910 written=18937
>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
> width=47) (actual time=1.651..980.740 rows=990821.00 loops=1)
>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
> time=1315.990..1369.576 rows=990821.00 loops=1)
>                Sort Key: b.c1
>                Sort Method: external sort  Disk: 79520kB
>                Buffers: temp read=29815 written=29842
>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
> width=47) (actual time=0.426..970.728 rows=990821.00 loops=1)
>  Planning Time: 0.491 ms
>  Execution Time: 3527.457 ms
> (19 rows)
>
> So it doesn't hurt in this case either. But that is because not the
> tuplestore which is added in this patch is spilling to disk here.
> I am working on finding a case when there are two or more active cursors
> and then when storing the tuples of one cursor,
> they are spilled onto the disk. That might give a picture of the worst
> case scenario of this patch.
>
> Also, thanks to Kenan, a fellow hacker who finds this work interesting and
> offered to do some performance analysis for this patch,
> maybe he can also post more results here.
>
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2025-11-27T10:50:27Z

On Tue, 25 Nov 2025 at 15:24, KENAN YILMAZ <kenan.yilmaz@localus.com.tr>
wrote:

> Hello Hackers,
>
> I have executed use_cursor = true/false with quick tests.
>
> The test table 't1' has 20,000,000 rows (~2.8 GB in size).
>
> The test query is `EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
> WHERE a > 1000;`
>
> The result sums are;
>
> ** `postgres_fdw.use_cursor = true` --> 20090.782 ms
> ** `postgres_fdw.use_cursor = false` --> 24103.994 ms
>
> If you wish to run more advanced and complex test queries, feel free to do
> so.
>
> All execution test scenarios are listed below;
>
> --
> -- restart with flush caches;
> $ pg_ctl -D $PGDATA stop && sudo sync && echo 3 | sudo tee
> /proc/sys/vm/drop_caches && rm $PGDATA/log/*.log && pg_ctl -D $PGDATA start
> -- Data prep stage
> DROP DATABASE IF EXISTS testdb;
> CREATE DATABASE testdb;
> \c testdb;
> DROP TABLE IF EXISTS t1 CASCADE;
> CREATE UNLOGGED TABLE t1 (a int, b int, c text, d timestamp);
>
> -- Insert test datas
> INSERT INTO t1 SELECT 10 + mod(i, 30), i, md5(i::text) ||
> md5((i+1000000)::text) || md5((i+2000000)::text), '2025-01-01
> 10:00:00'::timestamp + (random() * 31536000) * INTERVAL '1 second' FROM
> generate_series(1, 20000000) i;
>
> -- Table maintenance
> ALTER TABLE t1 SET LOGGED;
> VACUUM (ANALYZE, FULL) t1;
>
> -- FDW prep stage
> DROP EXTENSION IF EXISTS postgres_fdw CASCADE;
> CREATE EXTENSION postgres_fdw;
> CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
> (host '127.0.0.1', port '5432', dbname 'testdb');
> CREATE USER MAPPING FOR postgres SERVER foreign_server OPTIONS (user
> 'postgres');
> CREATE FOREIGN TABLE t1fdw (a int, b int, c text, d timestamp) SERVER
> foreign_server OPTIONS (table_name 't1');
>
> -- restart with flush caches are applied before all stage executions.
>
> --
> -- * Local t1 table EXPLAIN results
> --
>
> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1 WHERE a > 1000;
>                                                          QUERY PLAN
>
> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>  Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
> time=23260.306..23261.591 rows=0.00 loops=1)
>    Workers Planned: 2
>    Workers Launched: 2
>    Buffers: shared read=363637
>    I/O Timings: shared read=64590.910
>    ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1 width=105)
> (actual time=23242.279..23242.280 rows=0.00 loops=3)
>          Filter: (a > 1000)
>          Rows Removed by Filter: 6666667
>          Buffers: shared read=363637
>          I/O Timings: shared read=64590.910
>  Planning:
>    Buffers: shared hit=54 read=14 dirtied=1
>    I/O Timings: shared read=23.281
>  Planning Time: 38.734 ms
>  Execution Time: 23269.677 ms
> (15 rows)
>
> Time: 23347.716 ms (00:23.348)
>
> --
> -- * use_cursor = true (Default) EXPLAIN results
> --
>
> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
>                                                      QUERY PLAN
>
> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>  Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
> time=20074.746..20074.796 rows=0.00 loops=1)
>  Planning:
>    Buffers: shared hit=33 read=17 dirtied=1
>    I/O Timings: shared read=10.696
>  Planning Time: 43.852 ms
>  Execution Time: 20090.782 ms
> (6 rows)
>
> Time: 20169.081 ms (00:20.169)
>
> --> From Log Files
> [153045]: line=11 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
> TRANSACTION ISOLATION LEVEL REPEATABLE READ
> [153045]: line=12 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
> postgres.c:1078
> [153045]: line=13 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: execute
> <unnamed>: DECLARE c1 CURSOR FOR
>         SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
> [153045]: line=14 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>  exec_execute_message, postgres.c:2245
> [153045]: line=15 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
> FETCH 100 FROM c1
> [153045]: line=16 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>  exec_simple_query, postgres.c:1078
> [153044]: line=5 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
> app=psql client=[local]: LOG:  00000: duration: 20074.799 ms  plan:
>         Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
> WHERE a > 1000;
>         Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
> (actual time=20074.746..20074.796 rows=0.00 loops=1)
> [153044]: line=6 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
> app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
> [153045]: line=17 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
> CLOSE c1
> [153045]: line=18 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>  exec_simple_query, postgres.c:1078
> [153045]: line=19 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: duration:
> 20057.543 ms  plan:
>         Query Text: DECLARE c1 CURSOR FOR
>         SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>         Seq Scan on t1  (cost=0.00..613637.45 rows=1 width=105) (actual
> time=20057.541..20057.541 rows=0.00 loops=1)
>           Filter: (a > 1000)
>           Rows Removed by Filter: 20000000
> [153045]: line=20 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>  explain_ExecutorEnd, auto_explain.c:437
> [153045]: line=21 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
> COMMIT TRANSACTION
>
> --
> -- * use_cursor = false EXPLAIN results
> --
>
> testdb=# SET postgres_fdw.use_cursor = false;
>
> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
>                                                      QUERY PLAN
>
> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>  Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
> time=24080.945..24080.956 rows=0.00 loops=1)
>  Planning:
>    Buffers: shared hit=33 read=17
>    I/O Timings: shared read=30.074
>  Planning Time: 53.678 ms
>  Execution Time: 24103.994 ms
> (6 rows)
>
> Time: 24230.548 ms (00:24.231)
>
> --> From Log Files
> [153121]: line=11 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
> TRANSACTION ISOLATION LEVEL REPEATABLE READ
> [153121]: line=12 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
> postgres.c:1078
> [153121]: line=13 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOG:  00000: execute <unnamed>: SELECT
> a, b, c FROM public.t1 WHERE ((a > 1000))
> [153121]: line=14 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_execute_message,
> postgres.c:2245
> [153113]: line=7 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
> app=psql client=[local]: LOG:  00000: duration: 24080.958 ms  plan:
>         Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
> WHERE a > 1000;
>         Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
> (actual time=24080.945..24080.956 rows=0.00 loops=1)
> [153113]: line=8 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
> app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
> [153121]: line=15 sid=6923fc16.25621 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
> COMMIT TRANSACTION
> [153121]: line=16 sid=6923fc16.25621 tag=idle in transaction usr=postgres
> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>  exec_simple_query, postgres.c:1078
> [153121]: line=17 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOG:  00000: duration: 24059.372 ms
>  plan:
>         Query Text: SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>         Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
> time=24058.076..24059.367 rows=0.00 loops=1)
>           Workers Planned: 2
>           Workers Launched: 2
>           ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1
> width=105) (actual time=24051.406..24051.407 rows=0.00 loops=3)
>                 Filter: (a > 1000)
>                 Rows Removed by Filter: 6666667
> [153121]: line=18 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
> app=postgres_fdw client=127.0.0.1: LOCATION:  explain_ExecutorEnd,
> auto_explain.c:437
>
> ---
>
> Kenan YILMAZ
>
Thanks Kenan for these. So, it looks like the patch performs the same as in
the local scan case. I wonder if you found any case of performance
degradation with the patch.

Per an off-list discussion with Robert, he suggested using the
existing data structures for recording the state of last queries instead of
inventing something new.
Makes sense, so I reworked the patch to include tuplestore in
PgFdwScanState and then use PgFdwScanState as part of PgFdwConnState to
keep track of previously
active cursors. Nothing else is changed in this version of the patch.

>
>
>
> Rafia Sabih <rafia.pghackers@gmail.com>, 25 Kas 2025 Sal, 17:01 tarihinde
> şunu yazdı:
>
>>
>>
>> On Fri, 10 Oct 2025 at 22:02, Robert Haas <robertmhaas@gmail.com> wrote:
>> >
>> > On Mon, Sep 29, 2025 at 10:51 AM Rafia Sabih <rafia.pghackers@gmail.com>
>> wrote:
>> > > I am back at this work with a rebased and revised patch. The new
>> version is rebased and has a change in approach.
>> > > Whenever we are using non-cursor mode, for the first cursor we are
>> always saving the tuples
>> > > in the tuplestore, this is because we do not have any means to know
>> beforehand how many cursors are required for the query.
>> >
>> > This might have the advantage of being simpler, but it's definitely
>> > worse. If we're only fetching one result set, which will be common,
>> > we'll buffer the whole thing in a tuplestore where that could
>> > otherwise be avoided. Maybe it's still best to go with this approach;
>> > not sure.
>> >
>> > > And when we switch to the next query then we do not have a way to
>> fetch the tuples for the previous query.
>> > > So, the tuples retrieved earlier for the first query were lost if not
>> saved.
>> > > I would highly appreciate your time and feedback for this.
>> >
>> > My suggestions are to work on the following areas:
>> >
>> > 1. Automated testing. The patch has no regression tests, and won't get
>> > committed without those.
>> >
>> > 2. Manual testing. How does the performance with this new option
>> > compare to the existing method? The answer might be different for
>> > small result sets that fit in memory and large ones that spill to
>> > disk, and will certainly also depend on how productive parallel query
>> > can be on the remote side; but I think you want to do and post on this
>> > list some measurements showing the best and worst case for the patch.
>> >
>> > 3. Patch clean-up. There are plenty of typos and whitespace-only
>> > changes in the patch. It's best to clean those up. Running pgindent is
>> > a good idea too. Some places could use comments.
>> >
>> > --
>> > Robert Haas
>> > EDB: http://www.enterprisedb.com
>>
>> Thank you Robert, for reviewing this patch. On going through the patch
>> more, I realised this was not equipped to handle the cases when there are
>> more than two active cursors. So to accommodate such a case, I now modified
>> the new struct for saving the previous query to a list of such structs.
>> Also, it turns out we need not to save the tuples in case this is an active
>> cursor, so we only populate the associated tuplestore only when we need to
>> create a new cursor when the old cursor is not completely done.
>>
>> In this version I have added the regression tests as well. I wanted to
>> test this patch for all the cases of postgres_fdw, the only way I could
>> figure out how to do this was to test the select statements with the new
>> GUC.
>>
>> I also did some tests for performance. I used the contrib_regression
>> database and populated the table "S1"."T1" with more tuples to understand
>> the impact of patch on higher scale. I also used auto_explain to get the
>> foreign plans.
>>
>> contrib_regression=# select count(*) from ft1;
>> 2025-11-14 14:40:35.825 CET [44338] LOG:  duration: 61.336 ms  plan:
>> Query Text: select count(*) from ft1;
>> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>>   Relations: Aggregate on (ft1)
>> 2025-11-14 14:40:35.825 CET [44862] LOG:  duration: 60.575 ms  plan:
>> Query Text: DECLARE c1 CURSOR FOR
>> SELECT count(*) FROM "S 1"."T 1"
>> Aggregate  (cost=21888.22..21888.23 rows=1 width=8)
>>   ->  Seq Scan on "T 1"  (cost=0.00..19956.98 rows=772498 width=0)
>>  count
>> --------
>>  990821
>> (1 row)
>>
>> Time: 62.728 ms
>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>> SET
>> Time: 1.515 ms
>> contrib_regression=# select count(*) from ft1;
>> 2025-11-14 14:40:46.260 CET [44862] LOG:  duration: 21.875 ms  plan:
>> Query Text: SELECT count(*) FROM "S 1"."T 1"
>> Finalize Aggregate  (cost=17255.64..17255.65 rows=1 width=8)
>>   ->  Gather  (cost=17255.43..17255.64 rows=2 width=8)
>>         Workers Planned: 2
>>         ->  Partial Aggregate  (cost=16255.43..16255.44 rows=1 width=8)
>>               ->  Parallel Seq Scan on "T 1"  (cost=0.00..15450.74
>> rows=321874 width=0)
>> 2025-11-14 14:40:46.260 CET [44338] LOG:  duration: 22.623 ms  plan:
>> Query Text: select count(*) from ft1;
>> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>>   Relations: Aggregate on (ft1)
>>  count
>> --------
>>  990821
>> (1 row)
>>
>> Time: 24.862 ms
>>
>> So for this query, the advantage is coming from parallel query which was
>> otherwise not possible in this scenario.
>> To study more performance with this patch, I found another interesting
>> query and here are the results,
>>
>> contrib_regression=# SET postgres_fdw.use_cursor = true;
>> SET
>> contrib_regression=# explain  (analyse, buffers)  SELECT t1."C 1" FROM "S
>> 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 =
>> t1."C 1");2025-11-14 15:57:46.893 CET [1946]
>>                                                               QUERY PLAN
>>
>>
>> ---------------------------------------------------------------------------------------------------------------------------------------
>>  Hash Left Join  (cost=109013.50..131877.35 rows=772498 width=4) (actual
>> time=112311.578..112804.516 rows=990821.00 loops=1)
>>    Hash Cond: (t1."C 1" = t3.c1)
>>    Buffers: shared hit=12232, temp read=12754 written=12754
>>    ->  Seq Scan on "T 1" t1  (cost=0.00..19956.98 rows=772498 width=4)
>> (actual time=0.039..48.808 rows=990821.00 loops=1)
>>          Buffers: shared hit=12232
>>    ->  Hash  (cost=109001.00..109001.00 rows=1000 width=4) (actual
>> time=112310.386..112310.387 rows=990821.00 loops=1)
>>          Buckets: 262144 (originally 1024)  Batches: 8 (originally 1)
>>  Memory Usage: 6408kB
>>          Buffers: temp written=2537
>>          ->  Nested Loop  (cost=200.43..109001.00 rows=1000 width=4)
>> (actual time=0.728..112030.241 rows=990821.00 loops=1)
>>                ->  Foreign Scan on ft1 t2  (cost=100.00..331.00 rows=1000
>> width=4) (actual time=0.398..710.505 rows=990821.00 loops=1)
>>                ->  Foreign Scan on ft2 t3  (cost=100.43..108.66 rows=1
>> width=4) (actual time=0.082..0.082 rows=1.00 loops=990821)
>>  Planning:
>>    Buffers: shared hit=5
>>  Planning Time: 2.211 ms
>>  Execution Time: 112825.428 ms
>> (15 rows)
>>
>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>> SET
>> explain  (analyse, buffers)  SELECT t1."C 1" FROM "S 1"."T 1" t1 left
>> join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1");
>> QUERY PLAN
>>
>> ----------------------------------------------------------------------------------------------------------------------------------
>>  Hash Left Join (cost=109013.50..131877.35 rows=772498 width=4) (actual
>> time=261.416..354.520 rows=990821.00 loops=1)
>>   Hash Cond: (t1."C 1" = t3.c1)
>>   Buffers: shared hit=12232, temp written=2660
>>   -> Seq Scan on "T 1" t1 (cost=0.00..19956.98 rows=772498 width=4)
>> (actual time=0.021..35.531 rows=990821.00 loops=1)
>>      Buffers: shared hit=12232
>>   -> Hash (cost=109001.00..109001.00 rows=1000 width=4) (actual
>> time=261.381..261.383 rows=100.00 loops=1)
>>      Buckets: 1024 Batches: 1 Memory Usage: 12kB
>>      Buffers: temp written=2660
>>      -> Nested Loop (cost=200.43..109001.00 rows=1000 width=4) (actual
>> time=255.563..261.356 rows=100.00 loops=1)
>>         Buffers: temp written=2660
>>         -> Foreign Scan on ft1 t2 (cost=100.00..331.00 rows=1000 width=4)
>> (actual time=0.433..0.443 rows=100.00 loops=1)
>>         -> Foreign Scan on ft2 t3 (cost=100.43..108.66 rows=1 width=4)
>> (actual time=2.609..2.609 rows=1.00 loops=100)
>>            Buffers: temp written=2660
>>  Planning:
>>   Buffers: shared hit=5
>>  Planning Time: 2.284 ms
>>  Execution Time: 389.358 ms
>> (17 rows)
>>
>> So even in the case without a parallel plan, it is performing
>> significantly better. I investigated a bit more to find out why the query
>> was so slow with the cursors,
>> and came to understand that it is repeatedly abandoning and recreating
>> the cursor via the code path of postgresReScanForeignScan.
>> So, it looks like cursor management itself costs this much more time.
>>
>> To understand the impact of the patch in case of tuples spilled to disk,
>> I tried following example,
>> contrib_regression=# SET postgres_fdw.use_cursor = true;
>> SET
>> contrib_regression=# set enable_hashjoin = off;
>> SET
>> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
>> where a.c1 = b.c1 order by a.c2;
>>                                                                QUERY PLAN
>>
>>
>> ----------------------------------------------------------------------------------------------------------------------------------------
>>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
>> time=4537.437..4598.483 rows=990821.00 loops=1)
>>    Sort Key: a.c2
>>    Sort Method: external merge  Disk: 137768kB
>>    Buffers: temp read=83156 written=83253
>>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
>> time=3748.488..4090.547 rows=990821.00 loops=1)
>>          Merge Cond: (a.c1 = b.c1)
>>          Buffers: temp read=48725 written=48779
>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>> time=1818.521..1865.792 rows=990821.00 loops=1)
>>                Sort Key: a.c1
>>                Sort Method: external merge  Disk: 75664kB
>>                Buffers: temp read=18910 written=18937
>>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
>> width=47) (actual time=1.302..1568.640 rows=990821.00 loops=1)
>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>> time=1929.955..1981.104 rows=990821.00 loops=1)
>>                Sort Key: b.c1
>>                Sort Method: external sort  Disk: 79520kB
>>                Buffers: temp read=29815 written=29842
>>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
>> width=47) (actual time=0.528..1553.249 rows=990821.00 loops=1)
>>  Planning Time: 0.479 ms
>>  Execution Time: 4661.872 ms
>> (19 rows)
>>
>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>> SET
>> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1 b
>> where a.c1 = b.c1 order by a.c2;
>>                                                               QUERY PLAN
>>
>>
>> ---------------------------------------------------------------------------------------------------------------------------------------
>>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
>> time=3376.385..3435.406 rows=990821.00 loops=1)
>>    Sort Key: a.c2
>>    Sort Method: external merge  Disk: 137768kB
>>    Buffers: temp read=83156 written=83253
>>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
>> time=2565.517..2916.814 rows=990821.00 loops=1)
>>          Merge Cond: (a.c1 = b.c1)
>>          Buffers: temp read=48725 written=48779
>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>> time=1249.517..1300.132 rows=990821.00 loops=1)
>>                Sort Key: a.c1
>>                Sort Method: external merge  Disk: 75664kB
>>                Buffers: temp read=18910 written=18937
>>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
>> width=47) (actual time=1.651..980.740 rows=990821.00 loops=1)
>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>> time=1315.990..1369.576 rows=990821.00 loops=1)
>>                Sort Key: b.c1
>>                Sort Method: external sort  Disk: 79520kB
>>                Buffers: temp read=29815 written=29842
>>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
>> width=47) (actual time=0.426..970.728 rows=990821.00 loops=1)
>>  Planning Time: 0.491 ms
>>  Execution Time: 3527.457 ms
>> (19 rows)
>>
>> So it doesn't hurt in this case either. But that is because not the
>> tuplestore which is added in this patch is spilling to disk here.
>> I am working on finding a case when there are two or more active cursors
>> and then when storing the tuples of one cursor,
>> they are spilled onto the disk. That might give a picture of the worst
>> case scenario of this patch.
>>
>> Also, thanks to Kenan, a fellow hacker who finds this work interesting
>> and offered to do some performance analysis for this patch,
>> maybe he can also post more results here.
>>
>> --
>> Regards,
>> Rafia Sabih
>> CYBERTEC PostgreSQL International GmbH
>>
>

-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

KENAN YILMAZ <kenan.yilmaz@localus.com.tr> — 2025-11-28T17:12:36Z

Hello Hackers,

I have executed another quick tests based on v4-0001-Fetch-without-cursors
patch.
I also recompiled PostgreSQL from the latest main branch.

Here are the execution time results;

** Local t1 table select (1st run) → 35049.790 ms — Parallel workers
launched
** postgres_fdw.use_cursor = true → 18996.236 ms — No parallel workers
launched
** postgres_fdw.use_cursor = false → 24962.529 ms — Parallel workers
launched
** Local t1 table select (2nd run) → 21406.631 ms — Parallel workers
launched

In my opinion, the primary goal is to enable FDW to fully support parallel
execution.
>From that perspective, these results seem acceptable.

If you would like to run more advanced or complex test queries, please feel
free to proceed.

The following tests were executed in the same manner as in my previous test
runs;

--
-- * Local t1 table EXPLAIN results
--

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1 WHERE a > 1000;
                                                         QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
time=35042.394..35043.542 rows=0.00 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   Buffers: shared hit=240 read=363397
   I/O Timings: shared read=99749.585
   ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1 width=105)
(actual time=35023.992..35023.993 rows=0.00 loops=3)
         Filter: (a > 1000)
         Rows Removed by Filter: 6666667
         Buffers: shared hit=240 read=363397
         I/O Timings: shared read=99749.585
 Planning:
   Buffers: shared hit=64 read=9 dirtied=1
   I/O Timings: shared read=4.656
 Planning Time: 30.145 ms
 Execution Time: 35049.790 ms
(15 rows)

Time: 35154.644 ms (00:35.155)

--
-- * use_cursor = true EXPLAIN results
--

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
                                                     QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
time=18981.860..18981.896 rows=0.00 loops=1)
 Planning:
   Buffers: shared hit=33 read=17 dirtied=1
   I/O Timings: shared read=27.012
 Planning Time: 65.290 ms
 Execution Time: 18996.236 ms
(6 rows)

Time: 19121.351 ms (00:19.121)

--> From Log Files
--> From Log Files
[197885]: line=11 sid=6929af4a.304fd tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
TRANSACTION ISOLATION LEVEL REPEATABLE READ
[197885]: line=12 sid=6929af4a.304fd tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[197885]: line=13 sid=6929af4a.304fd tag=DECLARE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: execute
<unnamed>: DECLARE c1 CURSOR FOR
        SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
[197885]: line=14 sid=6929af4a.304fd tag=DECLARE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
 exec_execute_message, postgres.c:2245
[197885]: line=15 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: FETCH
100 FROM c1
[197885]: line=16 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[197882]: line=5 sid=6929af48.304fa tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOG:  00000: duration: 18981.899 ms  plan:
        Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
WHERE a > 1000;
        Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
(actual time=18981.860..18981.896 rows=0.00 loops=1)
[197882]: line=6 sid=6929af48.304fa tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
[197885]: line=17 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: CLOSE
c1
[197885]: line=18 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[197885]: line=19 sid=6929af4a.304fd tag=CLOSE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: duration:
18950.616 ms  plan:
        Query Text: DECLARE c1 CURSOR FOR
        SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
        Seq Scan on t1  (cost=0.00..613637.45 rows=1 width=105) (actual
time=18950.614..18950.614 rows=0.00 loops=1)
          Filter: (a > 1000)
          Rows Removed by Filter: 20000000
[197885]: line=20 sid=6929af4a.304fd tag=CLOSE CURSOR usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
 explain_ExecutorEnd, auto_explain.c:437
[197885]: line=21 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: COMMIT
TRANSACTION
[197885]: line=22 sid=6929af4a.304fd tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078

--
-- * use_cursor = false EXPLAIN results
--

testdb=# SET postgres_fdw.use_cursor = false;

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a > 1000;
                                                     QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
time=24940.034..24940.054 rows=0.00 loops=1)
 Planning:
   Buffers: shared hit=33 read=17
   I/O Timings: shared read=34.719
 Planning Time: 62.300 ms
 Execution Time: 24962.529 ms
(6 rows)

Time: 25124.566 ms (00:25.125)

--> From Log Files
[197919]: line=11 sid=6929afcb.3051f tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
TRANSACTION ISOLATION LEVEL REPEATABLE READ
[197919]: line=12 sid=6929afcb.3051f tag=idle usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[197919]: line=13 sid=6929afcb.3051f tag=SELECT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: execute <unnamed>: SELECT
a, b, c FROM public.t1 WHERE ((a > 1000))
[197919]: line=14 sid=6929afcb.3051f tag=SELECT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  exec_execute_message,
postgres.c:2245
[197917]: line=7 sid=6929afbd.3051d tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOG:  00000: duration: 24940.057 ms  plan:
        Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
WHERE a > 1000;
        Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
(actual time=24940.034..24940.054 rows=0.00 loops=1)
[197917]: line=8 sid=6929afbd.3051d tag=EXPLAIN usr=postgres db=testdb
app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
[197919]: line=15 sid=6929afcb.3051f tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: COMMIT
TRANSACTION
[197919]: line=16 sid=6929afcb.3051f tag=idle in transaction usr=postgres
db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
postgres.c:1078
[197919]: line=17 sid=6929afcb.3051f tag=COMMIT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOG:  00000: duration: 24908.608 ms
 plan:
        Query Text: SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
        Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
time=24906.370..24908.603 rows=0.00 loops=1)
          Workers Planned: 2
          Workers Launched: 2
          ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1
width=105) (actual time=24895.314..24895.314 rows=0.00 loops=3)
                Filter: (a > 1000)
                Rows Removed by Filter: 6666667
[197919]: line=18 sid=6929afcb.3051f tag=COMMIT usr=postgres db=testdb
app=postgres_fdw client=127.0.0.1: LOCATION:  explain_ExecutorEnd,
auto_explain.c:437

---
--
-- * Local t1 table EXPLAIN results (again)
--

testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1 WHERE a > 1000;
                                                         QUERY PLAN
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Gather  (cost=0.00..467803.67 rows=1 width=105) (actual
time=21403.492..21405.381 rows=0.00 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   Buffers: shared read=363637
   I/O Timings: shared read=59425.134
   ->  Parallel Seq Scan on t1  (cost=0.00..467803.67 rows=1 width=105)
(actual time=21394.997..21394.997 rows=0.00 loops=3)
         Filter: (a > 1000)
         Rows Removed by Filter: 6666667
         Buffers: shared read=363637
         I/O Timings: shared read=59425.134
 Planning:
   Buffers: shared hit=54 read=14
   I/O Timings: shared read=30.482
 Planning Time: 37.955 ms
 Execution Time: 21406.631 ms
(15 rows)

Time: 21506.079 ms (00:21.506)

---
Kenan YILMAZ


Rafia Sabih <rafia.pghackers@gmail.com>, 27 Kas 2025 Per, 13:50 tarihinde
şunu yazdı:

> On Tue, 25 Nov 2025 at 15:24, KENAN YILMAZ <kenan.yilmaz@localus.com.tr>
> wrote:
>
>> Hello Hackers,
>>
>> I have executed use_cursor = true/false with quick tests.
>>
>> The test table 't1' has 20,000,000 rows (~2.8 GB in size).
>>
>> The test query is `EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
>> WHERE a > 1000;`
>>
>> The result sums are;
>>
>> ** `postgres_fdw.use_cursor = true` --> 20090.782 ms
>> ** `postgres_fdw.use_cursor = false` --> 24103.994 ms
>>
>> If you wish to run more advanced and complex test queries, feel free to
>> do so.
>>
>> All execution test scenarios are listed below;
>>
>> --
>> -- restart with flush caches;
>> $ pg_ctl -D $PGDATA stop && sudo sync && echo 3 | sudo tee
>> /proc/sys/vm/drop_caches && rm $PGDATA/log/*.log && pg_ctl -D $PGDATA start
>> -- Data prep stage
>> DROP DATABASE IF EXISTS testdb;
>> CREATE DATABASE testdb;
>> \c testdb;
>> DROP TABLE IF EXISTS t1 CASCADE;
>> CREATE UNLOGGED TABLE t1 (a int, b int, c text, d timestamp);
>>
>> -- Insert test datas
>> INSERT INTO t1 SELECT 10 + mod(i, 30), i, md5(i::text) ||
>> md5((i+1000000)::text) || md5((i+2000000)::text), '2025-01-01
>> 10:00:00'::timestamp + (random() * 31536000) * INTERVAL '1 second' FROM
>> generate_series(1, 20000000) i;
>>
>> -- Table maintenance
>> ALTER TABLE t1 SET LOGGED;
>> VACUUM (ANALYZE, FULL) t1;
>>
>> -- FDW prep stage
>> DROP EXTENSION IF EXISTS postgres_fdw CASCADE;
>> CREATE EXTENSION postgres_fdw;
>> CREATE SERVER foreign_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
>> (host '127.0.0.1', port '5432', dbname 'testdb');
>> CREATE USER MAPPING FOR postgres SERVER foreign_server OPTIONS (user
>> 'postgres');
>> CREATE FOREIGN TABLE t1fdw (a int, b int, c text, d timestamp) SERVER
>> foreign_server OPTIONS (table_name 't1');
>>
>> -- restart with flush caches are applied before all stage executions.
>>
>> --
>> -- * Local t1 table EXPLAIN results
>> --
>>
>> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1 WHERE a > 1000;
>>                                                          QUERY PLAN
>>
>> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>>  Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
>> time=23260.306..23261.591 rows=0.00 loops=1)
>>    Workers Planned: 2
>>    Workers Launched: 2
>>    Buffers: shared read=363637
>>    I/O Timings: shared read=64590.910
>>    ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1 width=105)
>> (actual time=23242.279..23242.280 rows=0.00 loops=3)
>>          Filter: (a > 1000)
>>          Rows Removed by Filter: 6666667
>>          Buffers: shared read=363637
>>          I/O Timings: shared read=64590.910
>>  Planning:
>>    Buffers: shared hit=54 read=14 dirtied=1
>>    I/O Timings: shared read=23.281
>>  Planning Time: 38.734 ms
>>  Execution Time: 23269.677 ms
>> (15 rows)
>>
>> Time: 23347.716 ms (00:23.348)
>>
>> --
>> -- * use_cursor = true (Default) EXPLAIN results
>> --
>>
>> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a >
>> 1000;
>>                                                      QUERY PLAN
>>
>> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>>  Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
>> time=20074.746..20074.796 rows=0.00 loops=1)
>>  Planning:
>>    Buffers: shared hit=33 read=17 dirtied=1
>>    I/O Timings: shared read=10.696
>>  Planning Time: 43.852 ms
>>  Execution Time: 20090.782 ms
>> (6 rows)
>>
>> Time: 20169.081 ms (00:20.169)
>>
>> --> From Log Files
>> [153045]: line=11 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
>> TRANSACTION ISOLATION LEVEL REPEATABLE READ
>> [153045]: line=12 sid=6923fba6.255d5 tag=idle usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
>> postgres.c:1078
>> [153045]: line=13 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: execute
>> <unnamed>: DECLARE c1 CURSOR FOR
>>         SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>> [153045]: line=14 sid=6923fba6.255d5 tag=DECLARE CURSOR usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>>  exec_execute_message, postgres.c:2245
>> [153045]: line=15 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
>> FETCH 100 FROM c1
>> [153045]: line=16 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>>  exec_simple_query, postgres.c:1078
>> [153044]: line=5 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
>> app=psql client=[local]: LOG:  00000: duration: 20074.799 ms  plan:
>>         Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
>> WHERE a > 1000;
>>         Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
>> (actual time=20074.746..20074.796 rows=0.00 loops=1)
>> [153044]: line=6 sid=6923fba4.255d4 tag=EXPLAIN usr=postgres db=testdb
>> app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
>> [153045]: line=17 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
>> CLOSE c1
>> [153045]: line=18 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>>  exec_simple_query, postgres.c:1078
>> [153045]: line=19 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: duration:
>> 20057.543 ms  plan:
>>         Query Text: DECLARE c1 CURSOR FOR
>>         SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>>         Seq Scan on t1  (cost=0.00..613637.45 rows=1 width=105) (actual
>> time=20057.541..20057.541 rows=0.00 loops=1)
>>           Filter: (a > 1000)
>>           Rows Removed by Filter: 20000000
>> [153045]: line=20 sid=6923fba6.255d5 tag=CLOSE CURSOR usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>>  explain_ExecutorEnd, auto_explain.c:437
>> [153045]: line=21 sid=6923fba6.255d5 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
>> COMMIT TRANSACTION
>>
>> --
>> -- * use_cursor = false EXPLAIN results
>> --
>>
>> testdb=# SET postgres_fdw.use_cursor = false;
>>
>> testdb=# EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw WHERE a >
>> 1000;
>>                                                      QUERY PLAN
>>
>> ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
>>  Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40) (actual
>> time=24080.945..24080.956 rows=0.00 loops=1)
>>  Planning:
>>    Buffers: shared hit=33 read=17
>>    I/O Timings: shared read=30.074
>>  Planning Time: 53.678 ms
>>  Execution Time: 24103.994 ms
>> (6 rows)
>>
>> Time: 24230.548 ms (00:24.231)
>>
>> --> From Log Files
>> [153121]: line=11 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOG:  00000: statement: START
>> TRANSACTION ISOLATION LEVEL REPEATABLE READ
>> [153121]: line=12 sid=6923fc16.25621 tag=idle usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_simple_query,
>> postgres.c:1078
>> [153121]: line=13 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOG:  00000: execute <unnamed>:
>> SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>> [153121]: line=14 sid=6923fc16.25621 tag=SELECT usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOCATION:  exec_execute_message,
>> postgres.c:2245
>> [153113]: line=7 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
>> app=psql client=[local]: LOG:  00000: duration: 24080.958 ms  plan:
>>         Query Text: EXPLAIN (ANALYZE, BUFFERS) SELECT a,b,c FROM t1fdw
>> WHERE a > 1000;
>>         Foreign Scan on t1fdw  (cost=100.00..215.67 rows=427 width=40)
>> (actual time=24080.945..24080.956 rows=0.00 loops=1)
>> [153113]: line=8 sid=6923fc0c.25619 tag=EXPLAIN usr=postgres db=testdb
>> app=psql client=[local]: LOCATION:  explain_ExecutorEnd, auto_explain.c:437
>> [153121]: line=15 sid=6923fc16.25621 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOG:  00000: statement:
>> COMMIT TRANSACTION
>> [153121]: line=16 sid=6923fc16.25621 tag=idle in transaction usr=postgres
>> db=testdb app=postgres_fdw client=127.0.0.1: LOCATION:
>>  exec_simple_query, postgres.c:1078
>> [153121]: line=17 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOG:  00000: duration: 24059.372 ms
>>  plan:
>>         Query Text: SELECT a, b, c FROM public.t1 WHERE ((a > 1000))
>>         Gather  (cost=0.00..467803.85 rows=1 width=105) (actual
>> time=24058.076..24059.367 rows=0.00 loops=1)
>>           Workers Planned: 2
>>           Workers Launched: 2
>>           ->  Parallel Seq Scan on t1  (cost=0.00..467803.85 rows=1
>> width=105) (actual time=24051.406..24051.407 rows=0.00 loops=3)
>>                 Filter: (a > 1000)
>>                 Rows Removed by Filter: 6666667
>> [153121]: line=18 sid=6923fc16.25621 tag=COMMIT usr=postgres db=testdb
>> app=postgres_fdw client=127.0.0.1: LOCATION:  explain_ExecutorEnd,
>> auto_explain.c:437
>>
>> ---
>>
>> Kenan YILMAZ
>>
> Thanks Kenan for these. So, it looks like the patch performs the same as
> in the local scan case. I wonder if you found any case of performance
> degradation with the patch.
>
> Per an off-list discussion with Robert, he suggested using the
> existing data structures for recording the state of last queries instead of
> inventing something new.
> Makes sense, so I reworked the patch to include tuplestore in
> PgFdwScanState and then use PgFdwScanState as part of PgFdwConnState to
> keep track of previously
> active cursors. Nothing else is changed in this version of the patch.
>
>>
>>
>>
>> Rafia Sabih <rafia.pghackers@gmail.com>, 25 Kas 2025 Sal, 17:01
>> tarihinde şunu yazdı:
>>
>>>
>>>
>>> On Fri, 10 Oct 2025 at 22:02, Robert Haas <robertmhaas@gmail.com> wrote:
>>> >
>>> > On Mon, Sep 29, 2025 at 10:51 AM Rafia Sabih <
>>> rafia.pghackers@gmail.com> wrote:
>>> > > I am back at this work with a rebased and revised patch. The new
>>> version is rebased and has a change in approach.
>>> > > Whenever we are using non-cursor mode, for the first cursor we are
>>> always saving the tuples
>>> > > in the tuplestore, this is because we do not have any means to know
>>> beforehand how many cursors are required for the query.
>>> >
>>> > This might have the advantage of being simpler, but it's definitely
>>> > worse. If we're only fetching one result set, which will be common,
>>> > we'll buffer the whole thing in a tuplestore where that could
>>> > otherwise be avoided. Maybe it's still best to go with this approach;
>>> > not sure.
>>> >
>>> > > And when we switch to the next query then we do not have a way to
>>> fetch the tuples for the previous query.
>>> > > So, the tuples retrieved earlier for the first query were lost if
>>> not saved.
>>> > > I would highly appreciate your time and feedback for this.
>>> >
>>> > My suggestions are to work on the following areas:
>>> >
>>> > 1. Automated testing. The patch has no regression tests, and won't get
>>> > committed without those.
>>> >
>>> > 2. Manual testing. How does the performance with this new option
>>> > compare to the existing method? The answer might be different for
>>> > small result sets that fit in memory and large ones that spill to
>>> > disk, and will certainly also depend on how productive parallel query
>>> > can be on the remote side; but I think you want to do and post on this
>>> > list some measurements showing the best and worst case for the patch.
>>> >
>>> > 3. Patch clean-up. There are plenty of typos and whitespace-only
>>> > changes in the patch. It's best to clean those up. Running pgindent is
>>> > a good idea too. Some places could use comments.
>>> >
>>> > --
>>> > Robert Haas
>>> > EDB: http://www.enterprisedb.com
>>>
>>> Thank you Robert, for reviewing this patch. On going through the patch
>>> more, I realised this was not equipped to handle the cases when there are
>>> more than two active cursors. So to accommodate such a case, I now modified
>>> the new struct for saving the previous query to a list of such structs.
>>> Also, it turns out we need not to save the tuples in case this is an active
>>> cursor, so we only populate the associated tuplestore only when we need to
>>> create a new cursor when the old cursor is not completely done.
>>>
>>> In this version I have added the regression tests as well. I wanted to
>>> test this patch for all the cases of postgres_fdw, the only way I could
>>> figure out how to do this was to test the select statements with the new
>>> GUC.
>>>
>>> I also did some tests for performance. I used the contrib_regression
>>> database and populated the table "S1"."T1" with more tuples to understand
>>> the impact of patch on higher scale. I also used auto_explain to get the
>>> foreign plans.
>>>
>>> contrib_regression=# select count(*) from ft1;
>>> 2025-11-14 14:40:35.825 CET [44338] LOG:  duration: 61.336 ms  plan:
>>> Query Text: select count(*) from ft1;
>>> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>>>   Relations: Aggregate on (ft1)
>>> 2025-11-14 14:40:35.825 CET [44862] LOG:  duration: 60.575 ms  plan:
>>> Query Text: DECLARE c1 CURSOR FOR
>>> SELECT count(*) FROM "S 1"."T 1"
>>> Aggregate  (cost=21888.22..21888.23 rows=1 width=8)
>>>   ->  Seq Scan on "T 1"  (cost=0.00..19956.98 rows=772498 width=0)
>>>  count
>>> --------
>>>  990821
>>> (1 row)
>>>
>>> Time: 62.728 ms
>>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>>> SET
>>> Time: 1.515 ms
>>> contrib_regression=# select count(*) from ft1;
>>> 2025-11-14 14:40:46.260 CET [44862] LOG:  duration: 21.875 ms  plan:
>>> Query Text: SELECT count(*) FROM "S 1"."T 1"
>>> Finalize Aggregate  (cost=17255.64..17255.65 rows=1 width=8)
>>>   ->  Gather  (cost=17255.43..17255.64 rows=2 width=8)
>>>         Workers Planned: 2
>>>         ->  Partial Aggregate  (cost=16255.43..16255.44 rows=1 width=8)
>>>               ->  Parallel Seq Scan on "T 1"  (cost=0.00..15450.74
>>> rows=321874 width=0)
>>> 2025-11-14 14:40:46.260 CET [44338] LOG:  duration: 22.623 ms  plan:
>>> Query Text: select count(*) from ft1;
>>> Foreign Scan  (cost=102.50..123.72 rows=1 width=8)
>>>   Relations: Aggregate on (ft1)
>>>  count
>>> --------
>>>  990821
>>> (1 row)
>>>
>>> Time: 24.862 ms
>>>
>>> So for this query, the advantage is coming from parallel query which was
>>> otherwise not possible in this scenario.
>>> To study more performance with this patch, I found another interesting
>>> query and here are the results,
>>>
>>> contrib_regression=# SET postgres_fdw.use_cursor = true;
>>> SET
>>> contrib_regression=# explain  (analyse, buffers)  SELECT t1."C 1" FROM
>>> "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 =
>>> t1."C 1");2025-11-14 15:57:46.893 CET [1946]
>>>                                                               QUERY PLAN
>>>
>>>
>>> ---------------------------------------------------------------------------------------------------------------------------------------
>>>  Hash Left Join  (cost=109013.50..131877.35 rows=772498 width=4) (actual
>>> time=112311.578..112804.516 rows=990821.00 loops=1)
>>>    Hash Cond: (t1."C 1" = t3.c1)
>>>    Buffers: shared hit=12232, temp read=12754 written=12754
>>>    ->  Seq Scan on "T 1" t1  (cost=0.00..19956.98 rows=772498 width=4)
>>> (actual time=0.039..48.808 rows=990821.00 loops=1)
>>>          Buffers: shared hit=12232
>>>    ->  Hash  (cost=109001.00..109001.00 rows=1000 width=4) (actual
>>> time=112310.386..112310.387 rows=990821.00 loops=1)
>>>          Buckets: 262144 (originally 1024)  Batches: 8 (originally 1)
>>>  Memory Usage: 6408kB
>>>          Buffers: temp written=2537
>>>          ->  Nested Loop  (cost=200.43..109001.00 rows=1000 width=4)
>>> (actual time=0.728..112030.241 rows=990821.00 loops=1)
>>>                ->  Foreign Scan on ft1 t2  (cost=100.00..331.00
>>> rows=1000 width=4) (actual time=0.398..710.505 rows=990821.00 loops=1)
>>>                ->  Foreign Scan on ft2 t3  (cost=100.43..108.66 rows=1
>>> width=4) (actual time=0.082..0.082 rows=1.00 loops=990821)
>>>  Planning:
>>>    Buffers: shared hit=5
>>>  Planning Time: 2.211 ms
>>>  Execution Time: 112825.428 ms
>>> (15 rows)
>>>
>>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>>> SET
>>> explain  (analyse, buffers)  SELECT t1."C 1" FROM "S 1"."T 1" t1 left
>>> join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1");
>>> QUERY PLAN
>>>
>>> ----------------------------------------------------------------------------------------------------------------------------------
>>>  Hash Left Join (cost=109013.50..131877.35 rows=772498 width=4) (actual
>>> time=261.416..354.520 rows=990821.00 loops=1)
>>>   Hash Cond: (t1."C 1" = t3.c1)
>>>   Buffers: shared hit=12232, temp written=2660
>>>   -> Seq Scan on "T 1" t1 (cost=0.00..19956.98 rows=772498 width=4)
>>> (actual time=0.021..35.531 rows=990821.00 loops=1)
>>>      Buffers: shared hit=12232
>>>   -> Hash (cost=109001.00..109001.00 rows=1000 width=4) (actual
>>> time=261.381..261.383 rows=100.00 loops=1)
>>>      Buckets: 1024 Batches: 1 Memory Usage: 12kB
>>>      Buffers: temp written=2660
>>>      -> Nested Loop (cost=200.43..109001.00 rows=1000 width=4) (actual
>>> time=255.563..261.356 rows=100.00 loops=1)
>>>         Buffers: temp written=2660
>>>         -> Foreign Scan on ft1 t2 (cost=100.00..331.00 rows=1000
>>> width=4) (actual time=0.433..0.443 rows=100.00 loops=1)
>>>         -> Foreign Scan on ft2 t3 (cost=100.43..108.66 rows=1 width=4)
>>> (actual time=2.609..2.609 rows=1.00 loops=100)
>>>            Buffers: temp written=2660
>>>  Planning:
>>>   Buffers: shared hit=5
>>>  Planning Time: 2.284 ms
>>>  Execution Time: 389.358 ms
>>> (17 rows)
>>>
>>> So even in the case without a parallel plan, it is performing
>>> significantly better. I investigated a bit more to find out why the query
>>> was so slow with the cursors,
>>> and came to understand that it is repeatedly abandoning and recreating
>>> the cursor via the code path of postgresReScanForeignScan.
>>> So, it looks like cursor management itself costs this much more time.
>>>
>>> To understand the impact of the patch in case of tuples spilled to disk,
>>> I tried following example,
>>> contrib_regression=# SET postgres_fdw.use_cursor = true;
>>> SET
>>> contrib_regression=# set enable_hashjoin = off;
>>> SET
>>> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1
>>> b where a.c1 = b.c1 order by a.c2;
>>>                                                                QUERY
>>> PLAN
>>>
>>> ----------------------------------------------------------------------------------------------------------------------------------------
>>>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
>>> time=4537.437..4598.483 rows=990821.00 loops=1)
>>>    Sort Key: a.c2
>>>    Sort Method: external merge  Disk: 137768kB
>>>    Buffers: temp read=83156 written=83253
>>>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
>>> time=3748.488..4090.547 rows=990821.00 loops=1)
>>>          Merge Cond: (a.c1 = b.c1)
>>>          Buffers: temp read=48725 written=48779
>>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>>> time=1818.521..1865.792 rows=990821.00 loops=1)
>>>                Sort Key: a.c1
>>>                Sort Method: external merge  Disk: 75664kB
>>>                Buffers: temp read=18910 written=18937
>>>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
>>> width=47) (actual time=1.302..1568.640 rows=990821.00 loops=1)
>>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>>> time=1929.955..1981.104 rows=990821.00 loops=1)
>>>                Sort Key: b.c1
>>>                Sort Method: external sort  Disk: 79520kB
>>>                Buffers: temp read=29815 written=29842
>>>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
>>> width=47) (actual time=0.528..1553.249 rows=990821.00 loops=1)
>>>  Planning Time: 0.479 ms
>>>  Execution Time: 4661.872 ms
>>> (19 rows)
>>>
>>> contrib_regression=# SET postgres_fdw.use_cursor = false;
>>> SET
>>> contrib_regression=# explain (analyse, buffers)select * from ft1 a, ft1
>>> b where a.c1 = b.c1 order by a.c2;
>>>                                                               QUERY PLAN
>>>
>>>
>>> ---------------------------------------------------------------------------------------------------------------------------------------
>>>  Sort  (cost=831.49..833.99 rows=1000 width=94) (actual
>>> time=3376.385..3435.406 rows=990821.00 loops=1)
>>>    Sort Key: a.c2
>>>    Sort Method: external merge  Disk: 137768kB
>>>    Buffers: temp read=83156 written=83253
>>>    ->  Merge Join  (cost=761.66..781.66 rows=1000 width=94) (actual
>>> time=2565.517..2916.814 rows=990821.00 loops=1)
>>>          Merge Cond: (a.c1 = b.c1)
>>>          Buffers: temp read=48725 written=48779
>>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>>> time=1249.517..1300.132 rows=990821.00 loops=1)
>>>                Sort Key: a.c1
>>>                Sort Method: external merge  Disk: 75664kB
>>>                Buffers: temp read=18910 written=18937
>>>                ->  Foreign Scan on ft1 a  (cost=100.00..331.00 rows=1000
>>> width=47) (actual time=1.651..980.740 rows=990821.00 loops=1)
>>>          ->  Sort  (cost=380.83..383.33 rows=1000 width=47) (actual
>>> time=1315.990..1369.576 rows=990821.00 loops=1)
>>>                Sort Key: b.c1
>>>                Sort Method: external sort  Disk: 79520kB
>>>                Buffers: temp read=29815 written=29842
>>>                ->  Foreign Scan on ft1 b  (cost=100.00..331.00 rows=1000
>>> width=47) (actual time=0.426..970.728 rows=990821.00 loops=1)
>>>  Planning Time: 0.491 ms
>>>  Execution Time: 3527.457 ms
>>> (19 rows)
>>>
>>> So it doesn't hurt in this case either. But that is because not the
>>> tuplestore which is added in this patch is spilling to disk here.
>>> I am working on finding a case when there are two or more active cursors
>>> and then when storing the tuples of one cursor,
>>> they are spilled onto the disk. That might give a picture of the worst
>>> case scenario of this patch.
>>>
>>> Also, thanks to Kenan, a fellow hacker who finds this work interesting
>>> and offered to do some performance analysis for this patch,
>>> maybe he can also post more results here.
>>>
>>> --
>>> Regards,
>>> Rafia Sabih
>>> CYBERTEC PostgreSQL International GmbH
>>>
>>
>
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2025-12-10T17:43:48Z

On Thu, Nov 27, 2025 at 5:50 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Thanks Kenan for these. So, it looks like the patch performs the same as in the local scan case. I wonder if you found any case of performance degradation with the patch.
>
> Per an off-list discussion with Robert, he suggested using the existing data structures for recording the state of last queries instead of inventing something new.
> Makes sense, so I reworked the patch to include tuplestore in PgFdwScanState and then use PgFdwScanState as part of PgFdwConnState to keep track of previously
> active cursors. Nothing else is changed in this version of the patch.

Thanks for your continued work on this topic.

As we discussed off-list, the regression tests in this patch need
quite a bit more work. We shouldn't just repeat large numbers of tests
-- we should repeat enough to provide meaningful test coverage of the
new feature, and maybe add a few new ones that specifically target
scenarios that the patch is intended to cover. One somewhat bizarre
thing that I noticed scrolling through the regression test outputs is
this:

+-- Test with non-cursor mode
+SET postgres_fdw.use_cursor = false;
+SET postgres_fdw.use_cursor = true;

This happens in multiple places in the regression tests, and doesn't
really make any sense to me, because changing the GUC from to false,
doing nothing, and then changing it back to true doesn't seem like a
useful test scenario, and definitely doesn't seem like a test scenario
that we need to repeat multiple times. Honestly, I wonder how this
happened. Did you maybe run a script over the .sql files to generate
the new versions and then not check what the output actually looked
like? I really can't stress enough the need to be thoughtful about
constructing test cases for a patch of this type.

A related problem is that you've included 12,721 lines of useless
output in the patch file, because somehow postgres_fdw.out.orig got
checked into the repository. It's always a good idea, when posting a
patch to the list, to check the diffstat to make sure that there's
nothing in there that you don't expect to see. The presence of this
line just below your commit message could have alerted you to this
problem:

.../expected/postgres_fdw.out.orig | 12721 ++++++++++++++++

Ideally, I really recommend scrolling through the patch, not just the
diffstat, to make sure that everything is the way you want it to be,
before giving it to others to look at.

There are a number of other cosmetic problems with this patch that
make it hard to review the actual code changes. For instance:

+ /* To be used only in non-cursor mode */
+ Tuplestorestate *tuplestore;
+ TupleTableSlot *slot;
+ bool tuples_ready;
+ struct PgFdwScanState *next;

The comment is good, but it only explains a general fact about all
four of these fields. It doesn't explain specifically what each of
these fields is intended to do. Naming a field very generally, like
"next", when it must have some very specific purpose that is not
documented, makes it really hard for someone reading the code -- in
this case, me -- to understand what the point is. So, these fields
should probably have comments, but also, some of them probably need to
be renamed. Maybe "tuplestore" and "slot" are OK but "next" is not
going to be good enough.

+/* Fill the Tupleslot when another query needs to execute. */
+static void
+fillTupleSlot(PgFdwScanState *fsstate, ForeignScanState *node)

I think you're filling a tuple store, not a tuple slot.

+ initStringInfo(&buf);

What seems to happen here is that you create an empty buffer, add
fsstate->query to it and nothing else, and then use buf.data. So why
not just get rid of buf and use fsstate->query directly?

+ /* Always fetch the last prev_query */
+ for (; p1->next != NULL; p1 = p1->next);

There are multiple problems here. First, this code is not per
PostgreSQL style and would be reindented by pgindent, which you should
make a habit of running before posting. Second, p1 is not a
particularly informative or understandable variable name. Third, why
are we using a linked list if the value we're going to need is at the
end of the list? If we need to be able to access the last element of
the list efficiently, maybe we should be keeping the list in reverse
order, or maybe we should be using a List which permits efficient
random access.

But looking quickly through the patch, I have an even bigger question
here. It doesn't really seem like we ever care about any element of
the list other than the last. It looks like we go to a fair amount of
trouble to maintain the list at various points in the code, but it
seems like when we access the list, we just always go to the end. That
makes me wonder why we even need a list. Perhaps instead of
PgFdwConnState storing a pointer to "prev_queries", it should just
store a pointer to the PgFdwScanState for the query that is currently
"using" the connection, if there is one. That is, the query whose
result set is currently being received, and which must be buffered
into a tuplestore before using the connection for something else. When
we've done that, we reset the field to NULL, and we don't maintain any
list of the older "previous queries". Maybe there's some problem with
that idea, but that gets back to my earlier point about comments: the
comment for "next" (or however it got renamed) ought to be explaining
something about why it exists and what it's for. Without such a
comment, the only ways for me to be sure whether we really need "next"
is to either (a) ask you or (b) spend a long time staring at the code
to try to figure it out or (c) try removing it and see if it works.

Ah, wait! I just found some code that cares about the list but not
just about the last element:

+ for (; p1->next != NULL; p1 = p1->next)
+ /* find the correct prev_query */
+ {
+ if ((p1->tuples_ready && fsstate->cursor_number == p1->cursor_number))
+ cur_prev = p1;
+ }

But I'm still confused. Why do we need to iterate through the
prev_queries list to find the correct ForeignScanState? Isn't that
exactly what fsstate is here? I'm inclined to think that this is just
a complicated way of setting p1 = fsstate and then setting cur_prev =
p1, so we could skip all this and just test whether
fsstate->tuplestore != NULL and if so retrieve tuples from there. If
there's some reason why fsstate and cur_prev would end up being
different, then there should be some comment explaining it. I'm
inclined to think that would be a sign of a design flaw: I think the
idea here should be to make use of the ForeignScanState object that
already exists to hold the details that we need for this feature,
rather than creating a new one. But, if there were a comment telling
me why it's like this, I might change my mind.

+ /* Clear the last query details, once tuples are retrieved. */
+ if (fsstate->conn_state->prev_queries == cur_prev)
+ {
+ /*
+ * This is the first prev query in the list, check if there
+ * are more
+ */
+ if (fsstate->conn_state->num_prev_queries > 1)
+
+ /*
+ * fsstate->conn_state->prev_queries->next =
+ * cur_prev->next;
+ */
+ fsstate->next = cur_prev->next;
+ clear_prev_query_state(cur_prev);
+ }

This code can remove an item from the prev_queries list, but only if
it's the first item. Now, if my analysis above is correct and we don't
even need the prev_queries list, then it doesn't matter; all this
logic can basically go away. If we do need the prev_queries list, then
I don't think it can be right to only be able to remove the first
element of the list. There's no reason why the oldest prev_query has
to finish first. What if there are three queries in the list and the
middle one finishes first? Then this will just leave it in the list,
whereas if the first one had finished first, it would have been
deleted from the list. That kind of inconsistency doesn't seem like a
good idea.

+ /*
+ * This is to fetch all the tuples of this query and save
+ * them in Tuple Slot. Since it is using
+ * PQSetChunkedRowsMode, we only get the
+ * fsstate->fetch_size tuples in one run, so keep on
+ * executing till we get NULL in PGresult i.e. all the
+ * tuples are retrieved.
+ */
+ p1->tuplestore = tuplestore_begin_heap(false, true, work_mem);
+ p1->slot = MakeSingleTupleTableSlot(fsstate->tupdesc, &TTSOpsMinimalTuple);

The slot doesn't seem to be used anywhere in the code that follows. Is
there a reason to initialize it here, rather than wherever it's
needed? If so, maybe the comment could mention that.

+ i = 0;

I don't think this does anything, because you reinitialize i in the
inner loop where you use it. In general, try moving your variable
declarations to the innermost scope where they are needed. It makes
the code more clear and allows the compiler to tell you about stuff
like this.

+ else if (PQresultStatus(res) == PGRES_TUPLES_OK)
+ {
+ while (res != NULL)
+ res = pgfdw_get_result(conn);
+ break;
+ }

I doubt that that this is right. It seems like it's willing to throw
away an infinite number of result sets without doing anything with
them, or discard an infinite number of errors without processing them,
but that doesn't seem likely to be the right thing.

+ else if (PQresultStatus(res) == PGRES_FATAL_ERROR)
+ {
+ clear_conn_state(fsstate->conn_state);
+ pgfdw_report_error(res, conn, fsstate->query);
+ }

I also doubt that this is right. If it's necessary to call
clear_conn_state() before reporting this error, then the error
handling in this patch is probably wrong. We don't know where errors
will be thrown in general and cannot rely on being able to do manual
error cleanup before an error occurs. The goal should for it to be
safe for pgfdw_report_error -- or just ereport(ERROR, ...) -- to
happen anywhere without causing problems in the future.

clear_conn_state() doesn't look right either. It does some cleanup on
each element of the prev_queries list and decrements num_prev_queries,
but it doesn't actually remove the queries from the list. So after
calling this function the first time, I think that num_prev_queries
might end up being 0 while prev_queries is still a list of the same
length as before. After that, I imagine calling clear_conn_state() a
second time would result in num_prev_queries going negative. I don't
really understand why num_prev_queries exists -- it seems like it's
just a recipe for mistakes to have both the list itself, and a
separate field that gives you the list length. If you need that, the
existing List data type will do that bookkeeping for you, and then
there's less opportunity for mistakes.

Overall, I think the direction of the patch set has some promise, but
I think it needs a lot of cleanup: removal of unnecessary code, proper
formatting, moving variables to inner scopes, explanatory comments,
good names for variables and functions and structure members, removal
of unnecessary files from the patch, cleanup of the regression test
coverage so that it doesn't add more bloat than necessary, proper
choice of data structures, and so on. Right now, the good things that
you've done here are being hidden by these sorts of mechanical issues.
That's not just an issue for me as a reviewer: I suspect it's also
blocking you, as the patch author, from finding places where the code
could be made better. Being able to find such opportunities for
improvement and act on them is what will get this patch from
"interesting proof of concept" to "potentially committable patch".

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Alexander Pyhalov <a.pyhalov@postgrespro.ru> — 2025-12-17T14:09:28Z

Rafia Sabih писал(а) 2025-11-27 13:50:
> On Tue, 25 Nov 2025 at 15:24, KENAN YILMAZ
> <kenan.yilmaz@localus.com.tr> wrote:
> 
>> Hello Hackers,
>> 
>> I have executed use_cursor = true/false with quick tests.
>> 

Hi.

I've looked at this patch, because also was playing with tuplestore in 
PgFdwScanState to have more predictable memory usage with large 
fetch_size. Unfortunately, I've found that you can't use 
TTSOpsMinimalTuple(?) tuplestore to store tuple's ctid. I hoped this 
patch provides some solution for this issue, but no, it seems have the 
same problem. Attaching test case with a reproducer.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-01-26T11:44:23Z

On Wed, 10 Dec 2025 at 18:44, Robert Haas <robertmhaas@gmail.com> wrote:

>
>
> Overall, I think the direction of the patch set has some promise, but
> I think it needs a lot of cleanup: removal of unnecessary code, proper
> formatting, moving variables to inner scopes, explanatory comments,
> good names for variables and functions and structure members, removal
> of unnecessary files from the patch, cleanup of the regression test
> coverage so that it doesn't add more bloat than necessary, proper
> choice of data structures, and so on. Right now, the good things that
> you've done here are being hidden by these sorts of mechanical issues.
> That's not just an issue for me as a reviewer: I suspect it's also
> blocking you, as the patch author, from finding places where the code
> could be made better. Being able to find such opportunities for
> improvement and act on them is what will get this patch from
> "interesting proof of concept" to "potentially committable patch".
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>

Thanks Robert for your time and attention to this patch.
Based on these review comments and an off list discussion about the design
of the patch, I have reworked the patch significantly.
In this version, a tuplestore is added to the PgFdwScanState along with
a flag. Now, in case of a cursor switch, this tuplestore is filled with the
remaining tuples of the query. The corresponding flag is set to indicate
that the tuplestore is ready to be fetched. To remember the last query that
was executing, a pointer to its PgFdwScanState is maintained in the
conn_state. Note that we only need to remember just the very last query and
once tuples for it are fetched we can forget that pointer, because now its
fsstate has the remaining tuples in the tuplestore.
So, this version of the patch looks simpler than before.
A few test cases are also added in the attached patch to cover the
different scenarios in case of non-cursor mode.
-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-03-04T20:31:51Z

Came to know that there were some CI failures. Posting the new version of
the patch which fixes them.

On Mon, 26 Jan 2026 at 03:44, Rafia Sabih <rafia.pghackers@gmail.com> wrote:

>
>
> On Wed, 10 Dec 2025 at 18:44, Robert Haas <robertmhaas@gmail.com> wrote:
>
>>
>>
>> Overall, I think the direction of the patch set has some promise, but
>> I think it needs a lot of cleanup: removal of unnecessary code, proper
>> formatting, moving variables to inner scopes, explanatory comments,
>> good names for variables and functions and structure members, removal
>> of unnecessary files from the patch, cleanup of the regression test
>> coverage so that it doesn't add more bloat than necessary, proper
>> choice of data structures, and so on. Right now, the good things that
>> you've done here are being hidden by these sorts of mechanical issues.
>> That's not just an issue for me as a reviewer: I suspect it's also
>> blocking you, as the patch author, from finding places where the code
>> could be made better. Being able to find such opportunities for
>> improvement and act on them is what will get this patch from
>> "interesting proof of concept" to "potentially committable patch".
>>
>> --
>> Robert Haas
>> EDB: http://www.enterprisedb.com
>>
>
> Thanks Robert for your time and attention to this patch.
> Based on these review comments and an off list discussion about the design
> of the patch, I have reworked the patch significantly.
> In this version, a tuplestore is added to the PgFdwScanState along with
> a flag. Now, in case of a cursor switch, this tuplestore is filled with the
> remaining tuples of the query. The corresponding flag is set to indicate
> that the tuplestore is ready to be fetched. To remember the last query that
> was executing, a pointer to its PgFdwScanState is maintained in the
> conn_state. Note that we only need to remember just the very last query and
> once tuples for it are fetched we can forget that pointer, because now its
> fsstate has the remaining tuples in the tuplestore.
> So, this version of the patch looks simpler than before.
> A few test cases are also added in the attached patch to cover the
> different scenarios in case of non-cursor mode.
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-03-06T20:50:56Z

On Mon, Jan 26, 2026 at 6:44 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Thanks Robert for your time and attention to this patch.
> Based on these review comments and an off list discussion about the design of the patch, I have reworked the patch significantly.
> In this version, a tuplestore is added to the PgFdwScanState along with a flag. Now, in case of a cursor switch, this tuplestore is filled with the remaining tuples of the query. The corresponding flag is set to indicate that the tuplestore is ready to be fetched. To remember the last query that was executing, a pointer to its PgFdwScanState is maintained in the conn_state. Note that we only need to remember just the very last query and once tuples for it are fetched we can forget that pointer, because now its fsstate has the remaining tuples in the tuplestore.
> So, this version of the patch looks simpler than before.
> A few test cases are also added in the attached patch to cover the different scenarios in case of non-cursor mode.

+       DefineCustomBoolVariable("postgres_fdw.use_cursor",

Usually I would expect the GUC for a few feature to be defined in such
a way that a true values means to use the new feature and a false
value means not to do that. This is reversed. Maybe consider renaming
the GUC so that the sense is inverted.

+       /* To be used only in non-cursor mode */
+       Tuplestorestate *tuplestore;    /* Tuplestore to save the tuples of the
+
  * query for later fetc
h. */
+       TupleTableSlot *slot;           /* Slot to be used when
reading the tuple from
+                                                                * the
tuplestore */
+       bool            tuples_ready;   /* To indicate when tuplestore
is ready to be
+                                                                * read. */
+       int                     total_tuples;   /* total tuples in the
tuplestore. */

There's already a tuplestore_tuple_count() function, so it's not clear
why you need total_tuples. It's also not exactly clear what
tuples_ready means here: sure, it means that we're ready to read
tuples, but when does that happen? I think the /* To be used only in
non-cursor mode */ comment could use expansion, too. Maybe use this to
explain the idea that we're going to use a Tuplestore in non-cursor
mode when, and only when, there are overlapping scans.

+       fsstate->conn_state->last_query = NULL;

I think this should be called something like active_scan. last_query
makes it sound like the last query we ran, whether or not it is still
in progress. But the value that it points to is an PgFdwScanState, so
it's not a query, it's a scan, and we should only have this set to a
non-NULL value for as long as that scan is actually active (i.e. has
the exclusive use of the connection).

+ * This routine fetches all the tuples of a query and saves them in a
tuplestore.
+ * This is required when the result of a query is not completely
fetched but the control
+ * switches to a different query.
+ * A call to fetch_data is made from here, hence we need complete
ForeignScanState here.

This is a good explanation but the last sentence probably isn't
needed. Also, you don't mention that this only used in non-cursor mode
(or whatever we end up calling it).

+       if (conn->asyncStatus == PGASYNC_IDLE)
+       {
+               /* If the connection is not active then set up */
+               if (!PQsendQueryParams(conn, last_fsstate->query,
last_fsstate->numParams,
+                                                          NULL,
values, NULL, NULL, 0))
+                       pgfdw_report_error(NULL, conn, last_fsstate->query);
+
+               if (!PQsetChunkedRowsMode(conn, last_fsstate->fetch_size))
+                       pgfdw_report_error(NULL, conn, last_fsstate->query);
+       }

I think this is a problem for multiple reasons. One problem is that
the function header says that we use this "when the result of a query
is not completely fetched". But if the status where PGASYNC_IDLE, the
query has not even started yet. In that case, seems there is no reason
to call this function in the first place. We can just start the new
scan right away and wait to do anything about the other scan until
it's needed. Additionally, asyncStatus is not part of libpq's exposed
API. You're only able to access it here because you've included
libpq-int.h into postgres_fdw.h, but libpq-int.h is so-called because
it's "internal", so you should really be trying very hard not to use
it, and especially not to include it in another header file. I suggest
that there's probably just a bug here -- I don't think you should need
to check asyncStatus here in the first place.

+ if (pgfdw_use_cursor)
+ snprintf(sql, sizeof(sql), "CLOSE c%u",
+ fsstate->cursor_number);
+ else
+ close_cursor = true;

This is extremely confusing coding. If we're using a cursor, we write
some SQL into a buffer but don't actually do anything with the buffer
right away. But if we're not using a cursor, then we set a flag that
tells us to close a cursor ... which we're not using? I think the
logic updates in the function are not at all clear. You need to back
up and think more carefully about what the function is supposed to do
in each case. Look at how the function starts after your patch:

postgresReScanForeignScan(ForeignScanState *node)
{
    PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
    char        sql[64];
    PGresult   *res;
    bool        close_cursor = false;

    /* If we haven't created the cursor yet, nothing to do. */
    if (!fsstate->cursor_exists)
        return;

Well, if we're in non-cursor mode, that presumably means this function
always returns without doing anything, since there should not be a
cursor if we are not using with them. That can't be right.
Alternatively, maybe it means that the cursor_exists flag is now being
used to track something other than whether a cursor exists, which
wouldn't be right either. I don't know off the top of my head exactly
what this function should do in each case, but as the patch author,
it's your job to go through, figure that out, and make sure that it is
all correct and well-commented.

Similarly, you have a bunch of changes to a function called
create_cursor(), but why would a function that creates a cursor get
changed to handle a mode where we don't create cursors? Either the
function needs to be renamed, or there should be two separate
functions, or these changes don't do anything in the first place.

+ while (res != NULL)
+ res = pgfdw_get_result(fsstate->conn);

I don't see how this code can be right. We should know how many result
sets we expect and read exactly that number, not just read over and
over until there's nothing more. But even if we did want to read until
there's nothing more, we would need to free those result sets rather
than leaking them.

-
- /* Clean up */
- pfree(buf.data);

It's hard to understand why this would be a good idea.

  /*
  * We'll store the tuples in the batch_cxt.  First, flush the previous
  * batch.
  */
  fsstate->tuples = NULL;
- MemoryContextReset(fsstate->batch_cxt);
  oldcontext = MemoryContextSwitchTo(fsstate->batch_cxt);

It's also hard to understand why this would be a good idea. It seems
like it makes the comment false: we wouldn't be flushing the previous
batch any more. We'd just be forgetting that it existed and leaking
the memory.

+ /*
+ * In non-cursor mode, there are three options for the further
+ * processing: 1. If the tuplestore is already filled, retrieve the
+ * tuples from there. 2. Fetch the tuples till the end of the query
+ * and store them in tuplestore. 3. Perform a normal fetch and process
+ * the tuples.

I find this comment pretty confusing. First of all, it's not really
clear what performing a "normal fetch" means here. But also, this says
that in non-cursor mode there are three options, and that's followed
by an if-else block i.e. two options. Three isn't the same as two, so
something isn't right here.

The code that follows is quite involved. It's rather deeply nested in
places, so possibly some of it needs to be broken out into separate
functions. It contains another instance, of this, which has the same
problems I pointed out earlier:

+ /* There are no more tuples to fetch */
+ while (res != NULL)
+ res = pgfdw_get_result(conn);

If there are no more tuples to fetch, then why do we need to loop
fetching result sets until we get a NULL? And if we ever get any, we
leak them, which is also bad.

+ /*
+ * Reset cursor mode when in asynchronous mode as it is not supported in
+ * non-cursor mode
+ */
+ if (!pgfdw_use_cursor)
+ pgfdw_use_cursor = true;

It's not allowed to change a GUC variable from outside of the GUC
code. If you need to disable the optimization in certain cases, you
need a separate flag for that, which is initially set based on
pgfdw_use_cursor and then can be changed here or wherever is needed.
You can't change the GUC value itself. This would cause a variety of
problems, including the fact that the optimization would then stay
disabled for later queries in the session, but probably also some more
GUC-related stuff.

-
 SELECT current_database() AS current_database,
   current_setting('port') AS current_port
 \gset
-

Please avoid including unnecessary cosmetic changes in the patch file.

Overall, I think the test case changes are in much better shape than
they were in earlier patch versions. I suspect there might still be a
bit of room for improvement, but we can deal with that once some of
these other issues are sorted out.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-04-02T18:00:25Z

On Fri, 6 Mar 2026 at 21:51, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Jan 26, 2026 at 6:44 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > Thanks Robert for your time and attention to this patch.
> > Based on these review comments and an off list discussion about the
> design of the patch, I have reworked the patch significantly.
> > In this version, a tuplestore is added to the PgFdwScanState along with
> a flag. Now, in case of a cursor switch, this tuplestore is filled with the
> remaining tuples of the query. The corresponding flag is set to indicate
> that the tuplestore is ready to be fetched. To remember the last query that
> was executing, a pointer to its PgFdwScanState is maintained in the
> conn_state. Note that we only need to remember just the very last query and
> once tuples for it are fetched we can forget that pointer, because now its
> fsstate has the remaining tuples in the tuplestore.
> > So, this version of the patch looks simpler than before.
> > A few test cases are also added in the attached patch to cover the
> different scenarios in case of non-cursor mode.
>
> +       DefineCustomBoolVariable("postgres_fdw.use_cursor",
>
> Usually I would expect the GUC for a few feature to be defined in such
> a way that a true values means to use the new feature and a false
> value means not to do that. This is reversed. Maybe consider renaming
> the GUC so that the sense is inverted.
>
> +       /* To be used only in non-cursor mode */
> +       Tuplestorestate *tuplestore;    /* Tuplestore to save the tuples
> of the
> +
>   * query for later fetc
> h. */
> +       TupleTableSlot *slot;           /* Slot to be used when
> reading the tuple from
> +                                                                * the
> tuplestore */
> +       bool            tuples_ready;   /* To indicate when tuplestore
> is ready to be
> +                                                                * read. */
> +       int                     total_tuples;   /* total tuples in the
> tuplestore. */
>
> There's already a tuplestore_tuple_count() function, so it's not clear
> why you need total_tuples. It's also not exactly clear what
> tuples_ready means here: sure, it means that we're ready to read
> tuples, but when does that happen? I think the /* To be used only in
> non-cursor mode */ comment could use expansion, too. Maybe use this to
> explain the idea that we're going to use a Tuplestore in non-cursor
> mode when, and only when, there are overlapping scans.
>
> +       fsstate->conn_state->last_query = NULL;
>
> I think this should be called something like active_scan. last_query
> makes it sound like the last query we ran, whether or not it is still
> in progress. But the value that it points to is an PgFdwScanState, so
> it's not a query, it's a scan, and we should only have this set to a
> non-NULL value for as long as that scan is actually active (i.e. has
> the exclusive use of the connection).
>
> + * This routine fetches all the tuples of a query and saves them in a
> tuplestore.
> + * This is required when the result of a query is not completely
> fetched but the control
> + * switches to a different query.
> + * A call to fetch_data is made from here, hence we need complete
> ForeignScanState here.
>
> This is a good explanation but the last sentence probably isn't
> needed. Also, you don't mention that this only used in non-cursor mode
> (or whatever we end up calling it).
>
> +       if (conn->asyncStatus == PGASYNC_IDLE)
> +       {
> +               /* If the connection is not active then set up */
> +               if (!PQsendQueryParams(conn, last_fsstate->query,
> last_fsstate->numParams,
> +                                                          NULL,
> values, NULL, NULL, 0))
> +                       pgfdw_report_error(NULL, conn,
> last_fsstate->query);
> +
> +               if (!PQsetChunkedRowsMode(conn, last_fsstate->fetch_size))
> +                       pgfdw_report_error(NULL, conn,
> last_fsstate->query);
> +       }
>
> I think this is a problem for multiple reasons. One problem is that
> the function header says that we use this "when the result of a query
> is not completely fetched". But if the status where PGASYNC_IDLE, the
> query has not even started yet. In that case, seems there is no reason
> to call this function in the first place. We can just start the new
> scan right away and wait to do anything about the other scan until
> it's needed. Additionally, asyncStatus is not part of libpq's exposed
> API. You're only able to access it here because you've included
> libpq-int.h into postgres_fdw.h, but libpq-int.h is so-called because
> it's "internal", so you should really be trying very hard not to use
> it, and especially not to include it in another header file. I suggest
> that there's probably just a bug here -- I don't think you should need
> to check asyncStatus here in the first place.
>
> + if (pgfdw_use_cursor)
> + snprintf(sql, sizeof(sql), "CLOSE c%u",
> + fsstate->cursor_number);
> + else
> + close_cursor = true;
>
> This is extremely confusing coding. If we're using a cursor, we write
> some SQL into a buffer but don't actually do anything with the buffer
> right away. But if we're not using a cursor, then we set a flag that
> tells us to close a cursor ... which we're not using? I think the
> logic updates in the function are not at all clear. You need to back
> up and think more carefully about what the function is supposed to do
> in each case. Look at how the function starts after your patch:
>
> postgresReScanForeignScan(ForeignScanState *node)
> {
>     PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
>     char        sql[64];
>     PGresult   *res;
>     bool        close_cursor = false;
>
>     /* If we haven't created the cursor yet, nothing to do. */
>     if (!fsstate->cursor_exists)
>         return;
>
> Well, if we're in non-cursor mode, that presumably means this function
> always returns without doing anything, since there should not be a
> cursor if we are not using with them. That can't be right.
> Alternatively, maybe it means that the cursor_exists flag is now being
> used to track something other than whether a cursor exists, which
> wouldn't be right either. I don't know off the top of my head exactly
> what this function should do in each case, but as the patch author,
> it's your job to go through, figure that out, and make sure that it is
> all correct and well-commented.
>
> Similarly, you have a bunch of changes to a function called
> create_cursor(), but why would a function that creates a cursor get
> changed to handle a mode where we don't create cursors? Either the
> function needs to be renamed, or there should be two separate
> functions, or these changes don't do anything in the first place.
>
> + while (res != NULL)
> + res = pgfdw_get_result(fsstate->conn);
>
> I don't see how this code can be right. We should know how many result
> sets we expect and read exactly that number, not just read over and
> over until there's nothing more. But even if we did want to read until
> there's nothing more, we would need to free those result sets rather
> than leaking them.
>
> -
> - /* Clean up */
> - pfree(buf.data);
>
> It's hard to understand why this would be a good idea.
>
>   /*
>   * We'll store the tuples in the batch_cxt.  First, flush the previous
>   * batch.
>   */
>   fsstate->tuples = NULL;
> - MemoryContextReset(fsstate->batch_cxt);
>   oldcontext = MemoryContextSwitchTo(fsstate->batch_cxt);
>
> It's also hard to understand why this would be a good idea. It seems
> like it makes the comment false: we wouldn't be flushing the previous
> batch any more. We'd just be forgetting that it existed and leaking
> the memory.
>
> + /*
> + * In non-cursor mode, there are three options for the further
> + * processing: 1. If the tuplestore is already filled, retrieve the
> + * tuples from there. 2. Fetch the tuples till the end of the query
> + * and store them in tuplestore. 3. Perform a normal fetch and process
> + * the tuples.
>
> I find this comment pretty confusing. First of all, it's not really
> clear what performing a "normal fetch" means here. But also, this says
> that in non-cursor mode there are three options, and that's followed
> by an if-else block i.e. two options. Three isn't the same as two, so
> something isn't right here.
>
> The code that follows is quite involved. It's rather deeply nested in
> places, so possibly some of it needs to be broken out into separate
> functions. It contains another instance, of this, which has the same
> problems I pointed out earlier:
>
> + /* There are no more tuples to fetch */
> + while (res != NULL)
> + res = pgfdw_get_result(conn);
>
> If there are no more tuples to fetch, then why do we need to loop
> fetching result sets until we get a NULL? And if we ever get any, we
> leak them, which is also bad.
>
> + /*
> + * Reset cursor mode when in asynchronous mode as it is not supported in
> + * non-cursor mode
> + */
> + if (!pgfdw_use_cursor)
> + pgfdw_use_cursor = true;
>
> It's not allowed to change a GUC variable from outside of the GUC
> code. If you need to disable the optimization in certain cases, you
> need a separate flag for that, which is initially set based on
> pgfdw_use_cursor and then can be changed here or wherever is needed.
> You can't change the GUC value itself. This would cause a variety of
> problems, including the fact that the optimization would then stay
> disabled for later queries in the session, but probably also some more
> GUC-related stuff.
>
> -
>  SELECT current_database() AS current_database,
>    current_setting('port') AS current_port
>  \gset
> -
>
> Please avoid including unnecessary cosmetic changes in the patch file.
>
> Overall, I think the test case changes are in much better shape than
> they were in earlier patch versions. I suspect there might still be a
> bit of room for improvement, but we can deal with that once some of
> these other issues are sorted out.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>

Thank you for the detailed review of the patch. Please find the attached
file for the updated patch.
To summarise the changes done in this patch following these review comments,
- the new GUC is called disable_cursor, so when we want to use this feature
we set it, otherwise it remains false by default. I hope this makes it
easier to understand as you mentioned.

- Simplified the code in fetch_more_data for this new mode. Now we have a
function called fetch_from_tuplestore, and it is called from
fetch_more_data when tuples from tuplestore are requested. Another function
called save_to_tuplestore is also introduced in this patch, it is called
from create_cursor code when all the tuples are to be fetched for the
active_scan. So things look better code readability wise.

- For handling the async mode, I went to one level up and modified
async_capable to false when no_cursor mode is used. So, now we do not
change any GUC as such.

- With regards to use of flag cursor_exists and functions create_cursor and
close_cursor, these are used in this no_cursor mode as well. For the flag
cursor_exists, I used it to check if we have already done the required
steps for when disale_cursor is set, like setting the ChunkedRowsMode,
saving the tuples to the tuplestore if required. Inherently there is no
nothing in fsstate or conn_state through which we can identify that all the
required steps are done or if this scan is coming for the first time, so
maybe we can have a different flag for the case when disable_cursor is set
and use it for such checks. But to me it feels like additional code when I
can use existing vars, but I agree on making it more readable that way. So,
I modified in this patch to add a separate flag called scan_init and
functions init_scan and end_scan for the setting and resetting of the
required values when cursors are not used. So, let me know what you think
is better using the existing flag or creating a new one for this mode as
done in this patch.

- For this change,
  /*
  * We'll store the tuples in the batch_cxt.  First, flush the previous
  * batch.
  */
  fsstate->tuples = NULL;
- MemoryContextReset(fsstate->batch_cxt);
  oldcontext = MemoryContextSwitchTo(fsstate->batch_cxt);
since, the tuplestore is now created in batch_cxt, so we can't reset it
here. But I now changed it to be done only when disable_cursor is set.

- Regarding the following change,
+       if (conn->asyncStatus == PGASYNC_IDLE)
+       {
+               /* If the connection is not active then set up */
When we are here after the call to the rescan, the connection is as per the
current fsstate which has not yet started, hence the idle status, but we
are here to fetch the tuples for the active scan which got reset after the
rescan so it is required to set it up again. But you are right in that
using asyncStatus is not appropriate here, so I added a new flag to check
for rescan, which is set in rescan and used here to decide if we need to
set the ChunkedRowsMode.

- Modified and added more comments
-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-04-10T07:41:48Z

Rebased version of the patch is attached.
It is a minor change from the earlier version.

On Thu, 2 Apr 2026 at 20:00, Rafia Sabih <rafia.pghackers@gmail.com> wrote:

>
>
> On Fri, 6 Mar 2026 at 21:51, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> On Mon, Jan 26, 2026 at 6:44 AM Rafia Sabih <rafia.pghackers@gmail.com>
>> wrote:
>> > Thanks Robert for your time and attention to this patch.
>> > Based on these review comments and an off list discussion about the
>> design of the patch, I have reworked the patch significantly.
>> > In this version, a tuplestore is added to the PgFdwScanState along with
>> a flag. Now, in case of a cursor switch, this tuplestore is filled with the
>> remaining tuples of the query. The corresponding flag is set to indicate
>> that the tuplestore is ready to be fetched. To remember the last query that
>> was executing, a pointer to its PgFdwScanState is maintained in the
>> conn_state. Note that we only need to remember just the very last query and
>> once tuples for it are fetched we can forget that pointer, because now its
>> fsstate has the remaining tuples in the tuplestore.
>> > So, this version of the patch looks simpler than before.
>> > A few test cases are also added in the attached patch to cover the
>> different scenarios in case of non-cursor mode.
>>
>> +       DefineCustomBoolVariable("postgres_fdw.use_cursor",
>>
>> Usually I would expect the GUC for a few feature to be defined in such
>> a way that a true values means to use the new feature and a false
>> value means not to do that. This is reversed. Maybe consider renaming
>> the GUC so that the sense is inverted.
>>
>> +       /* To be used only in non-cursor mode */
>> +       Tuplestorestate *tuplestore;    /* Tuplestore to save the tuples
>> of the
>> +
>>   * query for later fetc
>> h. */
>> +       TupleTableSlot *slot;           /* Slot to be used when
>> reading the tuple from
>> +                                                                * the
>> tuplestore */
>> +       bool            tuples_ready;   /* To indicate when tuplestore
>> is ready to be
>> +                                                                * read.
>> */
>> +       int                     total_tuples;   /* total tuples in the
>> tuplestore. */
>>
>> There's already a tuplestore_tuple_count() function, so it's not clear
>> why you need total_tuples. It's also not exactly clear what
>> tuples_ready means here: sure, it means that we're ready to read
>> tuples, but when does that happen? I think the /* To be used only in
>> non-cursor mode */ comment could use expansion, too. Maybe use this to
>> explain the idea that we're going to use a Tuplestore in non-cursor
>> mode when, and only when, there are overlapping scans.
>>
>> +       fsstate->conn_state->last_query = NULL;
>>
>> I think this should be called something like active_scan. last_query
>> makes it sound like the last query we ran, whether or not it is still
>> in progress. But the value that it points to is an PgFdwScanState, so
>> it's not a query, it's a scan, and we should only have this set to a
>> non-NULL value for as long as that scan is actually active (i.e. has
>> the exclusive use of the connection).
>>
>> + * This routine fetches all the tuples of a query and saves them in a
>> tuplestore.
>> + * This is required when the result of a query is not completely
>> fetched but the control
>> + * switches to a different query.
>> + * A call to fetch_data is made from here, hence we need complete
>> ForeignScanState here.
>>
>> This is a good explanation but the last sentence probably isn't
>> needed. Also, you don't mention that this only used in non-cursor mode
>> (or whatever we end up calling it).
>>
>> +       if (conn->asyncStatus == PGASYNC_IDLE)
>> +       {
>> +               /* If the connection is not active then set up */
>> +               if (!PQsendQueryParams(conn, last_fsstate->query,
>> last_fsstate->numParams,
>> +                                                          NULL,
>> values, NULL, NULL, 0))
>> +                       pgfdw_report_error(NULL, conn,
>> last_fsstate->query);
>> +
>> +               if (!PQsetChunkedRowsMode(conn, last_fsstate->fetch_size))
>> +                       pgfdw_report_error(NULL, conn,
>> last_fsstate->query);
>> +       }
>>
>> I think this is a problem for multiple reasons. One problem is that
>> the function header says that we use this "when the result of a query
>> is not completely fetched". But if the status where PGASYNC_IDLE, the
>> query has not even started yet. In that case, seems there is no reason
>> to call this function in the first place. We can just start the new
>> scan right away and wait to do anything about the other scan until
>> it's needed. Additionally, asyncStatus is not part of libpq's exposed
>> API. You're only able to access it here because you've included
>> libpq-int.h into postgres_fdw.h, but libpq-int.h is so-called because
>> it's "internal", so you should really be trying very hard not to use
>> it, and especially not to include it in another header file. I suggest
>> that there's probably just a bug here -- I don't think you should need
>> to check asyncStatus here in the first place.
>>
>> + if (pgfdw_use_cursor)
>> + snprintf(sql, sizeof(sql), "CLOSE c%u",
>> + fsstate->cursor_number);
>> + else
>> + close_cursor = true;
>>
>> This is extremely confusing coding. If we're using a cursor, we write
>> some SQL into a buffer but don't actually do anything with the buffer
>> right away. But if we're not using a cursor, then we set a flag that
>> tells us to close a cursor ... which we're not using? I think the
>> logic updates in the function are not at all clear. You need to back
>> up and think more carefully about what the function is supposed to do
>> in each case. Look at how the function starts after your patch:
>>
>> postgresReScanForeignScan(ForeignScanState *node)
>> {
>>     PgFdwScanState *fsstate = (PgFdwScanState *) node->fdw_state;
>>     char        sql[64];
>>     PGresult   *res;
>>     bool        close_cursor = false;
>>
>>     /* If we haven't created the cursor yet, nothing to do. */
>>     if (!fsstate->cursor_exists)
>>         return;
>>
>> Well, if we're in non-cursor mode, that presumably means this function
>> always returns without doing anything, since there should not be a
>> cursor if we are not using with them. That can't be right.
>> Alternatively, maybe it means that the cursor_exists flag is now being
>> used to track something other than whether a cursor exists, which
>> wouldn't be right either. I don't know off the top of my head exactly
>> what this function should do in each case, but as the patch author,
>> it's your job to go through, figure that out, and make sure that it is
>> all correct and well-commented.
>>
>> Similarly, you have a bunch of changes to a function called
>> create_cursor(), but why would a function that creates a cursor get
>> changed to handle a mode where we don't create cursors? Either the
>> function needs to be renamed, or there should be two separate
>> functions, or these changes don't do anything in the first place.
>>
>> + while (res != NULL)
>> + res = pgfdw_get_result(fsstate->conn);
>>
>> I don't see how this code can be right. We should know how many result
>> sets we expect and read exactly that number, not just read over and
>> over until there's nothing more. But even if we did want to read until
>> there's nothing more, we would need to free those result sets rather
>> than leaking them.
>>
>> -
>> - /* Clean up */
>> - pfree(buf.data);
>>
>> It's hard to understand why this would be a good idea.
>>
>>   /*
>>   * We'll store the tuples in the batch_cxt.  First, flush the previous
>>   * batch.
>>   */
>>   fsstate->tuples = NULL;
>> - MemoryContextReset(fsstate->batch_cxt);
>>   oldcontext = MemoryContextSwitchTo(fsstate->batch_cxt);
>>
>> It's also hard to understand why this would be a good idea. It seems
>> like it makes the comment false: we wouldn't be flushing the previous
>> batch any more. We'd just be forgetting that it existed and leaking
>> the memory.
>>
>> + /*
>> + * In non-cursor mode, there are three options for the further
>> + * processing: 1. If the tuplestore is already filled, retrieve the
>> + * tuples from there. 2. Fetch the tuples till the end of the query
>> + * and store them in tuplestore. 3. Perform a normal fetch and process
>> + * the tuples.
>>
>> I find this comment pretty confusing. First of all, it's not really
>> clear what performing a "normal fetch" means here. But also, this says
>> that in non-cursor mode there are three options, and that's followed
>> by an if-else block i.e. two options. Three isn't the same as two, so
>> something isn't right here.
>>
>> The code that follows is quite involved. It's rather deeply nested in
>> places, so possibly some of it needs to be broken out into separate
>> functions. It contains another instance, of this, which has the same
>> problems I pointed out earlier:
>>
>> + /* There are no more tuples to fetch */
>> + while (res != NULL)
>> + res = pgfdw_get_result(conn);
>>
>> If there are no more tuples to fetch, then why do we need to loop
>> fetching result sets until we get a NULL? And if we ever get any, we
>> leak them, which is also bad.
>>
>> + /*
>> + * Reset cursor mode when in asynchronous mode as it is not supported in
>> + * non-cursor mode
>> + */
>> + if (!pgfdw_use_cursor)
>> + pgfdw_use_cursor = true;
>>
>> It's not allowed to change a GUC variable from outside of the GUC
>> code. If you need to disable the optimization in certain cases, you
>> need a separate flag for that, which is initially set based on
>> pgfdw_use_cursor and then can be changed here or wherever is needed.
>> You can't change the GUC value itself. This would cause a variety of
>> problems, including the fact that the optimization would then stay
>> disabled for later queries in the session, but probably also some more
>> GUC-related stuff.
>>
>> -
>>  SELECT current_database() AS current_database,
>>    current_setting('port') AS current_port
>>  \gset
>> -
>>
>> Please avoid including unnecessary cosmetic changes in the patch file.
>>
>> Overall, I think the test case changes are in much better shape than
>> they were in earlier patch versions. I suspect there might still be a
>> bit of room for improvement, but we can deal with that once some of
>> these other issues are sorted out.
>>
>> --
>> Robert Haas
>> EDB: http://www.enterprisedb.com
>>
>
> Thank you for the detailed review of the patch. Please find the attached
> file for the updated patch.
> To summarise the changes done in this patch following these review
> comments,
> - the new GUC is called disable_cursor, so when we want to use this
> feature we set it, otherwise it remains false by default. I hope this makes
> it easier to understand as you mentioned.
>
> - Simplified the code in fetch_more_data for this new mode. Now we have a
> function called fetch_from_tuplestore, and it is called from
> fetch_more_data when tuples from tuplestore are requested. Another function
> called save_to_tuplestore is also introduced in this patch, it is called
> from create_cursor code when all the tuples are to be fetched for the
> active_scan. So things look better code readability wise.
>
> - For handling the async mode, I went to one level up and modified
> async_capable to false when no_cursor mode is used. So, now we do not
> change any GUC as such.
>
> - With regards to use of flag cursor_exists and functions create_cursor
> and close_cursor, these are used in this no_cursor mode as well. For the
> flag cursor_exists, I used it to check if we have already done the required
> steps for when disale_cursor is set, like setting the ChunkedRowsMode,
> saving the tuples to the tuplestore if required. Inherently there is no
> nothing in fsstate or conn_state through which we can identify that all the
> required steps are done or if this scan is coming for the first time, so
> maybe we can have a different flag for the case when disable_cursor is set
> and use it for such checks. But to me it feels like additional code when I
> can use existing vars, but I agree on making it more readable that way. So,
> I modified in this patch to add a separate flag called scan_init and
> functions init_scan and end_scan for the setting and resetting of the
> required values when cursors are not used. So, let me know what you think
> is better using the existing flag or creating a new one for this mode as
> done in this patch.
>
> - For this change,
>   /*
>   * We'll store the tuples in the batch_cxt.  First, flush the previous
>   * batch.
>   */
>   fsstate->tuples = NULL;
> - MemoryContextReset(fsstate->batch_cxt);
>   oldcontext = MemoryContextSwitchTo(fsstate->batch_cxt);
> since, the tuplestore is now created in batch_cxt, so we can't reset it
> here. But I now changed it to be done only when disable_cursor is set.
>
> - Regarding the following change,
> +       if (conn->asyncStatus == PGASYNC_IDLE)
> +       {
> +               /* If the connection is not active then set up */
> When we are here after the call to the rescan, the connection is as per
> the current fsstate which has not yet started, hence the idle status, but
> we are here to fetch the tuples for the active scan which got reset after
> the rescan so it is required to set it up again. But you are right in that
> using asyncStatus is not appropriate here, so I added a new flag to check
> for rescan, which is set in rescan and used here to decide if we need to
> set the ChunkedRowsMode.
>
> - Modified and added more comments
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-04-13T19:30:36Z

On Fri, Apr 10, 2026 at 3:42 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Rebased version of the patch is attached.
> It is a minor change from the earlier version.

Hi,

Some issues:

- fetch_more_data() still gains a use_tuplestore parameter, but now
it's always false and never used for anything. already_done is also
dead code; it's never set to anything but false. There are also other
places in the patch where you haven't cleaned up things that aren't
needed any more.

- I think that instead of add scan_init alongside cursor_exists, it
might be better to just rename the existing cursor_exists flag and use
it in both modes. Right now, you've got a bunch of places in the code
where when disable_cursor has one value you look at cursor_exists and
when it has the other value you look at scan_init; basically, the two
flags seem to be the same thing, and only the name of the flag doesn't
make sense after your patch. But I would suggest calling it
scan_in_progress rather than scan_init, because it gets set back to
false in postgresEndForeignScan() when the scan finishes. In fact, I
would suggest creating a preparatory patch that does the cursor_exists
-> scan_in_progress renaming and make that 0001, and then make your
main patch 0002 on top of that. That patch could possibly be committed
separately at some point.

- postgresBeginForeignScan incorrectly does this:

    /* Initially, there is no active_scan */
    fsstate->conn_state->active_scan = NULL;

I think this is compensating for the lack of anything to clear
active_scan in postgresEndForeignScan; if you didn't do this, then
probably the first query using non-cursor mode would work and the
second one would fail. But the right fix is to remove this and make
postgresEndForeignScan clean it up properly instead. Otherwise,
imagine that query A begins using a postgres_fdw connection and is in
the middle of streaming a result set over the wire when a call to a
user-defined function triggers query B which accesses the same
postgres_fdw connection. This logic now nulls out the active-scan
pointer from the outer connection while in reality that outer
connection is still active. I expect terrible things to happen after
that. You might want to try to create a test case that exercises this
scenario.

- The logic in postgresReScanForeignScan() has multiple flaws. One of
these is, as you probably know already, where it says /* TODO: Handle
it in non-cursor mode as well */. Let's think through how this needs
to work a little bit. When we're using a cursor, we have three
possible strategies: we can close the cursor and re-execute the query,
we can use MOVE BACKWARD ALL, or if we still have every tuple we've
ever fetched from the remote side, we can just rescan them (the "Easy"
case). When there's no cursor, only two strategies are possible --
there's no equivalent of MOVE BACKWARD ALL. We can either throw away
the tuplestore we've accumulated and any results that are still being
received from the remote side (which is similar to "CLOSE") or we just
rewind our internal scan position (the "Easy" case).

So what I would suggest here is: set a local variable
reinitialize_scan = false at the top. In the cases where we currently
print CLOSE c%u, just set reinitialize_scan = true. In the MOVE
BACKWARD ALL case (fetch_ct_2 > 1 and PQserverVersion(fsstate->conn) <
150000 and !disable_cursor), construct the query, execute it, and
return from the function right there. Otherwise, fall through to the
end of the function and deal with what reinitializing the scan means
in the cursor case (CLOSE %u) and in the non-cursor case. But here we
come to a second bug: right now, you try to reinitialize the scan in
the disable_cursor case by just calling pgfdw_get_result(), but you do
so without checking whether the scan that we're rescanning is the same
as the active scan. If it is, then you need to do this, but if it
isn't, then you must not. Either way, you might also need to discard
any tuplestore that has been collected.

There's also the case where you don't reinitialize the scan, which is
the easy case: whether cursors are in use or not, you just need to
reset your notion of what's been fetched. But here we come to another
problem: fetch_ct_2 is, I would argue, not maintained properly in
non-cursor mode. In cursor mode, 0 means we haven't yet read any
tuples, 1 means we've read some tuples but not yet thrown any away,
and 2 means that some have been discarded. If it had the same meaning
in non-cursor mode, then the rescan code could use that value to
decide whether to take the "Easy" path where it just resets
next_tuple. But it doesn't: fetch_ct_2 in non-cursor mode just counts
the number of calls to fetch_more_data(), which doesn't tell us
anything about what rescan should do.

- The logic in postgresEndForeignScan has one of the same problems
that I mentioned in the previous point: it will call
pgfdw_get_result() even when the scan being ended is not the active
scan, and it doesn't deal with the associated tuplestore.

- save_to_tuplestore() gets confused between the scan FROM which it's
being called and the scan FOR which it's being called, or said
differently, it confuses the currently-active scan with the new scan
that we want to initiate. For instance, it tests fsscan->rescan to
decide whether to resend active_fsstate->query. That doesn't really
make any sense. I think a general problem with this patch is that it's
not clear exactly what the rescan flag is actually intended to mean,
but it certainly can't be right to decide that the query for plan node
A needs to be re-sent because plan node B is being rescanned. A little
further down, you switch to fsstate->batch_cxt to store tuples in
active_fsstate->tuplestore, which is another example of the same
confusion. You should probably have a function here that only gets the
*active* scan as an argument, so that you can't get mixed up like this
and mistakenly refer to the wrong one. We don't even know that fsstate
and active_fsstate are from the same query: recall the case I
mentioned earlier, where query A begins using a postgres_fdw
connection and is in the middle of streaming a result set over the
wire when a call to a user-defined function triggers query B which
accesses the same postgres_fdw connection.

- There are multiple points in save_to_tuplestore() where you can
overwrite res without calling PQclear() on the previous value of res.
You need to make sure that every place in the patch where you create a
PGresult, you also free it.

- You need to think about the fact that the GUC can be changed at any
time. disable_cursor could be true when the scan starts and false by
the time it ends, or false by the time it's rescanned, or it could be
true for an outer query that starts running a query on the connection
and then false by the time an inner level wants to use the same
connection for something else. Right now, if anything like that
happens, this code is going to fail terribly. I think you need every
scan to remember whether it was started with or without a cursor, and
do rescans and end-scans based on that, not the GUC. And even if the
*current* scan is being started with a cursor, that doesn't mean there
can't be a *previous* scan in progress that was started without using
a cursor; you need to make sure, for example, that
save_to_tuplestore() is still called in that case.

- I know I suggested inverting the sense of the GUC (the feature
should say what it does, not what it doesn't do) but I don't really
like disable_cursor as the name of the feature, either, because
defining the feature in terms of shutting off cursors is kind of
confusing. I think we should come up with a name for this feature that
doesn't have a negation in the name. Maybe something like
postgres_fdw.streaming_fetch = { true | false }? Or
postgres_fdw.fetch_mode = { cursor | stream }? That would be similar
to the existing fetch_size option; one could even argue for allowing
fetch_size = 0 to mean "don't use a cursor, just fetch everything,"
although maybe overloading the setting that way is a little too clever
(or maybe it's fine).

- But also note that postgres_fdw.fetch_size -- which, again, is
closely related to this feature -- is a table or server level option,
not a GUC. That suggests that this feature should also be a table or
server level option.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-05-11T11:03:09Z

On Mon, 13 Apr 2026 at 21:30, Robert Haas <robertmhaas@gmail.com> wrote:

> On Fri, Apr 10, 2026 at 3:42 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > Rebased version of the patch is attached.
> > It is a minor change from the earlier version.
>
> Hi,
>
> Some issues:
>
> - fetch_more_data() still gains a use_tuplestore parameter, but now
> it's always false and never used for anything. already_done is also
> dead code; it's never set to anything but false. There are also other
> places in the patch where you haven't cleaned up things that aren't
> needed any more.
>
> - I think that instead of add scan_init alongside cursor_exists, it
> might be better to just rename the existing cursor_exists flag and use
> it in both modes. Right now, you've got a bunch of places in the code
> where when disable_cursor has one value you look at cursor_exists and
> when it has the other value you look at scan_init; basically, the two
> flags seem to be the same thing, and only the name of the flag doesn't
> make sense after your patch. But I would suggest calling it
> scan_in_progress rather than scan_init, because it gets set back to
> false in postgresEndForeignScan() when the scan finishes. In fact, I
> would suggest creating a preparatory patch that does the cursor_exists
> -> scan_in_progress renaming and make that 0001, and then make your
> main patch 0002 on top of that. That patch could possibly be committed
> separately at some point.
>
> - postgresBeginForeignScan incorrectly does this:
>
>     /* Initially, there is no active_scan */
>     fsstate->conn_state->active_scan = NULL;
>
> I think this is compensating for the lack of anything to clear
> active_scan in postgresEndForeignScan; if you didn't do this, then
> probably the first query using non-cursor mode would work and the
> second one would fail. But the right fix is to remove this and make
> postgresEndForeignScan clean it up properly instead. Otherwise,
> imagine that query A begins using a postgres_fdw connection and is in
> the middle of streaming a result set over the wire when a call to a
> user-defined function triggers query B which accesses the same
> postgres_fdw connection. This logic now nulls out the active-scan
> pointer from the outer connection while in reality that outer
> connection is still active. I expect terrible things to happen after
> that. You might want to try to create a test case that exercises this
> scenario.
>
> - The logic in postgresReScanForeignScan() has multiple flaws. One of
> these is, as you probably know already, where it says /* TODO: Handle
> it in non-cursor mode as well */. Let's think through how this needs
> to work a little bit. When we're using a cursor, we have three
> possible strategies: we can close the cursor and re-execute the query,
> we can use MOVE BACKWARD ALL, or if we still have every tuple we've
> ever fetched from the remote side, we can just rescan them (the "Easy"
> case). When there's no cursor, only two strategies are possible --
> there's no equivalent of MOVE BACKWARD ALL. We can either throw away
> the tuplestore we've accumulated and any results that are still being
> received from the remote side (which is similar to "CLOSE") or we just
> rewind our internal scan position (the "Easy" case).
>
> So what I would suggest here is: set a local variable
> reinitialize_scan = false at the top. In the cases where we currently
> print CLOSE c%u, just set reinitialize_scan = true. In the MOVE
> BACKWARD ALL case (fetch_ct_2 > 1 and PQserverVersion(fsstate->conn) <
> 150000 and !disable_cursor), construct the query, execute it, and
> return from the function right there. Otherwise, fall through to the
> end of the function and deal with what reinitializing the scan means
> in the cursor case (CLOSE %u) and in the non-cursor case. But here we
> come to a second bug: right now, you try to reinitialize the scan in
> the disable_cursor case by just calling pgfdw_get_result(), but you do
> so without checking whether the scan that we're rescanning is the same
> as the active scan. If it is, then you need to do this, but if it
> isn't, then you must not. Either way, you might also need to discard
> any tuplestore that has been collected.
>
> There's also the case where you don't reinitialize the scan, which is
> the easy case: whether cursors are in use or not, you just need to
> reset your notion of what's been fetched. But here we come to another
> problem: fetch_ct_2 is, I would argue, not maintained properly in
> non-cursor mode. In cursor mode, 0 means we haven't yet read any
> tuples, 1 means we've read some tuples but not yet thrown any away,
> and 2 means that some have been discarded. If it had the same meaning
> in non-cursor mode, then the rescan code could use that value to
> decide whether to take the "Easy" path where it just resets
> next_tuple. But it doesn't: fetch_ct_2 in non-cursor mode just counts
> the number of calls to fetch_more_data(), which doesn't tell us
> anything about what rescan should do.
>
> - The logic in postgresEndForeignScan has one of the same problems
> that I mentioned in the previous point: it will call
> pgfdw_get_result() even when the scan being ended is not the active
> scan, and it doesn't deal with the associated tuplestore.
>
> - save_to_tuplestore() gets confused between the scan FROM which it's
> being called and the scan FOR which it's being called, or said
> differently, it confuses the currently-active scan with the new scan
> that we want to initiate. For instance, it tests fsscan->rescan to
> decide whether to resend active_fsstate->query. That doesn't really
> make any sense. I think a general problem with this patch is that it's
> not clear exactly what the rescan flag is actually intended to mean,
> but it certainly can't be right to decide that the query for plan node
> A needs to be re-sent because plan node B is being rescanned. A little
> further down, you switch to fsstate->batch_cxt to store tuples in
> active_fsstate->tuplestore, which is another example of the same
> confusion. You should probably have a function here that only gets the
> *active* scan as an argument, so that you can't get mixed up like this
> and mistakenly refer to the wrong one. We don't even know that fsstate
> and active_fsstate are from the same query: recall the case I
> mentioned earlier, where query A begins using a postgres_fdw
> connection and is in the middle of streaming a result set over the
> wire when a call to a user-defined function triggers query B which
> accesses the same postgres_fdw connection.
>
> - There are multiple points in save_to_tuplestore() where you can
> overwrite res without calling PQclear() on the previous value of res.
> You need to make sure that every place in the patch where you create a
> PGresult, you also free it.
>
> - You need to think about the fact that the GUC can be changed at any
> time. disable_cursor could be true when the scan starts and false by
> the time it ends, or false by the time it's rescanned, or it could be
> true for an outer query that starts running a query on the connection
> and then false by the time an inner level wants to use the same
> connection for something else. Right now, if anything like that
> happens, this code is going to fail terribly. I think you need every
> scan to remember whether it was started with or without a cursor, and
> do rescans and end-scans based on that, not the GUC. And even if the
> *current* scan is being started with a cursor, that doesn't mean there
> can't be a *previous* scan in progress that was started without using
> a cursor; you need to make sure, for example, that
> save_to_tuplestore() is still called in that case.
>
> - I know I suggested inverting the sense of the GUC (the feature
> should say what it does, not what it doesn't do) but I don't really
> like disable_cursor as the name of the feature, either, because
> defining the feature in terms of shutting off cursors is kind of
> confusing. I think we should come up with a name for this feature that
> doesn't have a negation in the name. Maybe something like
> postgres_fdw.streaming_fetch = { true | false }? Or
> postgres_fdw.fetch_mode = { cursor | stream }? That would be similar
> to the existing fetch_size option; one could even argue for allowing
> fetch_size = 0 to mean "don't use a cursor, just fetch everything,"
> although maybe overloading the setting that way is a little too clever
> (or maybe it's fine).
>
> - But also note that postgres_fdw.fetch_size -- which, again, is
> closely related to this feature -- is a table or server level option,
> not a GUC. That suggests that this feature should also be a table or
> server level option.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>

Hello there fellow hackers,

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.
-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-05-12T12:56:26Z

On Mon, May 11, 2026 at 7:03 AM Rafia Sabih <rafia.pghackers@gmail.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.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-05-20T10:40:11Z

On Tue, 12 May 2026 at 14:56, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, May 11, 2026 at 7:03 AM Rafia Sabih <rafia.pghackers@gmail.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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-06-05T09:03:07Z

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

On Wed, 20 May 2026 at 12:40, Rafia Sabih <rafia.pghackers@gmail.com> wrote:

>
>
> On Tue, 12 May 2026 at 14:56, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> On Mon, May 11, 2026 at 7:03 AM Rafia Sabih <rafia.pghackers@gmail.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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-06-16T12:58:32Z

On Fri, Jun 5, 2026 at 5:03 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> One of the patch required a rebase, hence attaching the rebased patches.

Did you generate these patches with "git format-patch"? They don't
apply for me, because 0002 contains some of the same changes that are
also present in 0001. I strongly recommend always generating the patch
sets you post from a private branch using "git rebase master" followed
by "git format-patch -v$VERSION master".

You added a new member to FdwScanPrivateIndex which is fine, but you
put it in the middle instead of at the end, which might be OK but is
slightly surprising, and you didn't add a comment on the new member,
which is definitely not OK.

It appears to me that save_to_tuplestore() still has the same problems
that I have mentioned in previous reviews -- maybe not quite as badly,
but to some degree. For example, it still knows about both fsstate and
active_fsstate. Mostly, the references seem to all point to
active_fsstate now, but there is still a comment that refers to just
fsstate, and this whole setup of having two variables like this is
just very error-prone. I think it's really important to get rid of
that. Also, I still think this is wrong:

+       if (active_fsstate->rescan)
+       {
+               if (!PQsendQueryParams(conn, active_fsstate->query,
active_fsstate->numParams,
+                                                          NULL,
active_fsstate->param_values, NULL, NULL, 0))
+                       pgfdw_report_error(NULL, conn, active_fsstate->query);

This is exactly what I talked about in
http://postgr.es/m/CA+TgmoZfxpohj1ag8PQyshKpFu+krUgeH0Xd=34Wvn29F5DWhw@mail.gmail.com
but it seems that it's not fixed: we're still sending the query if it
hasn't been sent yet, and we just shouldn't be doing that. I don't
think it's the right idea to have a "bool rescan" flag at all.
postgresReScanForeignScan just sets so that save_to_tuplestore can use
it to send the query when that's not actually needed in the first
place.

+       if (fsstate->conn_state->active_scan &&
+               fsstate != fsstate->conn_state->active_scan)
+               save_to_tuplestore(node);

I don't think this logic is correct. We shouldn't be trying to
initialize the current scan if the current scan is already active. It
seems to me that the only condition here should be the first one -- if
(fsstate_conn_state->active_scan == NULL) save_to_tuplestore(node). If
that's not sufficient, then it implies that init_scan() can be called
in some situation where the scan is already initialized, which is a
bug that should be fixed at its source, rather than here.

I generally recommend avoiding mention of specific details of the C
code in the regression tests. If you do that, as you have done, then
the SQL comments can need to be updated when the C code is changed,
even if the behavior has not changed, which is annoying.

I think that the changes to make_tuple_from_result_row are unlikely to
be correct. Why would this special handling be needed? If we're
passing the correct fsstate and the correct rel, then I don't
understand why the previous logic would produce the wrong answer. I
wonder if this is a holdover from an earlier patch version where
save_to_tuplestore() was more buggy than it is now. Or maybe it still
has a bug that we should fix. But I don't think changing
make_tuple_from_result_row is likely to be the right answer.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-06-23T09:38:41Z

On Tue, 16 Jun 2026 at 14:58, Robert Haas <robertmhaas@gmail.com> wrote:

> On Fri, Jun 5, 2026 at 5:03 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > One of the patch required a rebase, hence attaching the rebased patches.
>
> Did you generate these patches with "git format-patch"? They don't
> apply for me, because 0002 contains some of the same changes that are
> also present in 0001. I strongly recommend always generating the patch
> sets you post from a private branch using "git rebase master" followed
> by "git format-patch -v$VERSION master".
>
The attached version should work properly.

>
> You added a new member to FdwScanPrivateIndex which is fine, but you
> put it in the middle instead of at the end, which might be OK but is
> slightly surprising, and you didn't add a comment on the new member,
> which is definitely not OK.
>
 I added the comment now. I am not sure about the position, please do
suggest if it is better at the end, I just added it to being closest to
fetch_size.

>
> It appears to me that save_to_tuplestore() still has the same problems
> that I have mentioned in previous reviews -- maybe not quite as badly,
> but to some degree. For example, it still knows about both fsstate and
> active_fsstate. Mostly, the references seem to all point to
> active_fsstate now, but there is still a comment that refers to just
> fsstate, and this whole setup of having two variables like this is
> just very error-prone. I think it's really important to get rid of
>

I understand your concern and I tried to solve it by passing fsstate now,
also saving a backpointer to the node in active_fsstate to solve the issue
with make_tuple_from_result_row. Since we need to have conn from fsstate, I
am not sure how we can do that if we have only active_fsstate passed to the
function.

> that. Also, I still think this is wrong:
>
> +       if (active_fsstate->rescan)
> +       {
> +               if (!PQsendQueryParams(conn, active_fsstate->query,
> active_fsstate->numParams,
> +                                                          NULL,
> active_fsstate->param_values, NULL, NULL, 0))
> +                       pgfdw_report_error(NULL, conn,
> active_fsstate->query);
>
> This is exactly what I talked about in
>
> http://postgr.es/m/CA+TgmoZfxpohj1ag8PQyshKpFu+krUgeH0Xd=34Wvn29F5DWhw@mail.gmail.com
> but it seems that it's not fixed: we're still sending the query if it
> hasn't been sent yet, and we just shouldn't be doing that. I don't
> think it's the right idea to have a "bool rescan" flag at all.
> postgresReScanForeignScan just sets so that save_to_tuplestore can use
> it to send the query when that's not actually needed in the first
> place.
>
Yes, it is removed in this version of the patch.

>
> +       if (fsstate->conn_state->active_scan &&
> +               fsstate != fsstate->conn_state->active_scan)
> +               save_to_tuplestore(node);
>
> I don't think this logic is correct. We shouldn't be trying to
> initialize the current scan if the current scan is already active. It
> seems to me that the only condition here should be the first one -- if
> (fsstate_conn_state->active_scan == NULL) save_to_tuplestore(node). If
> that's not sufficient, then it implies that init_scan() can be called
> in some situation where the scan is already initialized, which is a
> bug that should be fixed at its source, rather than here.
>
> I generally recommend avoiding mention of specific details of the C
> code in the regression tests. If you do that, as you have done, then
> the SQL comments can need to be updated when the C code is changed,
> even if the behavior has not changed, which is annoying.
>
> Changed the comments.

> I think that the changes to make_tuple_from_result_row are unlikely to
> be correct. Why would this special handling be needed? If we're
> passing the correct fsstate and the correct rel, then I don't
> understand why the previous logic would produce the wrong answer. I
> wonder if this is a holdover from an earlier patch version where
> save_to_tuplestore() was more buggy than it is now. Or maybe it still
> has a bug that we should fix. But I don't think changing
> make_tuple_from_result_row is likely to be the right answer.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-06-26T19:42:14Z

On Tue, Jun 23, 2026 at 5:38 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> I understand your concern and I tried to solve it by passing fsstate now, also saving a backpointer to the node in active_fsstate to solve the issue with make_tuple_from_result_row. Since we need to have conn from fsstate, I am not sure how we can do that if we have only active_fsstate passed to the function.

If fsstate->conn and active_fsstate->conn can be different, I think we
have a big problem. The idea of save_to_tuplestore() is that there's a
query running on the connection already and we have to finish reading
its results before we can use the connection for something else. But
that only makes sense if it's the SAME connection in both cases. If
we're running a connection on connection A, there's no reason we need
to do anything before sending a new query on connection B.

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-06-29T09:27:17Z

On Fri, 26 Jun 2026 at 21:42, Robert Haas <robertmhaas@gmail.com> wrote:

> On Tue, Jun 23, 2026 at 5:38 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > I understand your concern and I tried to solve it by passing fsstate
> now, also saving a backpointer to the node in active_fsstate to solve the
> issue with make_tuple_from_result_row. Since we need to have conn from
> fsstate, I am not sure how we can do that if we have only active_fsstate
> passed to the function.
>
> If fsstate->conn and active_fsstate->conn can be different, I think we
> have a big problem. The idea of save_to_tuplestore() is that there's a
> query running on the connection already and we have to finish reading
> its results before we can use the connection for something else. But
> that only makes sense if it's the SAME connection in both cases. If
> we're running a connection on connection A, there's no reason we need
> to do anything before sending a new query on connection B.
>
> So, do you suggest we can have a backpointer of conn also in
active_fsstate...?

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


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re:Re: Bypassing cursors in postgres_fdw to enable parallel plans

Yilin Zhang <jiezhilove@126.com> — 2026-07-02T07:02:37Z

At 2026-06-29 17:27:17，"Rafia Sabih" <rafia.pghackers@gmail.com>  wrote  ：


>I understand your concern and I tried to solve it by passing fsstate now,
>also saving a backpointer to the node in active_fsstate to solve the issue
>with make_tuple_from_result_row. Since we need to have conn from fsstate, I
>am not sure how we can do that if we have only active_fsstate passed to the
>function.


I have reviewed all your previous patch revisions and tested the v12 patch.
You have fixed the issue raised by Robert where active_scan was not cleared when it ought to have been.
However, in my opinion, active_scan is cleared prematurely before the drain loop finishes executing.
If an error occurs mid-drain (such as query cancellation), the connection remains in an in-flight query state with no active_scan referencing it.
In the subsequent postgresEndForeignScan, is_active_scan() will return false, so pgfdw_cancel_scan() will never be invoked.
In subsequent queries, conn_state->active_scan may become a dangling pointer, triggering a crash.
I have written a test case that reproduces this crash to confirm the bug.










Best regards,

--

Yilin Zhang

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-07-02T17:57:36Z

On Mon, Jun 29, 2026 at 5:27 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> So, do you suggest we can have a backpointer of conn also in active_fsstate...?

I think that fsstate and active_fsstate have to be pointing to the
same conn_state object. Do you think otherwise?

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

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Rafia Sabih <rafia.pghackers@gmail.com> — 2026-07-03T12:51:12Z

On Thu, 2 Jul 2026 at 19:57, Robert Haas <robertmhaas@gmail.com> wrote:

> On Mon, Jun 29, 2026 at 5:27 AM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
> > So, do you suggest we can have a backpointer of conn also in
> active_fsstate...?
>
> I think that fsstate and active_fsstate have to be pointing to the
> same conn_state object. Do you think otherwise?
>
> No, but I don't understand why you think they can be different in the
current implementation.
Currently, as per init_scan fsstate->conn_state->active_scan = fsstate, and
it is never reassigned or anything.
So they cannot have different conn_state.
Am I missing something?

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


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Yilin Zhang <jiezhilove@126.com> — 2026-07-06T06:14:46Z

At 2026-07-03 20:51:12，"Rafia Sabih" <rafia.pghackers@gmail.com>  wrote  ：
>
> No, but I don't understand why you think they can be different in the
> current implementation.
> Currently, as per init_scan fsstate->conn_state->active_scan = fsstate, and
> it is never reassigned or anything.
> So they cannot have different conn_state.
> Am I missing something?
> 


This is the reproduction procedure for the crash caused by clearing active_scan that I mentioned last week.


Data preparation:
  CREATE TABLE local_big (id int, val text);
  CREATE TABLE local_big2 (id int, val text);
  INSERT INTO local_big SELECT i, 'val_' || i FROM generate_series(1, 300000) i;
  INSERT INTO local_big2 SELECT i, 'val_' || i FROM generate_series(1, 300000) i;
  CREATE SERVER test_srv FOREIGN DATA WRAPPER postgres_fdw
      OPTIONS (host 'localhost', port '55436', dbname 'test_bug2');
  CREATE USER MAPPING FOR CURRENT_USER SERVER test_srv;
  CREATE FOREIGN TABLE ft_big (id int, val text)
      SERVER test_srv
      OPTIONS (schema_name 'public', table_name 'local_big',
               streaming_fetch 'true', fetch_size '1');
  CREATE FOREIGN TABLE ft_small (id int, val text)
      SERVER test_srv
      OPTIONS (schema_name 'public', table_name 'local_big2',
               streaming_fetch 'true', fetch_size '1');


session-1


  DROP TABLE IF EXISTS bug2_pid;
  CREATE TEMP TABLE bug2_pid (pid int);
  INSERT INTO bug2_pid VALUES (pg_backend_pid()); -- pid_1


  DO $$
  DECLARE cnt int;
  BEGIN
      SET LOCAL enable_hashjoin = off;
      SET LOCAL enable_mergejoin = off;


      SELECT count(*) INTO cnt
      FROM ft_big b, ft_small s
      WHERE b.id = s.id
        AND pg_backend_pid() > 0;


      RAISE NOTICE 'completed: cnt=%', cnt;


  EXCEPTION WHEN OTHERS THEN
      RAISE NOTICE 'caught: %', SQLERRM;
  END;
  $$;


  SELECT 'post_check' AS phase, count(*) FROM ft_big; -- CRASH HERE


session-2
SELECT pid FROM bug2_pid;
SELECT pg_cancel_backend(pid); -- After Session 1 enters the drain loop, execute pg_cancel_backend(pid) in Session 2.


I believe the issue regarding fsstate->conn and active_fsstate->conn that you've been discussing with Robert is related to this crash.
If only one PgFdwScanState* (active_fsstate) is available inside save_to_tuplestore, the assignment active_scan = NULL will clearly not be executed prior to draining.
The draining logic operates on data from active_fsstate, and we can only clear it after draining completes successfully.


Best regards,

--

Yilin Zhang

Re: Bypassing cursors in postgres_fdw to enable parallel plans

Robert Haas <robertmhaas@gmail.com> — 2026-07-06T18:19:25Z

On Fri, Jul 3, 2026 at 8:51 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> On Thu, 2 Jul 2026 at 19:57, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Jun 29, 2026 at 5:27 AM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>> > So, do you suggest we can have a backpointer of conn also in active_fsstate...?
>>
>> I think that fsstate and active_fsstate have to be pointing to the
>> same conn_state object. Do you think otherwise?
>>
> No, but I don't understand why you think they can be different in the current implementation.
> Currently, as per init_scan fsstate->conn_state->active_scan = fsstate, and it is never reassigned or anything.
> So they cannot have different conn_state.
> Am I missing something?

I don't think they can be different. I think you shouldn't be passing
both of them, because save_to_tuplestore() is buggy if it ever
references fsstate. If you only pass active_fsstate, then that can't
happen.

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