Re: Bypassing cursors in postgres_fdw to enable parallel plans

From: Rafia Sabih <rafia(dot)pghackers(at)gmail(dot)com>
To: Yilin Zhang <jiezhilove(at)126(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, KENAN YILMAZ <kenan(dot)yilmaz(at)localus(dot)com(dot)tr>, Andy Fan <zhihuifan1213(at)163(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Bypassing cursors in postgres_fdw to enable parallel plans
Date: 2026-07-15 11:50:32
Message-ID: CA+FpmFfw6GQhmY_o+AUqc+ArRfx=iNcpVpWF=dTjGKKThzZc=Q@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, 6 Jul 2026 at 11:45, Yilin Zhang <jiezhilove(at)126(dot)com> wrote:

> At 2026-07-03 20:51:12,"Rafia Sabih" <rafia(dot)pghackers(at)gmail(dot)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.
>
> Hello Yilin,
Thanks for your input. But unfortunately I am not able to reproduce the
issue with this test case. Also, I am not able to understand it much, as to
how we are going to access bug2_pid from session 2, because it is a temp
table from a different session. Maybe you can simplify this a little more,
or maybe share the backtrace in case of the crash.

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.
>
> Yes you are right, this was the gap, in case of crash while draining,
active_fsstate couldn't be cleared and a dangling pointer was left, which
causes the crash on the next execution.
To fix this, I have now added a condition in pgfdw_abort_cleanup, to reset
the active_scan in case of unfinished drain. Since I was not able to use
your test case so I couldn't verify if this fixes the issue.
Apart from that I have now changed the save_to_tuplestore function to only
have active_fsstate and PGconn. Also, there is a wrapper function to call
it and check the relevant condition so we don't duplicate it all those 5
places. I have also added the calls to function to drain tuples to
tuplestore in execute_dml_stmt and exectute_foreign_modify, to ensure that
whenever a new query is sent on the connection, we perform the draining of
the tuples to tuplestore if any or mark the other scan completed
appropriately.

Please find the rebased patches attached.

> Best regards,
> --
> Yilin Zhang
>
>
>

--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

Attachment Content-Type Size
v13-0001-postgres_fdw-Rename-cursor_exists-flag-to-scan_i.patch application/octet-stream 4.0 KB
v13-0002-postgres_fdw-Add-streaming_fetch-option-for-curs.patch application/octet-stream 123.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2026-07-15 11:53:05 Re: sequencesync worker race with REFRESH SEQUENCES
Previous Message Hayato Kuroda (Fujitsu) 2026-07-15 11:40:08 RE: Fix publisher-side sequence permission reporting