Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c)

Hi Tomas,

Thanks for your reply. I attached a v8 with patches for all the supported
branches:

- v8-0001/0002: master, no changes from v7, just rebased on current master.
- nocfbot-v8-0001/0002-PG19: same as master.
- nocfbot-v8-0001/0002-PG14 to PG18: same test as master, and the same fix
on the five branches, with two differences:

1) ResourceOwnerDesc doesn't exist before 17, so ri_PerformCheck() wraps
the execution in PG_TRY(), and the catch block drops the count of that
execution and re-throws. I used PG_TRY() on 17 and 18 too, so that
the five branches get the same patch. On master I kept the resource
owner, since SPI already pins the CachedPlan it executes that way, and
it warns if a code path ever misses the decrement.

2) AtEOXact_RI() doesn't exist before 19, so a plan whose freeing was
deferred and whose last execution was aborted is freed in the next
call to
ri_PerformCheck().

Without the fix, the test crashes the backend on 14, 15, 18, 19 and master.
On 16 and 17 it doesn't crash in my builds, although the plan is freed while
in use (I checked it with some logging), since it depends on how the freed
memory gets reused. With the fix, regress and isolation pass on all
branches.

For the benchmark I used release builds of master with and without the
patch, one client, 10 rounds alternating both builds, and took the average:

test case base patched diff
cascade, no children (1M) 4642 ms 4772 ms +2.8%
cascade, one child (1M) 8046 ms 8251 ms +2.6%
self-ref, nested (200k) 7168 ms 7258 ms +1.3%
self-ref, nested + inval (20k) crash 115705 ms

The first two are the common case, a cascade with no nesting. The last two
are the case of the test: a BEFORE DELETE trigger on a self-referencing
table reenters the cascade, so the refcount table is not empty and
ri_PreparedPlanFreeDeferred() scans it on every nested call; in the last one
the trigger also invalidates the plan, which is the crash without the fix.

The setup minimizes I/O (unlogged tables, fsync and autovacuum off) to focus
on the cost of the patched code, so with real I/O the difference should be
much smaller. The scripts are attached.

The scripts used for benchmarking are attached.

Regards,
Lucas Jeffrey