PG19: flat-copied RelOptInfos share extension_state with the original rel

From: Shubhankar Anand Kulkarni <shubhankar(dot)ak(at)zohocorp(dot)com>
To: "pgsql-hackers" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "pgsql-hackers" <pgsql-hackers(at)postgresql(dot)org>
Cc: "robertmhaas" <robertmhaas(at)gmail(dot)com>, "guofenglinux" <guofenglinux(at)gmail(dot)com>
Subject: PG19: flat-copied RelOptInfos share extension_state with the original rel
Date: 2026-10-12 10:41:03
Message-ID: 1a12fb3a08d.429ac7fc62960.6325095284115187647@zohocorp.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi hackers,

While testing the new v19 planner extension APIs with an extension that

keeps per-rel state, I found that RelOptInfos created by flat copy

share their extension_state array with the rel they were copied from.

build_grouped_rel(), create_unique_paths() and

create_partial_unique_paths() memcpy() a RelOptInfo and reset the path

fields, and build_minmax_path() does the same for PlannerInfo, but none

of them resets extension_state / extension_state_allocated.  With

joinrel_setup_hook (4020b370f2) and build_simple_rel_hook (91f33a2ae9),

the natural place for an extension to attach state is right when a rel

is built, and make_join_rel() runs joinrel_setup_hook before

make_grouped_join_rel() copies the joinrel.  So:

1. When join_path_setup_hook or set_join_pathlist_hook is called for

   the grouped joinrel, GetRelOptInfoExtensionState() returns the state

   stored for the plain joinrel, and anything stored for the grouped

   joinrel overwrites the plain joinrel's entry.

2. If the store needs a slot past the end of the array,

   SetRelOptInfoExtensionState() repallocs the shared array.  That

   frees the array the plain joinrel still points to, and the next

   lookup on the plain joinrel reads freed memory.

Eager aggregation is on by default, so a GROUP BY over a two-table join

is enough.  The attached test module stores a pointer to the owning rel

in its state, so it can tell whose state it gets back.  On

REL_19_STABLE (2359968b44) with --enable-cassert, using the tables from

eager_aggregate.sql:

  SET test_extstate.mode = 1;

  EXPLAIN (COSTS OFF)

  SELECT t1.a, avg(t2.c)

    FROM eager_agg_t1 t1 JOIN eager_agg_t2 t2 ON t1.b = t2.b

  GROUP BY t1.a;

  WARNING:  extension state on grouped joinrel (b 1 2) belongs to a different RelOptInfo

  SET test_extstate.mode = 2;    -- also stores into a higher slot

  EXPLAIN ... same query ...

  WARNING:  plain joinrel (b 1 2) still points at array 0x740a72cbfc48, which repalloc just freed; slot 1 now reads 0x7f7f7f7f7f7f7f7f

  server closed the connection unexpectedly.

The crash is in the module's join_path_setup_hook, called from

add_paths_to_joinrel() <- populate_joinrel_with_paths() <- make_join_rel() for the plain joinrel. 

With enable_eager_aggregate = off there are no warnings.

This is the same pattern as 58fca6bdf2 ("Don't pass grouped relations to FDWs"), which cleared the FDW fields in build_grouped_rel() [1];

extension_state needs the same treatment there and at the other copy

sites.  For the unique-ified rel, the intent was that it stays opaque

to extensions [2], but it is passed to the join hooks as outerrel /innerrel, so extensions do see it.

The attached patch resets the two fields at all four copy sites.  The

build_minmax_path() copy is not reachable through any current hook,

because preprocess_minmax_aggregates() runs before anything can attach

state to the parent PlannerInfo, but resetting it there seems right

too.  The patch also adds the "constant string" note that

GetExplainExtensionId() already has to GetPlannerExtensionId(), since

it keeps the caller's pointer rather than copying the name.

With the patch, the reproducer shows no warnings, the plan still has

the Partial HashAggregate below the join, and the core regression

tests pass (installcheck-parallel, 239 tests, cassert + ASan build).

For the eager-deduplication series [3]: its proposed

copy_rel_without_paths() would be a good single place to reset

extension_state, if that lands.

I found this while reviewing the v19 extension-facing planner APIs with

the help of an AI code-review and confirmed it on the build

above.

[1] https://postgr.es/m/CA+Tgmob7iSM9YkRM44VjUDuaCchW-fY54MV5njpTZTL9uNyV4w@mail.gmail.com

[2] https://postgr.es/m/CAMbWs4-UXdxMaZO-GgNx7Y4tWoDutc4uVmnhUBn+8afHp520og@mail.gmail.com

[3] https://postgr.es/m/PH0PR18MB444315CD3E17255AB4A629BEA6AD2@PH0PR18MB4443.namprd18.prod.outlook.com

Regards,

Shubhankar K.

Member Technical Staff

Zoho Corporation

Attachment Content-Type Size
repro.sql application/octet-stream 1.5 KB
test_extstate.c application/octet-stream 4.2 KB
v1-0001-Don-t-let-flat-copied-planner-structures-share-ex.patch application/octet-stream 4.8 KB
asan_crash_report_stock.txt text/plain 4.7 KB

Browse pgsql-hackers by date

  From Date Subject
Previous Message Greg Burd 2026-10-12 10:33:24 Re: Adding basic NUMA awareness