From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: error context for vacuum to include block number |
Date: | 2020-03-26 22:17:52 |
Message-ID: | 20200326221752.GR17431@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Mar 26, 2020 at 10:04:57AM -0500, Justin Pryzby wrote:
> Does that address your comment ?
I hope so.
> > 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.
I removed the free_oldindname argument.
> 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.
And addressed that.
Also, I realized that lazy_cleanup_index has an early "return", so the "Revert
back" was ineffective. We talked about how that wasn't needed, since we never
go back to a previous phase. Amit wanted to keep it there for consistency, but
I'd prefer to put any extra effort into calling out the special treatment
needed/given to lazy_vacuum_heap/index, rather than making everything
"consistent".
Amit: I also moved the TRUNCATE_HEAP bit back to truncate_heap(), since 1) it's
odd if we don't have anything in truncate_heap() about error reporting except
for "vacrelstats->blkno = blkno"; and, 2) it's nice to set the err callback arg
right after pgstat_progress, and outside of any loop. In previous versions, it
was within the loop, because it closely wrapped RelationTruncate() and
count_nondeletable_pages() - a previous version used separate phases.
--
Justin
Attachment | Content-Type | Size |
---|---|---|
v36-0001-Introduce-vacuum-errcontext-to-display-additiona.patch | text/x-diff | 21.4 KB |
v36-0002-Drop-reltuples.patch | text/x-diff | 4.0 KB |
v36-0003-Avoid-some-calls-to-RelationGetRelationName.patch | text/x-diff | 3.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Cary Huang | 2020-03-26 22:33:33 | Re: Include sequence relation support in logical replication |
Previous Message | Alvaro Herrera | 2020-03-26 21:56:36 | Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line |