Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid ABI break in ModifyTableState from the FDW pruning fix

  2. Re-index ModifyTable FDW arrays when pruning result relations

  3. Fix universal builds on MacOS

  4. Track unpruned relids to avoid processing pruned relations

  1. BUG #19484: Segmentation fault triggered by FDW

    PG Bug reporting form <noreply@postgresql.org> — 2026-05-18T06:38:34Z

    The following bug has been logged on the website:

Bug reference:      19484
Logged by:          Chi Zhang
Email address:      798604270@qq.com
PostgreSQL version: 18.4
Operating system:   Ubuntu 24.04
Description:        

Hi,

I found the following test case triggers a segmentation fault:

```
\set ON_ERROR_STOP on

CREATE EXTENSION postgres_fdw;

CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (
  host '/path/to/pg_socket',
  port '5432',
  dbname :'dbname'
);

CREATE USER MAPPING FOR postgres SERVER loopback
OPTIONS (user 'postgres');

CREATE SCHEMA r;
CREATE TABLE r.remote_p2 (a int NOT NULL, b int);

CREATE TABLE pt (a int NOT NULL, b int) PARTITION BY LIST (a);
CREATE TABLE pt_p1 PARTITION OF pt FOR VALUES IN (1);
CREATE FOREIGN TABLE pt_p2 PARTITION OF pt FOR VALUES IN (2)
  SERVER loopback
  OPTIONS (schema_name 'r', table_name 'remote_p2');

INSERT INTO pt_p1 VALUES (1, 10);
INSERT INTO r.remote_p2 VALUES (2, 20);

SET plan_cache_mode = force_generic_plan;

PREPARE upd(int) AS
  UPDATE pt
  SET b = b + 1
  WHERE a = $1
  RETURNING tableoid::regclass, a, b;

EXPLAIN (costs off) EXECUTE upd(2);
EXECUTE upd(2);
SELECT * FROM r.remote_p2 ORDER BY a;

```

This is the log:

```
2026-05-18 13:40:41.888 CST [21729] LOG:  database system is ready to accept
connections
  2026-05-18 13:41:03.317 CST [21932] LOG:  unexpected EOF on client
connection with an open transaction
  2026-05-18 13:41:03.317 CST [21729] LOG:  client backend (PID 21931) was
terminated by signal 11: Segmentation fault
  2026-05-18 13:41:03.317 CST [21729] DETAIL:  Failed process was running:
EXECUTE upd(2);
  2026-05-18 13:41:03.317 CST [21729] LOG:  terminating any other active
server processes
  2026-05-18 13:41:03.319 CST [21729] LOG:  all server processes terminated;
reinitializing
  2026-05-18 13:41:03.345 CST [21936] LOG:  database system was interrupted;
last known up at 2026-05-18 13:40:41 CST
  2026-05-18 13:41:03.509 CST [21936] LOG:  database system was not properly
shut down; automatic recovery in progress
  2026-05-18 13:41:03.513 CST [21936] LOG:  redo starts at 0/98371040
  2026-05-18 13:41:03.531 CST [21936] LOG:  invalid record length at
0/987B6E68: expected at least 24, got 0
  2026-05-18 13:41:03.531 CST [21936] LOG:  redo done at 0/987B6E40 system
usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.01 s
  2026-05-18 13:41:03.537 CST [21937] LOG:  checkpoint starting:
end-of-recovery fast wait
  2026-05-18 13:41:03.654 CST [21937] LOG:  checkpoint complete:
end-of-recovery fast wait: wrote 975 buffers (6.0%), wrote 3 SLRU buffers; 0
WAL file(s) added, 0 removed, 0
recycled; write=0.081 s, sync=0.030 s, total=0.121 s; sync files=325,
longest=0.005 s, average=0.001 s; distance=4375 kB, estimate=4375 kB;
lsn=0/987B6E68, redo lsn=0/987B6E68
  2026-05-18 13:41:03.660 CST [21729] LOG:  database system is ready to
accept connections
```

I built the Postgres from source code
901ed9b352b41f034e17bc540725082a488fce31 of github commit.

  2. Re: BUG #19484: Segmentation fault triggered by FDW

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-05-20T12:37:16Z

    Hi,

On Wed, 20 May 2026 at 03:59, PG Bug reporting form <noreply@postgresql.org>
wrote:

