pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Date: Fri, 14 Feb 2020 09:34:12 -0600
Message-ID: <20200214153412.GD31889@telsasoft.com> (raw )
In-Reply-To: <CA+fd4k5oJn1TevFH=7oDmqc=F95KCw2+XvunPL=s4L327UDXig@mail.gmail.com >
References: <20200127053813.GS13621@telsasoft.com >
<CA+fd4k71fHRYJAeGxigGza5U+_HDJcBAbMniiMK2xz5GHhsMmQ@mail.gmail.com >
<20200127225018.GY13621@telsasoft.com >
<CA+fd4k48wcdYpr-4UsLzchohOk6pwW3Aijmw_PR4hEXJBcs8Xw@mail.gmail.com >
<20200202060259.GG13621@telsasoft.com >
<CA+fd4k7U__9nWB5q0njLc66rBJfekKAUVJXOPJ0HEw-DYocyLQ@mail.gmail.com >
<20200208010107.GT403@telsasoft.com >
<CA+fd4k4QHeO_fpFjYGd4RRxMWtNWwXZozG2JGTWdo8YRQ15k8g@mail.gmail.com >
<20200213235254.GC31889@telsasoft.com >
<CA+fd4k5oJn1TevFH=7oDmqc=F95KCw2+XvunPL=s4L327UDXig@mail.gmail.com >
On Fri, Feb 14, 2020 at 12:30:25PM +0900, Masahiko Sawada wrote:
> * I think the function name is too generic. init_vacuum_error_callback
> or init_vacuum_errcallback is better.
> * The comment of this function is not accurate since this function is
> not only for heap vacuum but also index vacuum. How about just
> "Initialize vacuum error callback"?
> * I think it's easier to read the code if we set the relname and
> indname in the same order.
> * The comment I wrote in the previous mail seems better, because in
> this function the reader might get confused that 'rel' is a relation
> or an index depending on the phase but that comment helps it.
Fixed these
> * rel->rd_index->indexrelid should be rel->rd_index->indrelid.
Ack. I think that's been wrong since I first wrote it two weeks ago :(
The error is probably more obvious due to the switch statement you proposed.
Thanks for continued reviews.
--
Justin
Attachments:
[text/x-diff] v19-0001-vacuum-errcontext-to-show-block-being-processed.patch (8.5K, ../20200214153412.GD31889@telsasoft.com/2-v19-0001-vacuum-errcontext-to-show-block-being-processed.patch)
download | inline diff:
From 94768a134118d30853b75a96b90166363f0fef5b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 12 Dec 2019 20:54:37 -0600
Subject: [PATCH v19] vacuum errcontext to show block being processed
Discussion:
https://www.postgresql.org/message-id/20191120210600.GC30362@telsasoft.com
---
src/backend/access/heap/vacuumlazy.c | 120 +++++++++++++++++++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index a23cdef..ebfb2e7 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -292,6 +292,14 @@ typedef struct LVRelStats
bool lock_waiter_detected;
} LVRelStats;
+typedef struct
+{
+ char *relnamespace;
+ char *relname;
+ char *indname;
+ BlockNumber blkno; /* used only for heap operations */
+ int phase; /* Reusing same enums as for progress reporting */
+} vacuum_error_callback_arg;
/* A few variables that don't seem worth passing around as parameters */
static int elevel = -1;
@@ -361,6 +369,9 @@ static void end_parallel_vacuum(Relation *Irel, IndexBulkDeleteResult **stats,
LVParallelState *lps, int nindexes);
static LVSharedIndStats *get_indstats(LVShared *lvshared, int n);
static bool skip_parallel_vacuum_index(Relation indrel, LVShared *lvshared);
+static void vacuum_error_callback(void *arg);
+static void init_vacuum_error_callback(ErrorContextCallback *errcallback,
+ vacuum_error_callback_arg *errcbarg, Relation onerel, int phase);
/*
@@ -724,6 +735,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
PROGRESS_VACUUM_MAX_DEAD_TUPLES
};
int64 initprog_val[3];
+ ErrorContextCallback errcallback;
+ vacuum_error_callback_arg errcbarg;
pg_rusage_init(&ru0);
@@ -870,6 +883,10 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
else
skipping_blocks = false;
+ /* Setup error traceback support for ereport() */
+ init_vacuum_error_callback(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_SCAN_HEAP);
+ error_context_stack = &errcallback;
+
for (blkno = 0; blkno < nblocks; blkno++)
{
Buffer buf;
@@ -891,6 +908,8 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
#define FORCE_CHECK_PAGE() \
(blkno == nblocks - 1 && should_attempt_truncation(params, vacrelstats))
+ errcbarg.blkno = blkno;
+
pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
if (blkno == next_unskippable_block)
@@ -987,6 +1006,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
vmbuffer = InvalidBuffer;
}
+ /* Pop the error context stack while calling vacuum */
+ error_context_stack = errcallback.previous;
+
/* Work on all the indexes, then the heap */
lazy_vacuum_all_indexes(onerel, Irel, indstats,
vacrelstats, lps, nindexes);
@@ -1011,6 +1033,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
/* Report that we are once again scanning the heap */
pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_PHASE_SCAN_HEAP);
+
+ /* Set the error context while continuing heap scan */
+ error_context_stack = &errcallback;
}
/*
@@ -1597,6 +1622,9 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
RecordPageWithFreeSpace(onerel, blkno, freespace);
}
+ /* Pop the error context stack */
+ error_context_stack = errcallback.previous;
+
/* report that everything is scanned and vacuumed */
pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
@@ -1772,11 +1800,19 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
int npages;
PGRUsage ru0;
Buffer vmbuffer = InvalidBuffer;
+ ErrorContextCallback errcallback;
+ vacuum_error_callback_arg errcbarg;
/* Report that we are now vacuuming the heap */
pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
+ /*
+ * Setup error traceback support for ereport()
+ */
+ init_vacuum_error_callback(&errcallback, &errcbarg, onerel, PROGRESS_VACUUM_PHASE_VACUUM_HEAP);
+ error_context_stack = &errcallback;
+
pg_rusage_init(&ru0);
npages = 0;
@@ -1791,6 +1827,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
vacuum_delay_point();
tblk = ItemPointerGetBlockNumber(&vacrelstats->dead_tuples->itemptrs[tupindex]);
+ errcbarg.blkno = tblk;
buf = ReadBufferExtended(onerel, MAIN_FORKNUM, tblk, RBM_NORMAL,
vac_strategy);
if (!ConditionalLockBufferForCleanup(buf))
@@ -1811,6 +1848,9 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
npages++;
}
+ /* Pop the error context stack */
+ error_context_stack = errcallback.previous;
+
if (BufferIsValid(vmbuffer))
{
ReleaseBuffer(vmbuffer);
@@ -2318,6 +2358,8 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
IndexVacuumInfo ivinfo;
const char *msg;
PGRUsage ru0;
+ ErrorContextCallback errcallback;
+ vacuum_error_callback_arg errcbarg;
pg_rusage_init(&ru0);
@@ -2329,10 +2371,17 @@ lazy_vacuum_index(Relation indrel, IndexBulkDeleteResult **stats,
ivinfo.num_heap_tuples = reltuples;
ivinfo.strategy = vac_strategy;
+ /* Setup error traceback support for ereport() */
+ init_vacuum_error_callback(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_VACUUM_INDEX);
+ error_context_stack = &errcallback;
+
/* Do bulk deletion */
*stats = index_bulk_delete(&ivinfo, *stats,
lazy_tid_reaped, (void *) dead_tuples);
+ /* Pop the error context stack */
+ error_context_stack = errcallback.previous;
+
if (IsParallelWorker())
msg = gettext_noop("scanned index \"%s\" to remove %d row versions by parallel vacuum worker");
else
@@ -2359,6 +2408,8 @@ lazy_cleanup_index(Relation indrel,
IndexVacuumInfo ivinfo;
const char *msg;
PGRUsage ru0;
+ vacuum_error_callback_arg errcbarg;
+ ErrorContextCallback errcallback;
pg_rusage_init(&ru0);
@@ -2371,8 +2422,15 @@ lazy_cleanup_index(Relation indrel,
ivinfo.num_heap_tuples = reltuples;
ivinfo.strategy = vac_strategy;
+ /* Setup error traceback support for ereport() */
+ init_vacuum_error_callback(&errcallback, &errcbarg, indrel, PROGRESS_VACUUM_PHASE_INDEX_CLEANUP);
+ error_context_stack = &errcallback;
+
*stats = index_vacuum_cleanup(&ivinfo, *stats);
+ /* Pop the error context stack */
+ error_context_stack = errcallback.previous;
+
if (!(*stats))
return;
@@ -3375,3 +3433,65 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
table_close(onerel, ShareUpdateExclusiveLock);
pfree(stats);
}
+
+/*
+ * Error context callback for errors occurring during vacuum.
+ */
+static void
+vacuum_error_callback(void *arg)
+{
+ vacuum_error_callback_arg *cbarg = arg;
+
+ switch (cbarg->phase) {
+ case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
+ if (BlockNumberIsValid(cbarg->blkno))
+ errcontext(_("while scanning block %u of relation \"%s.%s\""),
+ cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+ break;
+
+ case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
+ if (BlockNumberIsValid(cbarg->blkno))
+ errcontext(_("while vacuuming block %u of relation \"%s.%s\""),
+ cbarg->blkno, cbarg->relnamespace, cbarg->relname);
+ break;
+
+ case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
+ errcontext(_("while vacuuming index \"%s.%s\" of relation \"%s\""),
+ cbarg->relnamespace, cbarg->indname, cbarg->relname);
+ break;
+
+ case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
+ errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
+ cbarg->relnamespace, cbarg->indname, cbarg->relname);
+ break;
+ }
+}
+
+/* Initialize vacuum error callback */
+static void
+init_vacuum_error_callback(ErrorContextCallback *errcallback, vacuum_error_callback_arg *errcbarg, Relation rel, int phase)
+{
+ switch (phase)
+ {
+ case PROGRESS_VACUUM_PHASE_SCAN_HEAP:
+ case PROGRESS_VACUUM_PHASE_VACUUM_HEAP:
+ errcbarg->relname = RelationGetRelationName(rel);
+ errcbarg->indname = NULL; /* Not used for heap */
+ break;
+
+ case PROGRESS_VACUUM_PHASE_VACUUM_INDEX:
+ case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
+ /* rel is an index relation in index vacuum case */
+ errcbarg->relname = get_rel_name(rel->rd_index->indrelid);
+ errcbarg->indname = RelationGetRelationName(rel);
+ break;
+ }
+
+ errcbarg->relnamespace = get_namespace_name(RelationGetNamespace(rel));
+ errcbarg->blkno = InvalidBlockNumber; /* Not known yet */
+ errcbarg->phase = phase;
+
+ errcallback->callback = vacuum_error_callback;
+ errcallback->arg = errcbarg;
+ errcallback->previous = error_context_stack;
+}
--
2.7.4
view thread (139+ messages) latest in thread
Message-ID: <20200214153412.GD31889@telsasoft.com>
Permalink: ../20200214153412.GD31889@telsasoft.com/
Also on: postgresql.org/message-id/20200214153412.GD31889@telsasoft.com
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, masahiko.sawada@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz, alvherre@2ndquadrant.com
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200214153412.GD31889@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox