| 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 |
| From | Date | Subject | |
|---|---|---|---|
| Previous Message | Greg Burd | 2026-10-12 10:33:24 | Re: Adding basic NUMA awareness |