Date: Thu, 26 Mar 2020 10:04:57 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	Alvaro Herrera <alvherre@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: <20200326150457.GB17431@telsasoft.com>
References: 
 <CAA4eK1KRvWEpcvEM9rc_oNi8PFSps-kOvzebxFUE0JXdanjE-g@mail.gmail.com>
 <20200325101229.GR21443@telsasoft.com>
 <CAA4eK1KwcMWa7Jwewh48xC9o9pExzd=CuRuox94+AHvYr3F7Qg@mail.gmail.com>
 <CA+fd4k6-zFG=mvkZzgsLQZ3v+df5Bk_0d8tpSzcRV0u40dM0rw@mail.gmail.com>
 <20200325124155.GU21443@telsasoft.com>
 <CAA4eK1+=ToXurzjOcPyQ4j6Vz2nG2C-cUdg=yZoUpN+g1k5m6w@mail.gmail.com>
 <20200326044115.GB28385@telsasoft.com>
 <CAA4eK1LBtp+qpsz8uR-oj2h6jzhLEVo9TEdm46anKM-n=8nS0g@mail.gmail.com>
 <CAA4eK1L33gvQ1z-CHctzTWng2HfmFn0cDJif_mpBOvG47Mymow@mail.gmail.com>
 <CA+fd4k61uPzeAWT1EZOwPWq7DkhgVTrC_wmUNCzVW+MbP9Wjfg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k61uPzeAWT1EZOwPWq7DkhgVTrC_wmUNCzVW+MbP9Wjfg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Mar 26, 2020 at 08:56:54PM +0900, Masahiko Sawada wrote:
> 1.
> @@ -1844,9 +1914,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber
> blkno, Buffer buffer,
>     int         uncnt = 0;
>     TransactionId visibility_cutoff_xid;
>     bool        all_frozen;
> +   LVRelStats  olderrcbarg;
> 
>     pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
> 
> +   /* Update error traceback information */
> +   olderrcbarg = *vacrelstats;
> +   update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> +                             blkno, NULL, false);
> 
> Since we update vacrelstats->blkno during in the loop in
> lazy_vacuum_heap() we unnecessarily update blkno twice to the same
> value. Also I think we don't need to revert back the callback
> arguments in lazy_vacuum_page(). Perhaps we can either remove the
> change of lazy_vacuum_page() or move the code updating
> vacrelstats->blkno to the beginning of lazy_vacuum_page(). I prefer
> the latter.

We want the error callback to be in place during lazy_scan_heap, since it
calls ReadBufferExtended().

We can't remove the change in lazy_vacuum_page, since it's also called from
lazy_scan_heap, if there are no indexes.

We want lazy_vacuum_page to "revert back" since we go from "scanning heap" to
"vacuuming heap".  lazy_vacuum_page was the motivation for saving and restoring
the called arguments, otherwise lazy_scan_heap() would have to clean up after
the function it called, which was unclean.  Now, every function cleans up after
itself.

Does that address your comment ?

> +static void
> +update_vacuum_error_cbarg(LVRelStats *errcbarg, int phase, BlockNumber blkno,
> +                         char *indname, bool free_oldindname)
> 
> I'm not sure why "free_oldindname" is necessary. Since we initialize
> vacrelstats->indname with NULL and revert the callback arguments at
> the end of functions that needs update them, vacrelstats->indname is
> NULL at the beginning of lazy_vacuum_index() and lazy_cleanup_index().
> And we make a copy of index name in update_vacuum_error_cbarg(). So I
> think we can pfree the old index name if errcbarg->indname is not NULL.

We want to avoid doing this:
 olderrcbarg = *vacrelstats // saves a pointer
 update_vacuum_error_cbarg(... NULL); // frees the pointer and sets indname to NULL
 update_vacuum_error_cbarg(... olderrcbarg.oldindnam) // puts back the pointer, which has been freed
 // hit an error, and the callback accesses the pfreed pointer

I think that's only an issue for lazy_vacuum_index().

And I think you're right: we only save state when the calling function has a
indname=NULL, so we never "put back" a non-NULL indname.  We go from having a
indname=NULL at lazy_scan_heap to not not-NULL at lazy_vacuum_index, and never
the other way around.  So once we've "reverted back", 1) the pointer is null;
and, 2) the callback function doesn't access it for the previous/reverted phase
anyway.

Hm, I was just wondering what happens if an error happens *during*
update_vacuum_error_cbarg().  It seems like if we set
errcbarg->phase=VACUUM_INDEX before setting errcbarg->indname=indname, then an
error would cause a crash.  And if we pfree and set indname before phase, it'd
be a problem when going from an index phase to non-index phase.  So maybe we
have to set errcbarg->phase=VACUUM_ERRCB_PHASE_UNKNOWN while in the function,
and errcbarg->phase=phase last.

-- 
Justin