Date: Thu, 20 Feb 2020 14:02:36 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: error context for vacuum to include block number
Message-ID: <20200220170236.GA16805@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200219203821.GA10914@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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