Re: Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) - Mailing list pgsql-hackers
| From | Trakshan Mishra |
|---|---|
| Subject | Re: Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) |
| Date | |
| Msg-id | CACRpqq9dRAV0AOBjmknYVRErS09=ViFVTWoKD_ZikA1t6hotPQ@mail.gmail.com Whole thread |
| In response to | Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) (Lucas Jeffrey <lucas.jeffrey@anachronics.com>) |
| List | pgsql-hackers |
Hi Lucas,
Thanks for v6. I retested it, with most of the attention on the new
deferred-free path, since that is the part that changed.
Test environment:
master @ 1b5dd3a24a4, plus v6-0001 and v6-0002
Linux x86_64, gcc 15.2.0
meson, --buildtype=debug -Dcassert=true
== Testing ==
Both patches apply cleanly with no whitespace errors, and the build has
no warnings.
Full "meson test" with v6: 362 ok, 0 failed, 50 skipped.
With only v6-0001 applied, the new test still segfaults the backend on
the cascade, and foreign_key plus the tests after it fail (52 in
total), as with v5.
As last time, I added temporary LOG lines where a plan is deferred,
left pending after an abort, and freed, to see which path frees each
plan:
- The DO block in the test: 12 plans deferred, 10 left pending by
the aborted executions. 9 of those are freed by the next
iteration's RI query and the last one by AtEOXact_RI() at commit.
The other 2 are freed when their last execution finishes normally.
- Top-level abort, 50 times, then an invalidation and a successful
cascade: all 50 pending plans are freed by AtEOXact_RI() on the
abort path. No crash and no assertion failure.
- SAVEPOINT / ROLLBACK TO, then COMMIT with no further RI query: the
plan is freed at commit, and the Assert that the table is empty
holds.
- New for v6: a case where a pending plan is freed by the hash scan
while another RI execution is still running. An outer ON DELETE
CASCADE on a different FK fires a BEFORE DELETE trigger that runs
the failing self-referencing cascade three times, each inside an
exception block. The scan sees two entries, skips the outer plan
(refcount 1), and frees the pending one; the third pending plan
goes at AtEOXact_RI(). The outer delete completes and the inner
deletes are rolled back as expected. So the refcount > 0 branch
and removing the current entry during hash_seq_search() both get
exercised, and nothing complained.
== Comments ==
All three of Tomas's points look addressed to me. Scanning the
refcount table instead of keeping a separate list is simpler, and the
early return on an empty table keeps the common path cheap. Two small
things:
1. The comment above RI_QueryPlanCacheExecutingRefCountEntry has a line
that is 91 characters long ("error, until the next RI query execution
or the end of the transaction. A plan is never"); it probably just
needs rewrapping after the rename.
2. The comment on ri_PreparedPlanFreeDeferred() says the table is empty
outside of nested RI checks. It can also hold pending entries after an
aborted execution, until the next ri_PerformCheck() or the end of the
transaction, which your mail describes correctly. Maybe worth saying
the same in the comment.
Doing master first and a separate, smaller fix for 14-18 sounds
reasonable to me.
Regards,
Trakshan Mishra
Thanks for v6. I retested it, with most of the attention on the new
deferred-free path, since that is the part that changed.
Test environment:
master @ 1b5dd3a24a4, plus v6-0001 and v6-0002
Linux x86_64, gcc 15.2.0
meson, --buildtype=debug -Dcassert=true
== Testing ==
Both patches apply cleanly with no whitespace errors, and the build has
no warnings.
Full "meson test" with v6: 362 ok, 0 failed, 50 skipped.
With only v6-0001 applied, the new test still segfaults the backend on
the cascade, and foreign_key plus the tests after it fail (52 in
total), as with v5.
As last time, I added temporary LOG lines where a plan is deferred,
left pending after an abort, and freed, to see which path frees each
plan:
- The DO block in the test: 12 plans deferred, 10 left pending by
the aborted executions. 9 of those are freed by the next
iteration's RI query and the last one by AtEOXact_RI() at commit.
The other 2 are freed when their last execution finishes normally.
- Top-level abort, 50 times, then an invalidation and a successful
cascade: all 50 pending plans are freed by AtEOXact_RI() on the
abort path. No crash and no assertion failure.
- SAVEPOINT / ROLLBACK TO, then COMMIT with no further RI query: the
plan is freed at commit, and the Assert that the table is empty
holds.
- New for v6: a case where a pending plan is freed by the hash scan
while another RI execution is still running. An outer ON DELETE
CASCADE on a different FK fires a BEFORE DELETE trigger that runs
the failing self-referencing cascade three times, each inside an
exception block. The scan sees two entries, skips the outer plan
(refcount 1), and frees the pending one; the third pending plan
goes at AtEOXact_RI(). The outer delete completes and the inner
deletes are rolled back as expected. So the refcount > 0 branch
and removing the current entry during hash_seq_search() both get
exercised, and nothing complained.
== Comments ==
All three of Tomas's points look addressed to me. Scanning the
refcount table instead of keeping a separate list is simpler, and the
early return on an empty table keeps the common path cheap. Two small
things:
1. The comment above RI_QueryPlanCacheExecutingRefCountEntry has a line
that is 91 characters long ("error, until the next RI query execution
or the end of the transaction. A plan is never"); it probably just
needs rewrapping after the rename.
2. The comment on ri_PreparedPlanFreeDeferred() says the table is empty
outside of nested RI checks. It can also hold pending entries after an
aborted execution, until the next ri_PerformCheck() or the end of the
transaction, which your mail describes correctly. Maybe worth saying
the same in the comment.
Doing master first and a separate, smaller fix for 14-18 sounds
reasonable to me.
Regards,
Trakshan Mishra
On Fri, Oct 09, 2026 12:56 AM, Lucas Jeffrey <lucas.jeffrey@anachronics.com> wrote:
Hi Tomas,
Thanks for your review. I attached a v6, rebased on current master and
with some changes handling some items in your review.
1) Agreed on the naming, there were three different terms for the same
thing. I used "free" everywhere, but since the plan is not always freed
immediately, the names now say that explicitly:
ri_PreparedPlanReleaseASAP() -> ri_PreparedPlanFreeOrDefer()
markedForDeletion -> freeDeferred
ri_PreparedPlanFreeUnused() -> ri_PreparedPlanFreeDeferred()
The comments and the commit message were updated to use the same
terms. The only remaining "Release" is ResOwnerReleaseRIPlanExecution(),
which follows the naming convention of the other resource owner
callbacks (ResOwnerReleaseBuffer(), ResOwnerReleaseCachedPlan(), etc.),
so I kept it.
2) Agreed, the "free later list" was not necessary, and I removed it. To beprecise, those plans are no longer in ri_query_cache, since
ri_FetchPreparedPlan() removes them from it, but their entries remain in
the refcount hash table, ri_query_plan_cache_executing_refcount, with a
zero count. ri_PreparedPlanFreeDeferred() now scans that table instead.
That table only has entries while RI queries are being executed, so it
stays small. However, ri_PreparedPlanFreeDeferred() is called on every
ri_PerformCheck(), so it returns immediately when the table is empty,
which is the case unless an RI query is already being executed or a
deferred plan is pending. This way the common path does not pay for a
hash_seq_search() over all the buckets. I also reduced the initial size
of the table to 16, instead of RI_INIT_QUERYHASHSIZE (256).
3) Yes, all supported branches are affected. However, this fix cannot be
backpatched as is: the ResourceOwnerDesc API exists only since 17, and
AtEOXact_RI() is new in 19. I would propose to agree on the fix for
master first, and then submit a separate, less invasive patch for 14-18,
using only the APIs and functions available in those branches.
Regards,
Lucas JeffreyEl mar, 6 oct 2026 a las 15:26, Tomas Vondra (<tomas@vondra.me>) escribió:Hi,
On 10/5/26 16:31, Lucas Jeffrey wrote:
> Hi hackers,
> I'll attach a new v5 version of my patch, I rebased so my patches can
> now apply cleanly in master.
> Reviews are welcomed.
> Regards,
> Lucas Jeffrey
>
I accidentally ran into this issue a couple days ago, unaware of this
thread. I really like the approach of the proposed fix, refcounting
seems like a sound approach and we already useit elsewhere to track
similar objects. I've only done a quick revie, but it seems much cleaner
than my PoC fix.
A couple (very cosmetic) review comments:
1) The naming seems a bit inconsistent
static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);
Why release vs. free? And the flag is called markedForDeletion, so
that's a third term. Also, I wouldn't use the ASAP in function name.
The SPI function is called SPI_freeplan, so what about using "free"
everywhere?
static void ri_PreparedPlanFree(SPIPlanPtr plan);
static void ri_PreparedPlanFreeUnused(void);
Although, I now realize we're not necessarily freeing the plan, so maybe
this is not an improvement ...
2) Is ri_plans_to_free necessary?
IIUC those plans are still in the hashtable, right? Why not to simply
scan the hashtable, and free the plans there? That'd mean we don't need
a separate list that can get out of sync. Maybe we expect the hash table
to be large, in which case this could be expensive. But is it? I don't
see any such argument in this thread.
If the list is necessary, maybe ri_PreparedPlanFreeUnused() should at
least have an assert that the hash_search() found a matching entry?
3) What about backpatching?
AFAIK this affects all backbranches, not just master, right? So we
should aim to fix the backbranches too. Do you think this refcounting
fix is backpatchable (it seems like it'd be OK), or do you have an idea
for a less invasive fix?
regards
--
Tomas Vondra
pgsql-hackers by date: