public inbox for [email protected]
help / color / mirror / Atom feedRe: Fix assertion in autovacuum worker
7+ messages / 3 participants
[nested] [flat]
* Re: Fix assertion in autovacuum worker
@ 2023-11-28 22:05 Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2023-11-28 22:05 UTC (permalink / raw)
To: David Geier <[email protected]>; +Cc: pgsql-hackers
On Tue, Nov 28, 2023 at 07:00:16PM +0100, David Geier wrote:
> PostgreSQL hit the following assertion during error cleanup, after being OOM
> in dsa_allocate0():
>
> void dshash_detach(dshash_table *hash_table) {
> ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table);
>
> called from pgstat_shutdown_hook(), called from shmem_exit(), called from
> proc_exit(), called from the exception handler.
Nice find.
> AutoVacWorkerMain() pgstat_report_autovac() pgstat_get_entry_ref_locked()
> pgstat_get_entry_ref() dshash_find_or_insert() resize() resize() locks all
> partitions so the hash table can safely be resized. Then it calls
> dsa_allocate0(). If dsa_allocate0() fails to allocate, it errors out. The
> exception handler calls proc_exit() which normally calls LWLockReleaseAll()
> via AbortTransaction() but only if there's an active transaction. However,
> pgstat_report_autovac() runs before a transaction got started and hence
> LWLockReleaseAll() doesn't run before pgstat_shutdown_hook() is called.
From a glance, it looks to me like the problem is that pgstat_shutdown_hook
is registered as a before_shmem_exit callback, while ProcKill is registered
as an on_shmem_exit callback. However, IIUC even moving them to the same
list wouldn't be sufficient because the pg_stat_shutdown_hook is registered
after ProcKill, and the code that calls the callbacks walks backwards
through the list.
I would expect your patch to fix this particular issue, but I'm wondering
whether there's a bigger problem here.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix assertion in autovacuum worker
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
@ 2023-11-29 00:03 ` Andres Freund <[email protected]>
2023-11-29 02:42 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2023-11-29 00:03 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: David Geier <[email protected]>; pgsql-hackers
Hi,
On 2023-11-28 16:05:16 -0600, Nathan Bossart wrote:
> On Tue, Nov 28, 2023 at 07:00:16PM +0100, David Geier wrote:
> > PostgreSQL hit the following assertion during error cleanup, after being OOM
> > in dsa_allocate0():
> >
> > void dshash_detach(dshash_table *hash_table) {
> > ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table);
> >
> > called from pgstat_shutdown_hook(), called from shmem_exit(), called from
> > proc_exit(), called from the exception handler.
>
> Nice find.
+1
> > AutoVacWorkerMain() pgstat_report_autovac() pgstat_get_entry_ref_locked()
> > pgstat_get_entry_ref() dshash_find_or_insert() resize() resize() locks all
> > partitions so the hash table can safely be resized. Then it calls
> > dsa_allocate0(). If dsa_allocate0() fails to allocate, it errors out. The
> > exception handler calls proc_exit() which normally calls LWLockReleaseAll()
> > via AbortTransaction() but only if there's an active transaction. However,
> > pgstat_report_autovac() runs before a transaction got started and hence
> > LWLockReleaseAll() doesn't run before pgstat_shutdown_hook() is called.
>
> From a glance, it looks to me like the problem is that pgstat_shutdown_hook
> is registered as a before_shmem_exit callback, while ProcKill is registered
> as an on_shmem_exit callback.
That's required, as pgstat_shutdown_hook() needs to acquire lwlocks, which you
can't after ProcKill(). It's also not unique to pgstat, several other
before_shmem_exit() callbacks acquire lwlocks (e.g. AtProcExit_Twophase(),
ParallelWorkerShutdown(), do_pg_abort_backup(), the first three uses of
before_shmem_exit when git grepping) - which makes sense, they are presumably
before_shmem_exit() because they need to manage shared state, which often
needs locks.
In normal backends this is fine-ish, because ShutdownPostgres() is registered
very late (and thus is called early in the shutdown sequence), and the
AbortOutOfAnyTransaction() contained therein indirectly calls
LWLockReleaseAll() and very little happens outside of the transaction context.
> I would expect your patch to fix this particular issue, but I'm wondering
> whether there's a bigger problem here.
Yes, there is - our subsystem initialization, shutdown, error recovery
infrastructure is a mess. We've interwoven transaction handling far too
tightly with error handling, the order of subystem initialization is basically
random and differs between operating systems (due to EXEC_BACKEND) and "mode"
of execution (the order differs when using single user mode) and we've
distributed error recovery into ~10 places (all the sigsetjmp()s in backend
code, xact.c and and a few other places like WalSndErrorCleanup()).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix assertion in autovacuum worker
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
@ 2023-11-29 02:42 ` Nathan Bossart <[email protected]>
2023-11-29 02:48 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2023-11-29 02:42 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: David Geier <[email protected]>; pgsql-hackers
On Tue, Nov 28, 2023 at 04:03:49PM -0800, Andres Freund wrote:
> On 2023-11-28 16:05:16 -0600, Nathan Bossart wrote:
>> From a glance, it looks to me like the problem is that pgstat_shutdown_hook
>> is registered as a before_shmem_exit callback, while ProcKill is registered
>> as an on_shmem_exit callback.
>
> That's required, as pgstat_shutdown_hook() needs to acquire lwlocks, which you
> can't after ProcKill(). It's also not unique to pgstat, several other
> before_shmem_exit() callbacks acquire lwlocks (e.g. AtProcExit_Twophase(),
> ParallelWorkerShutdown(), do_pg_abort_backup(), the first three uses of
> before_shmem_exit when git grepping) - which makes sense, they are presumably
> before_shmem_exit() because they need to manage shared state, which often
> needs locks.
>
> In normal backends this is fine-ish, because ShutdownPostgres() is registered
> very late (and thus is called early in the shutdown sequence), and the
> AbortOutOfAnyTransaction() contained therein indirectly calls
> LWLockReleaseAll() and very little happens outside of the transaction context.
Right. Perhaps we could add a LWLockReleaseAll() to
pgstat_shutdown_hook() instead of the autovacuum code, but I'm afraid that
is still just a hack.
>> I would expect your patch to fix this particular issue, but I'm wondering
>> whether there's a bigger problem here.
>
> Yes, there is - our subsystem initialization, shutdown, error recovery
> infrastructure is a mess. We've interwoven transaction handling far too
> tightly with error handling, the order of subystem initialization is basically
> random and differs between operating systems (due to EXEC_BACKEND) and "mode"
> of execution (the order differs when using single user mode) and we've
> distributed error recovery into ~10 places (all the sigsetjmp()s in backend
> code, xact.c and and a few other places like WalSndErrorCleanup()).
:(
I do remember looking into uniting all the various sigsetjmp() calls
before. That could be worth another try. The rest will probably require
additional thought...
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix assertion in autovacuum worker
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
2023-11-29 02:42 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
@ 2023-11-29 02:48 ` Andres Freund <[email protected]>
2023-11-29 17:52 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2023-11-29 02:48 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: David Geier <[email protected]>; pgsql-hackers
Hi,
On 2023-11-28 20:42:47 -0600, Nathan Bossart wrote:
> On Tue, Nov 28, 2023 at 04:03:49PM -0800, Andres Freund wrote:
> > On 2023-11-28 16:05:16 -0600, Nathan Bossart wrote:
> >> From a glance, it looks to me like the problem is that pgstat_shutdown_hook
> >> is registered as a before_shmem_exit callback, while ProcKill is registered
> >> as an on_shmem_exit callback.
> >
> > That's required, as pgstat_shutdown_hook() needs to acquire lwlocks, which you
> > can't after ProcKill(). It's also not unique to pgstat, several other
> > before_shmem_exit() callbacks acquire lwlocks (e.g. AtProcExit_Twophase(),
> > ParallelWorkerShutdown(), do_pg_abort_backup(), the first three uses of
> > before_shmem_exit when git grepping) - which makes sense, they are presumably
> > before_shmem_exit() because they need to manage shared state, which often
> > needs locks.
> >
> > In normal backends this is fine-ish, because ShutdownPostgres() is registered
> > very late (and thus is called early in the shutdown sequence), and the
> > AbortOutOfAnyTransaction() contained therein indirectly calls
> > LWLockReleaseAll() and very little happens outside of the transaction context.
>
> Right. Perhaps we could add a LWLockReleaseAll() to
> pgstat_shutdown_hook() instead of the autovacuum code, but I'm afraid that
> is still just a hack.
Yea, we'd need that in just about all before_shmem_exit() callbacks. I could
see an argument for doing it in proc_exit_prepare(). While that'd be a fairly
gross layering violation, we already do reset a number a bunch of stuff in
there:
/*
* Forget any pending cancel or die requests; we're doing our best to
* close up shop already. Note that the signal handlers will not set
* these flags again, now that proc_exit_inprogress is set.
*/
InterruptPending = false;
ProcDiePending = false;
QueryCancelPending = false;
InterruptHoldoffCount = 1;
CritSectionCount = 0;
> >> I would expect your patch to fix this particular issue, but I'm wondering
> >> whether there's a bigger problem here.
> >
> > Yes, there is - our subsystem initialization, shutdown, error recovery
> > infrastructure is a mess. We've interwoven transaction handling far too
> > tightly with error handling, the order of subystem initialization is basically
> > random and differs between operating systems (due to EXEC_BACKEND) and "mode"
> > of execution (the order differs when using single user mode) and we've
> > distributed error recovery into ~10 places (all the sigsetjmp()s in backend
> > code, xact.c and and a few other places like WalSndErrorCleanup()).
>
> :(
>
> I do remember looking into uniting all the various sigsetjmp() calls
> before. That could be worth another try. The rest will probably require
> additional thought...
It'd definitely be worth some effort. I'm quite sure that we have a number of
hard to find bugs around this.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix assertion in autovacuum worker
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
2023-11-29 02:42 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 02:48 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
@ 2023-11-29 17:52 ` Nathan Bossart <[email protected]>
2023-11-29 18:55 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Nathan Bossart @ 2023-11-29 17:52 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: David Geier <[email protected]>; pgsql-hackers
On Tue, Nov 28, 2023 at 06:48:59PM -0800, Andres Freund wrote:
> On 2023-11-28 20:42:47 -0600, Nathan Bossart wrote:
>> Right. Perhaps we could add a LWLockReleaseAll() to
>> pgstat_shutdown_hook() instead of the autovacuum code, but I'm afraid that
>> is still just a hack.
>
> Yea, we'd need that in just about all before_shmem_exit() callbacks. I could
> see an argument for doing it in proc_exit_prepare(). While that'd be a fairly
> gross layering violation, we already do reset a number a bunch of stuff in
> there:
Gross layering violations aside, that at least seems more future-proof
against other sigsetjmp() blocks that proc_exit() without doing any
preliminary cleanup.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix assertion in autovacuum worker
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
2023-11-29 02:42 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 02:48 ` Re: Fix assertion in autovacuum worker Andres Freund <[email protected]>
2023-11-29 17:52 ` Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
@ 2023-11-29 18:55 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Andres Freund @ 2023-11-29 18:55 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: David Geier <[email protected]>; pgsql-hackers
Hi,
On 2023-11-29 11:52:01 -0600, Nathan Bossart wrote:
> On Tue, Nov 28, 2023 at 06:48:59PM -0800, Andres Freund wrote:
> > On 2023-11-28 20:42:47 -0600, Nathan Bossart wrote:
> >> Right. Perhaps we could add a LWLockReleaseAll() to
> >> pgstat_shutdown_hook() instead of the autovacuum code, but I'm afraid that
> >> is still just a hack.
> >
> > Yea, we'd need that in just about all before_shmem_exit() callbacks. I could
> > see an argument for doing it in proc_exit_prepare(). While that'd be a fairly
> > gross layering violation, we already do reset a number a bunch of stuff in
> > there:
>
> Gross layering violations aside, that at least seems more future-proof
> against other sigsetjmp() blocks that proc_exit() without doing any
> preliminary cleanup.
It's not just sigsetjmp() blocks not doing cleanup that are problematic -
consider what happens if there's either no sigsetjmp() block established (and
thus ERROR is promoted to FATAL), or if a FATAL error is raised. Then cleanup
in the sigsetjmp() site doesn't matter. So we really need a better answer
here.
If we don't want to add LWLockReleaseAll() to proc_exit_prepare(), ISTM we
should all at least add some assertion infrastructure verifying it's being
called in relevant paths, triggering an assertion if there was no
LWLockReleaseAll() before reaching important before_shmem_exit() routines,
even if we don't actually hold an lwlock at the time of the error. Otherwise
problematic paths are way too hard to find.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v8 11/17] table_scan_bitmap_next_block counts lossy and exact pages
@ 2024-03-22 21:09 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Melanie Plageman @ 2024-03-22 21:09 UTC (permalink / raw)
Now that the table_scan_bitmap_next_block() callback only returns false
when the bitmap is exhausted, it is simpler to move the management of
the lossy and exact page counters into it. We will eventually remove
this callback and table_scan_bitmap_next_tuple() will update those
counters when a new block is read in.
---
src/backend/access/heap/heapam_handler.c | 8 ++++++--
src/backend/executor/nodeBitmapHeapscan.c | 9 ++-------
src/include/access/tableam.h | 21 +++++++++++++--------
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 2ad785e511..266b34fe6b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- bool *recheck, bool *lossy, BlockNumber *blockno)
+ bool *recheck, BlockNumber *blockno,
+ long *lossy_pages, long *exact_pages)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block;
@@ -2267,7 +2268,10 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
- *lossy = tbmres->ntuples < 0;
+ if (tbmres->ntuples < 0)
+ (*lossy_pages)++;
+ else
+ (*exact_pages)++;
/*
* Return true to indicate that a valid block was found and the bitmap is
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 7e73583fe5..96b55507a3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -69,7 +69,6 @@ BitmapHeapNext(BitmapHeapScanState *node)
{
ExprContext *econtext;
TableScanDesc scan;
- bool lossy;
TIDBitmap *tbm;
TupleTableSlot *slot;
ParallelBitmapHeapState *pstate = node->pstate;
@@ -267,14 +266,10 @@ new_page:
BitmapAdjustPrefetchIterator(node);
- if (!table_scan_bitmap_next_block(scan, &node->recheck, &lossy, &node->blockno))
+ if (!table_scan_bitmap_next_block(scan, &node->recheck, &node->blockno,
+ &node->lossy_pages, &node->exact_pages))
break;
- if (lossy)
- node->lossy_pages++;
- else
- node->exact_pages++;
-
/*
* If serial, we can error out if the the prefetch block doesn't stay
* ahead of the current block.
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index a820cc8c99..1d4b79a73f 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -797,8 +797,8 @@ typedef struct TableAmRoutine
* work to allow scan_bitmap_next_tuple() to return tuples (e.g. it might
* make sense to perform tuple visibility checks at this time).
*
- * lossy indicates whether or not the block's representation in the bitmap
- * is lossy or exact.
+ * lossy_pages is incremented if the block's representation in the bitmap
+ * is lossy, otherwise, exact_pages is incremented.
*
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
@@ -815,8 +815,10 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- bool *recheck, bool *lossy,
- BlockNumber *blockno);
+ bool *recheck,
+ BlockNumber *blockno,
+ long *lossy_pages,
+ long *exact_pages);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -2013,15 +2015,17 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
/*
* Prepare to fetch / check / return tuples as part of a bitmap table scan.
* `scan` needs to have been started via table_beginscan_bm(). Returns false if
- * there are no more blocks in the bitmap, true otherwise. lossy is set to true
- * if bitmap is lossy for the selected block and false otherwise.
+ * there are no more blocks in the bitmap, true otherwise. lossy_pages is
+ * incremented if bitmap is lossy for the selected block and exact_pages is
+ * incremented otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- bool *recheck, bool *lossy, BlockNumber *blockno)
+ bool *recheck, BlockNumber *blockno,
+ long *lossy_pages, long *exact_pages)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2032,7 +2036,8 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan, recheck,
- lossy, blockno);
+ blockno, lossy_pages,
+ exact_pages);
}
/*
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-03-22 21:09 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-28 22:05 Re: Fix assertion in autovacuum worker Nathan Bossart <[email protected]>
2023-11-29 00:03 ` Andres Freund <[email protected]>
2023-11-29 02:42 ` Nathan Bossart <[email protected]>
2023-11-29 02:48 ` Andres Freund <[email protected]>
2023-11-29 17:52 ` Nathan Bossart <[email protected]>
2023-11-29 18:55 ` Andres Freund <[email protected]>
2024-03-22 21:09 [PATCH v8 11/17] table_scan_bitmap_next_block counts lossy and exact pages Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox