From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Date: Thu, 20 Feb 2020 14:02:36 -0300
Message-ID: <20200220170236.GA16805@alvherre.pgsql> (raw)
In-Reply-To: <20200219203821.GA10914@telsasoft.com>
This is, by far, the most complex error context callback we've tried to
write ... Easy stuff first:
In the error context function itself, you don't need the _() around the
strings: errcontext() is marked as a gettext trigger and it does the
translation itself, so the manually added _() is just cruft.
When reporting index names, make sure to attach the namespace to the
table, not to the index. Example:
case PROGRESS_VACUUM_PHASE_INDEX_CLEANUP:
- errcontext(_("while cleaning up index \"%s.%s\" of relation \"%s\""),
- cbarg->relnamespace, cbarg->indname, cbarg->relname);
+ errcontext("while cleaning up index \"%s\" of relation \"%s.%s\"",
+ cbarg->indname, cbarg->relnamespace, cbarg->relname);
I think it would be worthwhile to have the "truncate wait" phase as a
separate thing from the truncate itself, since it requires acquiring a
possibly taken lock. This suggests that using the progress enum is not
a 100% solution ... or maybe it suggests that the progress enum too
needs to report the truncate-wait phase separately. (I like the latter
myself, actually.)
On 2020-Feb-19, Justin Pryzby wrote:
> Also, I was thinking that lazy_scan_heap doesn't needs to do this:
>
> + /* Pop the error context stack while calling vacuum */
> + error_context_stack = errcallback.previous;
> ...
> + /* Set the error context while continuing heap scan */
> + error_context_stack = &errcallback;
>
> It seems to me that's not actually necessary, since lazy_vacuum_heap will just
> *push* a context handler onto the stack, and then pop it back off.
So if you don't pop before pushing, you'll end up with two context
lines, right?
I find that arrangement a bit confusing. I think it would make sense to
initialize the context callback just *once* for a vacuum run, and from
that point onwards, just update the errcbarg struct to match what
you're currently doing -- not continually pop/push error callback stack
entries. See below ...
(This means you need to pass the "cbarg" as new argument to some of the
called functions, so that they can update it.)
Another point is that this patch seems to be leaking memory each time
you set relation/index/namespace name, since you never free those and
they are changed over and over.
In init_vacuum_error_callback() you don't need the "switch(phase)" bit;
instead, test rel->rd_rel->relkind, and if it's RELKIND_INDEX then you
put the relname as indexname, otherwise set it to NULL (after freeing
the previous value, if there's one). Note that with this, you only need
to set the relation name (table name) in the first call! IOW you should
split init_vacuum_error_callback() in two functions: one "init" to call
at start of vacuum, where you set relnamespace and relname; the other
function is update_vacuum_error_callback() (or you find a better name
for that) and it sets the phase, and optionally the block number and
index name (these last two get reset to InvalidBlkNum/ NULL if not
passed by caller). I'm not really sure what this means for the parallel
index vacuuming stuff; probably you'll need a special case for that: the
parallel children will need to "init" on their own, right?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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: alvherre@2ndquadrant.com, pryzby@telsasoft.com, masahiko.sawada@2ndquadrant.com, andres@anarazel.de, michael@paquier.xyz
Subject: Re: error context for vacuum to include block number
In-Reply-To: <20200220170236.GA16805@alvherre.pgsql>
* 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