Re: [PATCH] Fix vacuum_delay_point happening inside lock

Greg, I think this was meant for the list since it opens by greeting
everyone, so I've added pgsql-hackers back and quoted your message in
full.

> Hi Kevin, Andrey, Neil, Tom,
>
> I tested this on my aarch64 Windows/MSVC buildfarm animal (unicorn,
> --enable-cassert, injection_points) which has been crashing repeatedly
> in tests and it fixed the issue. I'd just yesterday started a thread
> [1] on this, but I'll shut that one down and point here instead as I
> think you've got it nailed.
>
> I think this raises the priority of the ANALYZE hunk in v7-0001, and
> argues for landing it ASAP.
>
> In my logs on that animal I find:
>
> TRAP: failed Assert("entry->data.lockmode == BUFFER_LOCK_UNLOCK"),
> bufmgr.c, in BufferLockAcquire (Windows exception 0xC0000409)
>
> And when I reviewed the crash dumps all 10 had the identical stack.
> Reading it outermost to innermost (nearest-symbol mislabels from
> optimized codegen noted):
>
> AutoVacWorkerMain
> -> vacuum -> analyze_rel -> acquire_sample_rows
> -> [heapam_scan_analyze_next_block holds BUFFER_LOCK_SHARE on a
> pg_class page and returns with it held]
> -> vacuum_delay_point(true) <-- analyze.c, delay point
> -> ProcessConfigFile(PGC_SIGHUP) (a config reload landed)
> -> ... -> check_default_text_search_config (GUC check hook)
> -> get_ts_config_oid -> LookupExplicitNamespace
> -> SearchSysCache -> table_open -> relation_open
> -> LockRelationOid -> AcceptInvalidationMessages
> -> RelationCacheInvalidate (rebuild a nailed catalog entry)
> -> systable_getnext -> heapgettup_pagemode
> -> heap_prepare_pagescan -> LockBuffer(BUFFER_LOCK_SHARE)
> -> Assert (second SHARE lock on the pg_class buffer that
> acquire_sample_rows already holds SHARE on)
>
> So this looks to be the same "vacuum_delay_point() reached with a buffer
> content lock held" bug. The ANALYZE call site that v7-0001 moves before
> scan_analyze_next_block(). The extra wrinkle on the ANALYZE path is that
> the delay point can run a SIGHUP config reload, whose GUC check hook
> does a catalog lookup that triggers a relcache rebuild, and that
> rebuild's pagemode pg_class scan re-locks the very buffer the sampling
> scan is still holding SHARE on.
>
> The reason this is more than a cancellation-latency issue on v19 is the
> buffer content-lock rewrite in fcb9c977aa5 tracks only one lock per
> buffer per backend (the single data.lockmode field). A second SHARE
> acquire on an already-share-locked buffer is now:
>
> - a hard Assert/crash in cassert builds (what unicorn shows), and
> - in a non-assert build, an asymmetric leak: BufferLockAttempt() adds
> a second BM_LOCK_VAL_SHARED to the shared state, data.lockmode
> records only one, and release subtracts one -- so that pg_class
> buffer is left permanently one shared-locker too high and can never
> again be locked exclusive. Any later exclusive waiter (VACUUM) on
> that buffer blocks for the life of the cluster.
>
> Pre-v19 the double SHARE was harmless (the held-lwlocks array could
> represent it), which is presumably why the call site survived so long.
>
> The steps to reproduce this are ordinary, an autovacuum ANALYZE of a
> catalog (pg_class here, reached via the text-search-config GUC hook's
> syscache lookup) that overlaps a config reload. My animal happens to
> build with injection_points, but nothing in this stack is an injection
> point, it is the stock ANALYZE -> vacuum_delay_point ->
> ProcessConfigFile -> relcache-rebuild path, so I don't believe
> injection_points is required to hit it.
>
> I have not seen the crash on non-cassert animals because there it silently
> leaks the lock rather than asserting, which is arguably worse.
>
> Given that v7-0001 already contains the fix (thank you), my only ask is
> that the ANALYZE hunk be treated as a v19 crash-regression fix rather than
> a latency improvement, and backpatched to REL_19 before GA. Happy to test
> again on the aarch64/MSVC animal.
>
> best.
>
> -greg
>
> [1] https://postgr.es/m/arKVu9wp5A7EdKkx@floki

I've attached v8 of the patch. The code is the same, but I extracted the
ANALYZE fix to 0001 so it can be backpatched separately.

This is a v19 regression from fcb9c977aa5, so I think it needs an open
item. I don't have wiki edit access yet, so could someone from the RMT
(Cc'd) add it, with Andres as owner?

- Kevin Rocker