> The following bug has been logged on the website:
>
> Bug reference:      19484
> Logged by:          Chi Zhang
> Email address:      798604270@qq.com
> PostgreSQL version: 18.4
> Operating system:   Ubuntu 24.04
> Description:
>
> Hi,
>
> I found the following test case triggers a segmentation fault:
>
> ```
> \set ON_ERROR_STOP on
>
> CREATE EXTENSION postgres_fdw;
>
> CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (
>   host '/path/to/pg_socket',
>   port '5432',
>   dbname :'dbname'
> );
>
> CREATE USER MAPPING FOR postgres SERVER loopback
> OPTIONS (user 'postgres');
>
> CREATE SCHEMA r;
> CREATE TABLE r.remote_p2 (a int NOT NULL, b int);
>
> CREATE TABLE pt (a int NOT NULL, b int) PARTITION BY LIST (a);
> CREATE TABLE pt_p1 PARTITION OF pt FOR VALUES IN (1);
> CREATE FOREIGN TABLE pt_p2 PARTITION OF pt FOR VALUES IN (2)
>   SERVER loopback
>   OPTIONS (schema_name 'r', table_name 'remote_p2');
>
> INSERT INTO pt_p1 VALUES (1, 10);
> INSERT INTO r.remote_p2 VALUES (2, 20);
>
> SET plan_cache_mode = force_generic_plan;
>
> PREPARE upd(int) AS
>   UPDATE pt
>   SET b = b + 1
>   WHERE a = $1
>   RETURNING tableoid::regclass, a, b;
>
> EXPLAIN (costs off) EXECUTE upd(2);
> EXECUTE upd(2);
> SELECT * FROM r.remote_p2 ORDER BY a;
>

Thanks for the very precise repro, that made this easy to track down.

I reproduced the crash on master.  The plan EXPLAIN under
force_generic_plan shows runtime pruning is in effect:

  Update on pt
    Foreign Update on pt_p2 pt_2
    ->  Append
          Subplans Removed: 1
          ->  Foreign Update on pt_p2 pt_2

The SEGV happens inside postgresBeginForeignModify() because
ExecInitModifyTable() builds re-indexed "kept" copies of several
parallel per-result-relation lists after dropping pruned relations -
withCheckOptionLists, returningLists, updateColnosLists,
mergeActionLists and mergeJoinConditions, however two members were
missed:

  - node->fdwPrivLists, read with list_nth(node->fdwPrivLists, i) when
    BeginForeignModify() is called, and
  - node->fdwDirectModifyPlans, checked with bms_is_member(i, ...) when
    setting ri_usesFdwDirectModify.

Both were still indexed against the original (pre-pruning) positions
while the surrounding loop's "i" is now the kept position.  When the
foreign partition's kept-index no longer matched its original index,
BeginForeignModify() got the wrong fdw_private and crashed.

Attached patch builds re-indexed kept copies for these two arrays in
the same loop as the other parallel lists, and uses them at the two
call sites.

Regards,
Ayush

  3. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-05-20T17:46:03Z

    Hi,

On Wed, May 20, 2026 at 5:37 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
> On Wed, 20 May 2026 at 03:59, PG Bug reporting form <noreply@postgresql.org> wrote:
>> I found the following test case triggers a segmentation fault:

[snip]

> Thanks for the very precise repro, that made this easy to track down.
>
> I reproduced the crash on master.  The plan EXPLAIN under
> force_generic_plan shows runtime pruning is in effect:
>
>   Update on pt
>     Foreign Update on pt_p2 pt_2
>     ->  Append
>           Subplans Removed: 1
>           ->  Foreign Update on pt_p2 pt_2
>
> The SEGV happens inside postgresBeginForeignModify() because
> ExecInitModifyTable() builds re-indexed "kept" copies of several
> parallel per-result-relation lists after dropping pruned relations -
> withCheckOptionLists, returningLists, updateColnosLists,
> mergeActionLists and mergeJoinConditions, however two members were
> missed:
>
>   - node->fdwPrivLists, read with list_nth(node->fdwPrivLists, i) when
>     BeginForeignModify() is called, and
>   - node->fdwDirectModifyPlans, checked with bms_is_member(i, ...) when
>     setting ri_usesFdwDirectModify.
>
> Both were still indexed against the original (pre-pruning) positions
> while the surrounding loop's "i" is now the kept position.  When the
> foreign partition's kept-index no longer matched its original index,
> BeginForeignModify() got the wrong fdw_private and crashed.
>
> Attached patch builds re-indexed kept copies for these two arrays in
> the same loop as the other parallel lists, and uses them at the two
> call sites.

Thanks Chi for the report, and Ayush for the analysis and patch!  Will review.

Best regards,
Etsuro Fujita

  4. Re: BUG #19484: Segmentation fault triggered by FDW

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-05-22T20:56:05Z

    On Wed May 20, 2026 at 9:37 AM -03, Ayush Tiwari wrote:
> I reproduced the crash on master.  The plan EXPLAIN under
> force_generic_plan shows runtime pruning is in effect:
>
>   Update on pt
>     Foreign Update on pt_p2 pt_2
>     ->  Append
>           Subplans Removed: 1
>           ->  Foreign Update on pt_p2 pt_2
>
> The SEGV happens inside postgresBeginForeignModify() because
> ExecInitModifyTable() builds re-indexed "kept" copies of several
> parallel per-result-relation lists after dropping pruned relations -
> withCheckOptionLists, returningLists, updateColnosLists,
> mergeActionLists and mergeJoinConditions, however two members were
> missed:
>
>   - node->fdwPrivLists, read with list_nth(node->fdwPrivLists, i) when
>     BeginForeignModify() is called, and
>   - node->fdwDirectModifyPlans, checked with bms_is_member(i, ...) when
>     setting ri_usesFdwDirectModify.
>
> Both were still indexed against the original (pre-pruning) positions
> while the surrounding loop's "i" is now the kept position.  When the
> foreign partition's kept-index no longer matched its original index,
> BeginForeignModify() got the wrong fdw_private and crashed.
>
> Attached patch builds re-indexed kept copies for these two arrays in
> the same loop as the other parallel lists, and uses them at the two
> call sites.
>

Hi, thanks for the patch. This issue started on version 18 by commit
cbc127917e0.

The patch fixes the issue and it make sense to me. One a minor comment
is that I think pg_indent is needed on nodeModifyTable.c

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  5. Re: BUG #19484: Segmentation fault triggered by FDW

    Rafia Sabih <rafia.pghackers@gmail.com> — 2026-05-30T06:18:13Z

    On Fri, 22 May 2026 at 22:56, Matheus Alcantara <matheusssilv97@gmail.com>
wrote:

> On Wed May 20, 2026 at 9:37 AM -03, Ayush Tiwari wrote:
> > I reproduced the crash on master.  The plan EXPLAIN under
> > force_generic_plan shows runtime pruning is in effect:
> >
> >   Update on pt
> >     Foreign Update on pt_p2 pt_2
> >     ->  Append
> >           Subplans Removed: 1
> >           ->  Foreign Update on pt_p2 pt_2
> >
> > The SEGV happens inside postgresBeginForeignModify() because
> > ExecInitModifyTable() builds re-indexed "kept" copies of several
> > parallel per-result-relation lists after dropping pruned relations -
> > withCheckOptionLists, returningLists, updateColnosLists,
> > mergeActionLists and mergeJoinConditions, however two members were
> > missed:
> >
> >   - node->fdwPrivLists, read with list_nth(node->fdwPrivLists, i) when
> >     BeginForeignModify() is called, and
> >   - node->fdwDirectModifyPlans, checked with bms_is_member(i, ...) when
> >     setting ri_usesFdwDirectModify.
> >
> > Both were still indexed against the original (pre-pruning) positions
> > while the surrounding loop's "i" is now the kept position.  When the
> > foreign partition's kept-index no longer matched its original index,
> > BeginForeignModify() got the wrong fdw_private and crashed.
> >
> > Attached patch builds re-indexed kept copies for these two arrays in
> > the same loop as the other parallel lists, and uses them at the two
> > call sites.
> >
>
A good catch. However there is one issue that remains here,
in show_modifytable_info still is using the old index here fdw_private =
(List *) list_nth(node->fdwPrivLists, j) i.e. the one before pruning.
In fact I found a scenario where it is causing crash, try this

create table fdw_part_update2 (a int not null, b int) partition by list (a);
create table fdw_part_update2_p1 partition of fdw_part_update2 for values
in (1);
create table fdw_part_update2_remote (a int not null, b int);
create foreign table fdw_part_update2_p2 partition of fdw_part_update2
    for values in (2)
    server loopback options (table_name 'fdw_part_update2_remote');
insert into fdw_part_update2_p1 values (1, 10);
insert into fdw_part_update2_remote values (2, 20);
set plan_cache_mode = force_generic_plan;
 prepare fdw_part_upd2(int) as
      update fdw_part_update2 set b = b + random()::int * 0 + 1 where a = $1
      returning tableoid::regclass, a, b;
execute fdw_part_upd2(2);
explain (analyze, verbose, costs off, timing off, summary off)
    execute fdw_part_upd2(2);

Please find the attached file for the patch to fix this. This patch applies
over the earlier patch (given by Ayush) in this thread.

>
> Hi, thanks for the patch. This issue started on version 18 by commit
> cbc127917e0.
>
> The patch fixes the issue and it make sense to me. One a minor comment
> is that I think pg_indent is needed on nodeModifyTable.c
>
> --
> Matheus Alcantara
> EDB: https://www.enterprisedb.com
>
>
>

-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

  6. Re: BUG #19484: Segmentation fault triggered by FDW

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-06-09T15:10:42Z

    On Sat May 30, 2026 at 3:18 AM -03, Rafia Sabih wrote:
> A good catch. However there is one issue that remains here,
> in show_modifytable_info still is using the old index here fdw_private =
> (List *) list_nth(node->fdwPrivLists, j) i.e. the one before pruning.
> In fact I found a scenario where it is causing crash, try this
>
> create table fdw_part_update2 (a int not null, b int) partition by list (a);
> create table fdw_part_update2_p1 partition of fdw_part_update2 for values
> in (1);
> create table fdw_part_update2_remote (a int not null, b int);
> create foreign table fdw_part_update2_p2 partition of fdw_part_update2
>     for values in (2)
>     server loopback options (table_name 'fdw_part_update2_remote');
> insert into fdw_part_update2_p1 values (1, 10);
> insert into fdw_part_update2_remote values (2, 20);
> set plan_cache_mode = force_generic_plan;
>  prepare fdw_part_upd2(int) as
>       update fdw_part_update2 set b = b + random()::int * 0 + 1 where a = $1
>       returning tableoid::regclass, a, b;
> execute fdw_part_upd2(2);
> explain (analyze, verbose, costs off, timing off, summary off)
>     execute fdw_part_upd2(2);
>
> Please find the attached file for the patch to fix this. This patch applies
> over the earlier patch (given by Ayush) in this thread.
>

Thanks for catching this, Rafia. The fix is correct —
show_modifytable_info() was indeed still reading from node->fdwPrivLists
using the post-pruning index j, which causes an out-of-bounds access
when partitions are pruned.

I think both patches should be squashed into a single one since they fix
the same underlying issue. I've done this locally and also ran pg_indent
over the result. Attached is the combined patch.

One minor naming observation: the new fdwPrivLists field in
ModifyTableState doesn't follow the mt_ prefix convention used by the
other re-indexed lists (mt_updateColnosLists, mt_mergeActionLists,
mt_mergeJoinConditions). Should we rename it to mt_fdwPrivLists for
consistency?

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  7. Re: BUG #19484: Segmentation fault triggered by FDW

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-06-10T05:15:27Z

    Hi,

On Tue, 9 Jun 2026 at 20:40, Matheus Alcantara <matheusssilv97@gmail.com>
wrote:

>
> I think both patches should be squashed into a single one since they fix
> the same underlying issue. I've done this locally and also ran pg_indent
> over the result. Attached is the combined patch.
>

Thanks for this!


> One minor naming observation: the new fdwPrivLists field in
> ModifyTableState doesn't follow the mt_ prefix convention used by the
> other re-indexed lists (mt_updateColnosLists, mt_mergeActionLists,
> mt_mergeJoinConditions). Should we rename it to mt_fdwPrivLists for
> consistency?
>

I think yes, it makes sense to rename it.

Regards,
Ayush

  8. Re: BUG #19484: Segmentation fault triggered by FDW

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-06-10T11:08:56Z

    On Wed Jun 10, 2026 at 2:15 AM -03, Ayush Tiwari wrote:
>> One minor naming observation: the new fdwPrivLists field in
>> ModifyTableState doesn't follow the mt_ prefix convention used by the
>> other re-indexed lists (mt_updateColnosLists, mt_mergeActionLists,
>> mt_mergeJoinConditions). Should we rename it to mt_fdwPrivLists for
>> consistency?
>>
>
> I think yes, it makes sense to rename it.
>

Attached v2 renamed, thanks.

(Also CC Amit on this since he committed cbc127917e0 which I believe
that is when the issue started)

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  9. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-10T13:09:12Z

    Hi Matheus,

On Wed, Jun 10, 2026 at 8:09 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Jun 10, 2026 at 2:15 AM -03, Ayush Tiwari wrote:
> >> One minor naming observation: the new fdwPrivLists field in
> >> ModifyTableState doesn't follow the mt_ prefix convention used by the
> >> other re-indexed lists (mt_updateColnosLists, mt_mergeActionLists,
> >> mt_mergeJoinConditions). Should we rename it to mt_fdwPrivLists for
> >> consistency?
> >>
> >
> > I think yes, it makes sense to rename it.
> >
>
> Attached v2 renamed, thanks.
>
> (Also CC Amit on this since he committed cbc127917e0 which I believe
> that is when the issue started)

Thanks for adding me.  I'll take a look at this early next week.

-- 
Thanks, Amit Langote

  10. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-10T13:27:22Z

    On Wed, Jun 10, 2026 at 10:09 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Jun 10, 2026 at 8:09 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > On Wed Jun 10, 2026 at 2:15 AM -03, Ayush Tiwari wrote:
> > >> One minor naming observation: the new fdwPrivLists field in
> > >> ModifyTableState doesn't follow the mt_ prefix convention used by the
> > >> other re-indexed lists (mt_updateColnosLists, mt_mergeActionLists,
> > >> mt_mergeJoinConditions). Should we rename it to mt_fdwPrivLists for
> > >> consistency?
> > >>
> > >
> > > I think yes, it makes sense to rename it.
> > >
> >
> > Attached v2 renamed, thanks.
> >
> > (Also CC Amit on this since he committed cbc127917e0 which I believe
> > that is when the issue started)
>
> Thanks for adding me.  I'll take a look at this early next week.

I looked, and the patch seems straightforward enough.

Before committing it, I'd like to wait briefly to see if Fujita-san
has any thoughts on the FDW-side concerns, since he has already chimed
in on the thread.


--
Thanks, Amit Langote

  11. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-11T10:40:49Z

    Amit-san,

On Wed, Jun 10, 2026 at 10:27 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Before committing it, I'd like to wait briefly to see if Fujita-san
> has any thoughts on the FDW-side concerns, since he has already chimed
> in on the thread.

I'll review the patch as early as I can next week, but if you want to
commit it quickly, please go ahead.

Thanks!

Best regards,
Etsuro Fujita

  12. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-11T10:44:20Z

    Hi Fujita-san,

On Thu, Jun 11, 2026 at 7:41 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> Amit-san,
>
> On Wed, Jun 10, 2026 at 10:27 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Before committing it, I'd like to wait briefly to see if Fujita-san
> > has any thoughts on the FDW-side concerns, since he has already chimed
> > in on the thread.
>
> I'll review the patch as early as I can next week, but if you want to
> commit it quickly, please go ahead.

No problem; I'll hold off.

-- 
Thanks, Amit Langote

  13. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-18T09:15:44Z

    Fujita-san,

On Thu, Jun 11, 2026 at 7:44 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Fujita-san,
>
> On Thu, Jun 11, 2026 at 7:41 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >
> > Amit-san,
> >
> > On Wed, Jun 10, 2026 at 10:27 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > Before committing it, I'd like to wait briefly to see if Fujita-san
> > > has any thoughts on the FDW-side concerns, since he has already chimed
> > > in on the thread.
> >
> > I'll review the patch as early as I can next week, but if you want to
> > commit it quickly, please go ahead.
>
> No problem; I'll hold off.

A small clarification on this one: the patch makes no changes to
postgres_fdw code, only adds a regression test. The bug, which I
introduced, is in how nodeModifyTable.c supplies fdw_private from the
pruned/validated fdwPrivLists in ModifyTableState rather than the
original list in ModifyTable, so the postgres_fdw side itself is
unchanged. The substantive change is the executor re-indexing.

Still holding off on committing per our exchange, but no rush given
you may want to prioritize the v19 open items. Happy to go ahead
whenever, or wait for your comments.

-- 
Thanks, Amit Langote

  14. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-18T10:50:59Z

    Amit-san,

On Thu, Jun 18, 2026 at 6:16 PM Amit Langote <amitlangote09@gmail.com> wrote:
> A small clarification on this one: the patch makes no changes to
> postgres_fdw code, only adds a regression test. The bug, which I
> introduced, is in how nodeModifyTable.c supplies fdw_private from the
> pruned/validated fdwPrivLists in ModifyTableState rather than the
> original list in ModifyTable, so the postgres_fdw side itself is
> unchanged. The substantive change is the executor re-indexing.

Thanks for the clarification!

> Still holding off on committing per our exchange, but no rush given
> you may want to prioritize the v19 open items. Happy to go ahead
> whenever, or wait for your comments.

  15. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-18T10:55:05Z

    Sorry, I accidentally sent this before I finished writing.

On Thu, Jun 18, 2026 at 7:50 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> Amit-san,
>
> On Thu, Jun 18, 2026 at 6:16 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > A small clarification on this one: the patch makes no changes to
> > postgres_fdw code, only adds a regression test. The bug, which I
> > introduced, is in how nodeModifyTable.c supplies fdw_private from the
> > pruned/validated fdwPrivLists in ModifyTableState rather than the
> > original list in ModifyTable, so the postgres_fdw side itself is
> > unchanged. The substantive change is the executor re-indexing.
>
> Thanks for the clarification!
>
> > Still holding off on committing per our exchange, but no rush given
> > you may want to prioritize the v19 open items. Happy to go ahead
> > whenever, or wait for your comments.

Actually, I've just started reviewing the patch.  I'll let you know
the results (if any) by tomorrow.  Thanks for the consideration as
well.

Best regards,
Etsuro Fujita

  16. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-18T10:57:07Z

    On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

> Sorry, I accidentally sent this before I finished writing.
>
> On Thu, Jun 18, 2026 at 7:50 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> >
> > Amit-san,
> >
> > On Thu, Jun 18, 2026 at 6:16 PM Amit Langote <amitlangote09@gmail.com>
> wrote:
> > > A small clarification on this one: the patch makes no changes to
> > > postgres_fdw code, only adds a regression test. The bug, which I
> > > introduced, is in how nodeModifyTable.c supplies fdw_private from the
> > > pruned/validated fdwPrivLists in ModifyTableState rather than the
> > > original list in ModifyTable, so the postgres_fdw side itself is
> > > unchanged. The substantive change is the executor re-indexing.
> >
> > Thanks for the clarification!
> >
> > > Still holding off on committing per our exchange, but no rush given
> > > you may want to prioritize the v19 open items. Happy to go ahead
> > > whenever, or wait for your comments.
>
> Actually, I've just started reviewing the patch.  I'll let you know
> the results (if any) by tomorrow.  Thanks for the consideration as
> well.


Ah, thanks for the heads up.

- Amit

>

  17. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-19T11:59:39Z

    Amit-san,

On Thu, Jun 18, 2026 at 7:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> Actually, I've just started reviewing the patch.  I'll let you know
>> the results (if any) by tomorrow.

> Ah, thanks for the heads up.

Here are my review comments:

@@ -5167,6 +5176,15 @@ ExecInitModifyTable(ModifyTable *node, EState
*estate, int eflags)

                returningLists = lappend(returningLists, returningList);
            }
+           if (node->fdwPrivLists)
+           {
+               List       *fdwPrivList = (List *)
list_nth(node->fdwPrivLists, i);
+
+               fdwPrivLists = lappend(fdwPrivLists, fdwPrivList);
+           }

As node->fdwPrivLists is always created, the if-test is useless, so I
removed it like the attached.  Also, this is nitpicking but, in the
attached I moved a comment added above
("fdwPrivLists/fdwDirectModifyPlans are re-indexed to match
resultRelations") to a more appropriate place, and did some minor
editorialization.

+-- Runtime pruning of result relations must keep ModifyTable's per-relation
+-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the kept
+-- resultRelations.  Otherwise BeginForeignModify() reads the wrong
+-- fdw_private and segfaults.

This comment isn't 100% correct, because this issue happens with
DirectModify as well.  I think we could expand the comment to mention
that as well, but the comment is already too much/detailed IMO; I
don't think we add such a comment for a test case, so how about
simplifying the comment like this: "Test that direct modify and
foreign modify work with runtime pruning of result relations (bug
#19484)"

+prepare fdw_part_upd2(int) as
+      update fdw_part_update set b = b + random()::int * 0 + 1 where a = $1
+      returning tableoid::regclass, a, b;
+execute fdw_part_upd2(2);
+explain (analyze, verbose, costs off, timing off, summary off)
+    execute fdw_part_upd2(2);

I think it's expensive to do explain with the analyze option enabled,
just for conforming the plan.  To save cycles, I removed the option.
(As the test case for DirectModify was missing the explain test, I
added it, for consistency.)  The test cases are placed in the "test
tuple routing for foreign-table partitions" section, but they are
basic ones to test writable foreign tables, so I moved them to the
"test writable foreign table stuff" section.

@@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
    int         mt_nrels;       /* number of entries in resultRelInfo[] */
    ResultRelInfo *resultRelInfo;   /* info about target relation(s) */

+   /*
+    * Re-indexed fdw private data lists, aligned with resultRelInfo[] after
+    * pruning
+    */
+   List       *mt_fdwPrivLists;

For v18, I think we should put this at the end of the ModifyTableState
struct, to avoid ABI breakage.

That's it.  Sorry for the delay.

Best regards,
Etsuro Fujita

  18. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-22T08:28:00Z

    Fujita-san,

On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Jun 18, 2026 at 7:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >> Actually, I've just started reviewing the patch.  I'll let you know
> >> the results (if any) by tomorrow.
>
> > Ah, thanks for the heads up.
>
> Here are my review comments:

Thanks a lot for the review.

> @@ -5167,6 +5176,15 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
>
>                 returningLists = lappend(returningLists, returningList);
>             }
> +           if (node->fdwPrivLists)
> +           {
> +               List       *fdwPrivList = (List *)
> list_nth(node->fdwPrivLists, i);
> +
> +               fdwPrivLists = lappend(fdwPrivLists, fdwPrivList);
> +           }
>
> As node->fdwPrivLists is always created, the if-test is useless, so I
> removed it like the attached.  Also, this is nitpicking but, in the
> attached I moved a comment added above
> ("fdwPrivLists/fdwDirectModifyPlans are re-indexed to match
> resultRelations") to a more appropriate place, and did some minor
> editorialization.

Looks good.

> +-- Runtime pruning of result relations must keep ModifyTable's per-relation
> +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the kept
> +-- resultRelations.  Otherwise BeginForeignModify() reads the wrong
> +-- fdw_private and segfaults.
>
> This comment isn't 100% correct, because this issue happens with
> DirectModify as well.  I think we could expand the comment to mention
> that as well, but the comment is already too much/detailed IMO; I
> don't think we add such a comment for a test case, so how about
> simplifying the comment like this: "Test that direct modify and
> foreign modify work with runtime pruning of result relations (bug
> #19484)"

I agree that the details shouldn't leak into test code and have been
trying to follow that principle.  Also, bug numbers shouldn't be
mentioned because one can always use `git blame` to find them.
However, since different committers have different styles, I'm fine
with it; kept in the attached updated patch.

> +prepare fdw_part_upd2(int) as
> +      update fdw_part_update set b = b + random()::int * 0 + 1 where a = $1
> +      returning tableoid::regclass, a, b;
> +execute fdw_part_upd2(2);
> +explain (analyze, verbose, costs off, timing off, summary off)
> +    execute fdw_part_upd2(2);
>
> I think it's expensive to do explain with the analyze option enabled,
> just for conforming the plan.  To save cycles, I removed the option.
> (As the test case for DirectModify was missing the explain test, I
> added it, for consistency.)  The test cases are placed in the "test
> tuple routing for foreign-table partitions" section, but they are
> basic ones to test writable foreign tables, so I moved them to the
> "test writable foreign table stuff" section.

Sounds good.

> @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
>     int         mt_nrels;       /* number of entries in resultRelInfo[] */
>     ResultRelInfo *resultRelInfo;   /* info about target relation(s) */
>
> +   /*
> +    * Re-indexed fdw private data lists, aligned with resultRelInfo[] after
> +    * pruning
> +    */
> +   List       *mt_fdwPrivLists;
>
> For v18, I think we should put this at the end of the ModifyTableState
> struct, to avoid ABI breakage.

Good point on ABI. Rather than placing it differently per branch, how
about putting mt_fdwPrivLists at the end of the struct alongside
mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, which
are the same kind of unpruned-filtered lists? That keeps it ABI-safe
and lets master and 18 share an identical change. I'd fold it into
their existing comment too.

> That's it.  Sorry for the delay.

No problem.

Updated patch attached.

-- 
Thanks, Amit Langote

  19. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-23T08:27:39Z

    Amit-san,

On Mon, Jun 22, 2026 at 5:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

> > +-- Runtime pruning of result relations must keep ModifyTable's per-relation
> > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the kept
> > +-- resultRelations.  Otherwise BeginForeignModify() reads the wrong
> > +-- fdw_private and segfaults.
> >
> > This comment isn't 100% correct, because this issue happens with
> > DirectModify as well.  I think we could expand the comment to mention
> > that as well, but the comment is already too much/detailed IMO; I
> > don't think we add such a comment for a test case, so how about
> > simplifying the comment like this: "Test that direct modify and
> > foreign modify work with runtime pruning of result relations (bug
> > #19484)"
>
> I agree that the details shouldn't leak into test code and have been
> trying to follow that principle.  Also, bug numbers shouldn't be
> mentioned because one can always use `git blame` to find them.
> However, since different committers have different styles, I'm fine
> with it; kept in the attached updated patch.

Thanks!

> > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
> >     int         mt_nrels;       /* number of entries in resultRelInfo[] */
> >     ResultRelInfo *resultRelInfo;   /* info about target relation(s) */
> >
> > +   /*
> > +    * Re-indexed fdw private data lists, aligned with resultRelInfo[] after
> > +    * pruning
> > +    */
> > +   List       *mt_fdwPrivLists;
> >
> > For v18, I think we should put this at the end of the ModifyTableState
> > struct, to avoid ABI breakage.
>
> Good point on ABI. Rather than placing it differently per branch, how
> about putting mt_fdwPrivLists at the end of the struct alongside
> mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, which
> are the same kind of unpruned-filtered lists? That keeps it ABI-safe
> and lets master and 18 share an identical change. I'd fold it into
> their existing comment too.

That's a good idea!  So +1

> Updated patch attached.

LGTM.

Best regards,
Etsuro Fujita

  20. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T00:00:59Z

    On Tue, Jun 23, 2026 at 5:27 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Mon, Jun 22, 2026 at 5:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> > > +-- Runtime pruning of result relations must keep ModifyTable's per-relation
> > > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the kept
> > > +-- resultRelations.  Otherwise BeginForeignModify() reads the wrong
> > > +-- fdw_private and segfaults.
> > >
> > > This comment isn't 100% correct, because this issue happens with
> > > DirectModify as well.  I think we could expand the comment to mention
> > > that as well, but the comment is already too much/detailed IMO; I
> > > don't think we add such a comment for a test case, so how about
> > > simplifying the comment like this: "Test that direct modify and
> > > foreign modify work with runtime pruning of result relations (bug
> > > #19484)"
> >
> > I agree that the details shouldn't leak into test code and have been
> > trying to follow that principle.  Also, bug numbers shouldn't be
> > mentioned because one can always use `git blame` to find them.
> > However, since different committers have different styles, I'm fine
> > with it; kept in the attached updated patch.
>
> Thanks!
>
> > > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
> > >     int         mt_nrels;       /* number of entries in resultRelInfo[] */
> > >     ResultRelInfo *resultRelInfo;   /* info about target relation(s) */
> > >
> > > +   /*
> > > +    * Re-indexed fdw private data lists, aligned with resultRelInfo[] after
> > > +    * pruning
> > > +    */
> > > +   List       *mt_fdwPrivLists;
> > >
> > > For v18, I think we should put this at the end of the ModifyTableState
> > > struct, to avoid ABI breakage.
> >
> > Good point on ABI. Rather than placing it differently per branch, how
> > about putting mt_fdwPrivLists at the end of the struct alongside
> > mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, which
> > are the same kind of unpruned-filtered lists? That keeps it ABI-safe
> > and lets master and 18 share an identical change. I'd fold it into
> > their existing comment too.
>
> That's a good idea!  So +1
>
> > Updated patch attached.
>
> LGTM.

Pushed, thanks for checking.


--
Thanks, Amit Langote

  21. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T01:43:32Z

    On Wed, Jun 24, 2026 at 9:00 Amit Langote <amitlangote09@gmail.com> wrote:

> On Tue, Jun 23, 2026 at 5:27 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> > On Mon, Jun 22, 2026 at 5:28 PM Amit Langote <amitlangote09@gmail.com>
> wrote:
> > > On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
> >
> > > > +-- Runtime pruning of result relations must keep ModifyTable's
> per-relation
> > > > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the
> kept
> > > > +-- resultRelations.  Otherwise BeginForeignModify() reads the wrong
> > > > +-- fdw_private and segfaults.
> > > >
> > > > This comment isn't 100% correct, because this issue happens with
> > > > DirectModify as well.  I think we could expand the comment to mention
> > > > that as well, but the comment is already too much/detailed IMO; I
> > > > don't think we add such a comment for a test case, so how about
> > > > simplifying the comment like this: "Test that direct modify and
> > > > foreign modify work with runtime pruning of result relations (bug
> > > > #19484)"
> > >
> > > I agree that the details shouldn't leak into test code and have been
> > > trying to follow that principle.  Also, bug numbers shouldn't be
> > > mentioned because one can always use `git blame` to find them.
> > > However, since different committers have different styles, I'm fine
> > > with it; kept in the attached updated patch.
> >
> > Thanks!
> >
> > > > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
> > > >     int         mt_nrels;       /* number of entries in
> resultRelInfo[] */
> > > >     ResultRelInfo *resultRelInfo;   /* info about target relation(s)
> */
> > > >
> > > > +   /*
> > > > +    * Re-indexed fdw private data lists, aligned with
> resultRelInfo[] after
> > > > +    * pruning
> > > > +    */
> > > > +   List       *mt_fdwPrivLists;
> > > >
> > > > For v18, I think we should put this at the end of the
> ModifyTableState
> > > > struct, to avoid ABI breakage.
> > >
> > > Good point on ABI. Rather than placing it differently per branch, how
> > > about putting mt_fdwPrivLists at the end of the struct alongside
> > > mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, which
> > > are the same kind of unpruned-filtered lists? That keeps it ABI-safe
> > > and lets master and 18 share an identical change. I'd fold it into
> > > their existing comment too.
> >
> > That's a good idea!  So +1
> >
> > > Updated patch attached.
> >
> > LGTM.
>
> Pushed, thanks for checking.


crake is now red on REL_18:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2026-06-24%2000%3A02%3A03

It reports sizeof(ModifyTableState) grew by 8 bytes.

That’s from adding mt_fdwPrivLists at the end of the struct. Placing it
last keeps existing field offsets stable, which is what extensions reading
the node rely on, but it does grow the struct, which is what the checker
flags. I believe we’ve added trailing struct members in back branches
before for exactly this reason. Is a flagged sizeof increase acceptable
here under that precedent, or do we now want to avoid any flagged ABI
change on a back branch?

If we’d rather avoid it, I have a no-field alternative for REL_18:
show_modifytable_info() is the only reader of the re-indexed list, and it
can recompute the mapping from node->resultRelations and
node->fdwPrivLists, both plan-ordered and untouched by pruning. master
would keep the field as committed, so the branches would differ slightly.

I’m inclined toward the no-field version for REL_18 to keep the farm green,
unless someone would rather keep the same code in both branches and accept
the size change. I’ll hold off briefly for any thoughts before doing that.
- Amit

>

  22. Re: BUG #19484: Segmentation fault triggered by FDW

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-06-24T01:53:55Z

    Amit Langote <amitlangote09@gmail.com> writes:
> crake is now red on REL_18:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2026-06-24%2000%3A02%3A03

> It reports sizeof(ModifyTableState) grew by 8 bytes.

> That’s from adding mt_fdwPrivLists at the end of the struct. Placing it
> last keeps existing field offsets stable, which is what extensions reading
> the node rely on, but it does grow the struct, which is what the checker
> flags. I believe we’ve added trailing struct members in back branches
> before for exactly this reason. Is a flagged sizeof increase acceptable
> here under that precedent, or do we now want to avoid any flagged ABI
> change on a back branch?

I don't believe that the addition of ABI checking was meant to change
our back-patching policies, just to make sure we don't make
unintentional ABI changes.  As you say, we've added trailing fields
before.  The critical question is whether it's likely that any
extensions create their own ModifyTableState nodes (and might make
them too small and/or fail to fill the new field correctly).

I was about to say that that seems unlikely to me, but a check of
Debian Code Search immediately found pg_rewrite doing it.  So yeah,
looks like we'd better adopt the no-ABI-break solution.

			regards, tom lane

  23. Re: BUG #19484: Segmentation fault triggered by FDW

    Richard Guo <guofenglinux@gmail.com> — 2026-06-24T02:23:12Z

    On Wed, Jun 24, 2026 at 10:43 AM Amit Langote <amitlangote09@gmail.com> wrote:
> If we’d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning.

This sounds like a nice solution.  Since show_modifytable_info() is
only used for EXPLAIN, I guess the recompute overhead on the
re-indexed list should be fine.

- Richard

  24. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T03:50:28Z

    On Wed, Jun 24, 2026 at 10:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > crake is now red on REL_18:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2026-06-24%2000%3A02%3A03
>
> > It reports sizeof(ModifyTableState) grew by 8 bytes.
>
> > That’s from adding mt_fdwPrivLists at the end of the struct. Placing it
> > last keeps existing field offsets stable, which is what extensions reading
> > the node rely on, but it does grow the struct, which is what the checker
> > flags. I believe we’ve added trailing struct members in back branches
> > before for exactly this reason. Is a flagged sizeof increase acceptable
> > here under that precedent, or do we now want to avoid any flagged ABI
> > change on a back branch?
>
> I don't believe that the addition of ABI checking was meant to change
> our back-patching policies, just to make sure we don't make
> unintentional ABI changes.  As you say, we've added trailing fields
> before.  The critical question is whether it's likely that any
> extensions create their own ModifyTableState nodes (and might make
> them too small and/or fail to fill the new field correctly).
>
> I was about to say that that seems unlikely to me, but a check of
> Debian Code Search immediately found pg_rewrite doing it.  So yeah,
> looks like we'd better adopt the no-ABI-break solution.

Ah, ok, will post the patch to do so in the REL_18_STABLE branch.

Thanks for the comment.

-- 
Thanks, Amit Langote

  25. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T04:25:16Z

    On Wed, Jun 24, 2026 at 11:23 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 10:43 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > If we’d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning.
>
> This sounds like a nice solution.  Since show_modifytable_info() is
> only used for EXPLAIN, I guess the recompute overhead on the
> re-indexed list should be fine.

Thanks for chiming in, here is the patch to do so.

--
Thanks, Amit Langote

  26. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-24T09:15:25Z

    Amit-san,

On Wed, Jun 24, 2026 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 11:23 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > On Wed, Jun 24, 2026 at 10:43 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > If we’d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning.
> >
> > This sounds like a nice solution.  Since show_modifytable_info() is
> > only used for EXPLAIN, I guess the recompute overhead on the
> > re-indexed list should be fine.
>
> Thanks for chiming in, here is the patch to do so.

Thanks for the patch!

+           /*
+            * node->fdwPrivLists is indexed by the original, pre-pruning
+            * result relation order and is parallel to node->resultRelations.
+            * Initial pruning may have dropped earlier relations, so the kept
+            * index j need not match the original position; find this
+            * relation's entry by its range table index instead.
+            */
+           forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
+           {
+               if (lfirst_int(lc1) == (int) rti)
+               {
+                   fdw_private = (List *) lfirst(lc2);
+                   break;
+               }
+           }

I think it's good to skip this for efficiency, when there are no
pruned result relations.

Other than that the patch looks good to me.

I didn't know pg_rewrite's use of ModifyTableState, which was
different than I expected.  Sorry for that.

Best regards,
Etsuro Fujita

  27. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T11:20:29Z

    Fujita-san,

On Wed, Jun 24, 2026 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 1:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Jun 24, 2026 at 11:23 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > > On Wed, Jun 24, 2026 at 10:43 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > If we’d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning.
> > >
> > > This sounds like a nice solution.  Since show_modifytable_info() is
> > > only used for EXPLAIN, I guess the recompute overhead on the
> > > re-indexed list should be fine.
> >
> > Thanks for chiming in, here is the patch to do so.
>
> Thanks for the patch!
>
> +           /*
> +            * node->fdwPrivLists is indexed by the original, pre-pruning
> +            * result relation order and is parallel to node->resultRelations.
> +            * Initial pruning may have dropped earlier relations, so the kept
> +            * index j need not match the original position; find this
> +            * relation's entry by its range table index instead.
> +            */
> +           forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> +           {
> +               if (lfirst_int(lc1) == (int) rti)
> +               {
> +                   fdw_private = (List *) lfirst(lc2);
> +                   break;
> +               }
> +           }
>
> I think it's good to skip this for efficiency, when there are no
> pruned result relations.

Thanks for the review. v2 attached implements your suggestion: it
indexes node->fdwPrivLists directly with j when no result relations
were pruned, and only falls back to matching by range table index when
pruning appears to have dropped some.

> Other than that the patch looks good to me.
>
> I didn't know pg_rewrite's use of ModifyTableState, which was
> different than I expected.  Sorry for that.

No need to apologize at all. Tom's point about it was new to me too.

I will push v2 to REL_18 tomorrow barring objections.

--
Thanks, Amit Langote

  28. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-24T17:02:41Z

    Amit-san,

On Wed, Jun 24, 2026 at 8:20 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > +           /*
> > +            * node->fdwPrivLists is indexed by the original, pre-pruning
> > +            * result relation order and is parallel to node->resultRelations.
> > +            * Initial pruning may have dropped earlier relations, so the kept
> > +            * index j need not match the original position; find this
> > +            * relation's entry by its range table index instead.
> > +            */
> > +           forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > +           {
> > +               if (lfirst_int(lc1) == (int) rti)
> > +               {
> > +                   fdw_private = (List *) lfirst(lc2);
> > +                   break;
> > +               }
> > +           }
> >
> > I think it's good to skip this for efficiency, when there are no
> > pruned result relations.
>
> Thanks for the review. v2 attached implements your suggestion: it
> indexes node->fdwPrivLists directly with j when no result relations
> were pruned, and only falls back to matching by range table index when
> pruning appears to have dropped some.

Thank you for doing that work!

This might be nitpicking, but:

+           if (list_length(node->resultRelations) == mtstate->mt_nrels)
+               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
+           else
+           {
+               Index       rti = resultRelInfo->ri_RangeTableIndex;
+               ListCell   *lc1;
+               ListCell   *lc2;
+
+               fdw_private = NIL;
+               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
+               {
+                   if (lfirst_int(lc1) == (int) rti)
+                   {
+                       fdw_private = (List *) lfirst(lc2);
+                       break;
+                   }
+               }
+           }

I'd put the if-test outside of the outer loop to save cycles.

Other than that v2 looks good to me.

(The forboth loop actually causes an n-squared calculation, but it's
done only when pruning occurs, in which case the number of remaining
result relations would be reduced, so that wouldn't be a problem.)

Best regards,
Etsuro Fujita

  29. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-24T23:24:08Z

    Fujita-san,

On Thu, Jun 25, 2026 at 2:02 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Jun 24, 2026 at 8:20 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Jun 24, 2026 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > > +           /*
> > > +            * node->fdwPrivLists is indexed by the original, pre-pruning
> > > +            * result relation order and is parallel to node->resultRelations.
> > > +            * Initial pruning may have dropped earlier relations, so the kept
> > > +            * index j need not match the original position; find this
> > > +            * relation's entry by its range table index instead.
> > > +            */
> > > +           forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > > +           {
> > > +               if (lfirst_int(lc1) == (int) rti)
> > > +               {
> > > +                   fdw_private = (List *) lfirst(lc2);
> > > +                   break;
> > > +               }
> > > +           }
> > >
> > > I think it's good to skip this for efficiency, when there are no
> > > pruned result relations.
> >
> > Thanks for the review. v2 attached implements your suggestion: it
> > indexes node->fdwPrivLists directly with j when no result relations
> > were pruned, and only falls back to matching by range table index when
> > pruning appears to have dropped some.
>
> Thank you for doing that work!
>
> This might be nitpicking, but:
>
> +           if (list_length(node->resultRelations) == mtstate->mt_nrels)
> +               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
> +           else
> +           {
> +               Index       rti = resultRelInfo->ri_RangeTableIndex;
> +               ListCell   *lc1;
> +               ListCell   *lc2;
> +
> +               fdw_private = NIL;
> +               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> +               {
> +                   if (lfirst_int(lc1) == (int) rti)
> +                   {
> +                       fdw_private = (List *) lfirst(lc2);
> +                       break;
> +                   }
> +               }
> +           }
>
> I'd put the if-test outside of the outer loop to save cycles.

Right, it's loop-invariant. v3 attached computes a boolean
(nopruning), like labeltargets, once before the loop and uses it
inside.

> Other than that v2 looks good to me.
>
> (The forboth loop actually causes an n-squared calculation, but it's
> done only when pruning occurs, in which case the number of remaining
> result relations would be reduced, so that wouldn't be a problem.)

Right, though strictly the inner forboth scans node->resultRelations,
which pruning leaves at its original length, so it's the original
relation count that bounds the scan rather than the reduced one.
Either way it's EXPLAIN-only with small counts, so it's not a concern.

Thanks again for the review.

-- 
Thanks, Amit Langote

  30. Re: BUG #19484: Segmentation fault triggered by FDW

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2026-06-25T06:15:22Z

    Amit-san,

On Thu, Jun 25, 2026 at 8:24 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 2:02 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > This might be nitpicking, but:
> >
> > +           if (list_length(node->resultRelations) == mtstate->mt_nrels)
> > +               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
> > +           else
> > +           {
> > +               Index       rti = resultRelInfo->ri_RangeTableIndex;
> > +               ListCell   *lc1;
> > +               ListCell   *lc2;
> > +
> > +               fdw_private = NIL;
> > +               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > +               {
> > +                   if (lfirst_int(lc1) == (int) rti)
> > +                   {
> > +                       fdw_private = (List *) lfirst(lc2);
> > +                       break;
> > +                   }
> > +               }
> > +           }
> >
> > I'd put the if-test outside of the outer loop to save cycles.
>
> Right, it's loop-invariant. v3 attached computes a boolean
> (nopruning), like labeltargets, once before the loop and uses it
> inside.
>
> > Other than that v2 looks good to me.
> >
> > (The forboth loop actually causes an n-squared calculation, but it's
> > done only when pruning occurs, in which case the number of remaining
> > result relations would be reduced, so that wouldn't be a problem.)
>
> Right, though strictly the inner forboth scans node->resultRelations,
> which pruning leaves at its original length, so it's the original
> relation count that bounds the scan rather than the reduced one.
> Either way it's EXPLAIN-only with small counts, so it's not a concern.

That's right.

The v3 patch looks good to me.  Thanks for updating the patch!

Best regards,
Etsuro Fujita

  31. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-25T11:47:22Z

    Fujita-san,

On Thu, Jun 25, 2026 at 3:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 8:24 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Jun 25, 2026 at 2:02 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > > This might be nitpicking, but:
> > >
> > > +           if (list_length(node->resultRelations) == mtstate->mt_nrels)
> > > +               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
> > > +           else
> > > +           {
> > > +               Index       rti = resultRelInfo->ri_RangeTableIndex;
> > > +               ListCell   *lc1;
> > > +               ListCell   *lc2;
> > > +
> > > +               fdw_private = NIL;
> > > +               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > > +               {
> > > +                   if (lfirst_int(lc1) == (int) rti)
> > > +                   {
> > > +                       fdw_private = (List *) lfirst(lc2);
> > > +                       break;
> > > +                   }
> > > +               }
> > > +           }
> > >
> > > I'd put the if-test outside of the outer loop to save cycles.
> >
> > Right, it's loop-invariant. v3 attached computes a boolean
> > (nopruning), like labeltargets, once before the loop and uses it
> > inside.
> >
> > > Other than that v2 looks good to me.
> > >
> > > (The forboth loop actually causes an n-squared calculation, but it's
> > > done only when pruning occurs, in which case the number of remaining
> > > result relations would be reduced, so that wouldn't be a problem.)
> >
> > Right, though strictly the inner forboth scans node->resultRelations,
> > which pruning leaves at its original length, so it's the original
> > relation count that bounds the scan rather than the reduced one.
> > Either way it's EXPLAIN-only with small counts, so it's not a concern.
>
> That's right.
>
> The v3 patch looks good to me.  Thanks for updating the patch!

Pushed, thanks for checking.

-- 
Thanks, Amit Langote

  32. Re: BUG #19484: Segmentation fault triggered by FDW

    amit <amitlangote09@gmail.com> — 2026-06-25T12:21:15Z

    On Thu, Jun 25, 2026 at 8:47 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 3:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Thu, Jun 25, 2026 at 8:24 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Thu, Jun 25, 2026 at 2:02 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > > > This might be nitpicking, but:
> > > >
> > > > +           if (list_length(node->resultRelations) == mtstate->mt_nrels)
> > > > +               fdw_private = (List *) list_nth(node->fdwPrivLists, j);
> > > > +           else
> > > > +           {
> > > > +               Index       rti = resultRelInfo->ri_RangeTableIndex;
> > > > +               ListCell   *lc1;
> > > > +               ListCell   *lc2;
> > > > +
> > > > +               fdw_private = NIL;
> > > > +               forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> > > > +               {
> > > > +                   if (lfirst_int(lc1) == (int) rti)
> > > > +                   {
> > > > +                       fdw_private = (List *) lfirst(lc2);
> > > > +                       break;
> > > > +                   }
> > > > +               }
> > > > +           }
> > > >
> > > > I'd put the if-test outside of the outer loop to save cycles.
> > >
> > > Right, it's loop-invariant. v3 attached computes a boolean
> > > (nopruning), like labeltargets, once before the loop and uses it
> > > inside.
> > >
> > > > Other than that v2 looks good to me.
> > > >
> > > > (The forboth loop actually causes an n-squared calculation, but it's
> > > > done only when pruning occurs, in which case the number of remaining
> > > > result relations would be reduced, so that wouldn't be a problem.)
> > >
> > > Right, though strictly the inner forboth scans node->resultRelations,
> > > which pruning leaves at its original length, so it's the original
> > > relation count that bounds the scan rather than the reduced one.
> > > Either way it's EXPLAIN-only with small counts, so it's not a concern.
> >
> > That's right.
> >
> > The v3 patch looks good to me.  Thanks for updating the patch!
>
> Pushed, thanks for checking.

And crake turns green:

https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=crake&br=REL_18_STABLE

-- 
Thanks, Amit Langote