Horton

Thread

Commits

  1. Improve vacuum error context handling.

  2. Introduce vacuum errcontext to display additional information.

  1. Backpatch b61d161c14

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-22T05:05:47Z

    I propose to backpatch b61d161c14 [1] (Introduce vacuum errcontext to
display additional information.).  In the recent past, we have seen an
error report similar to "ERROR: found xmin 2157740646 from before
relfrozenxid 1197" from multiple EDB customers.  A similar report is
seen on pgsql-bugs as well [2] which I think has triggered the
implementation of this feature for v13.  Such reports mostly indicate
database corruption rather than any postgres bug which is also
indicated by the error-code (from before relfrozenxid) for this
message. I think there is a good reason to back-patch this as multiple
users are facing similar issues.  This patch won't fix this issue but
it will help us in detecting the problematic part of the heap/index
and then if users wish they can delete the portion of data that
appeared to be corrupted and resume the operations on that relation.

I have tried to back-patch this for v12 and attached is the result.
The attached patch passes make check-world but I have yet to test it
manually and also prepare the patch for other branches once we agree
on this proposal.

Thoughts?

[1] -
commit b61d161c146328ae6ba9ed937862d66e5c8b035a
Author: Amit Kapila <akapila@postgresql.org>
Date:   Mon Mar 30 07:33:38 2020 +0530

    Introduce vacuum errcontext to display additional information.

    The additional information displayed will be block number for error
    occurring while processing heap and index name for error occurring
    while processing the index.

[2] - https://www.postgresql.org/message-id/20190807235154.erbmr4o4bo6vgnjv%40alap3.anarazel.de

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  2. Re: Backpatch b61d161c14

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-22T12:05:16Z

    On Mon, Jun 22, 2020 at 10:35 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I propose to backpatch b61d161c14 [1] (Introduce vacuum errcontext to
> display additional information.).  In the recent past, we have seen an
> error report similar to "ERROR: found xmin 2157740646 from before
> relfrozenxid 1197" from multiple EDB customers.  A similar report is
> seen on pgsql-bugs as well [2] which I think has triggered the
> implementation of this feature for v13.  Such reports mostly indicate
> database corruption rather than any postgres bug which is also
> indicated by the error-code (from before relfrozenxid) for this
> message.
>

Sorry, the error-code I want to refer to in above sentence was
ERRCODE_DATA_CORRUPTED.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  3. Re: Backpatch b61d161c14

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-06-22T16:32:13Z

    On 2020-Jun-22, Amit Kapila wrote:

> I propose to backpatch b61d161c14 [1] (Introduce vacuum errcontext to
> display additional information.).  In the recent past, we have seen an
> error report similar to "ERROR: found xmin 2157740646 from before
> relfrozenxid 1197" from multiple EDB customers.  A similar report is
> seen on pgsql-bugs as well [2] which I think has triggered the
> implementation of this feature for v13.

+1 to backpatching this change.  I did not review your actual patch,
though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: Backpatch b61d161c14

    Andres Freund <andres@anarazel.de> — 2020-06-22T20:09:39Z

    Hi,

On 2020-06-22 10:35:47 +0530, Amit Kapila wrote:
> I propose to backpatch b61d161c14 [1] (Introduce vacuum errcontext to
> display additional information.).  In the recent past, we have seen an
> error report similar to "ERROR: found xmin 2157740646 from before
> relfrozenxid 1197" from multiple EDB customers.  A similar report is
> seen on pgsql-bugs as well [2] which I think has triggered the
> implementation of this feature for v13.  Such reports mostly indicate
> database corruption rather than any postgres bug which is also
> indicated by the error-code (from before relfrozenxid) for this
> message. I think there is a good reason to back-patch this as multiple
> users are facing similar issues.  This patch won't fix this issue but
> it will help us in detecting the problematic part of the heap/index
> and then if users wish they can delete the portion of data that
> appeared to be corrupted and resume the operations on that relation.
> 
> I have tried to back-patch this for v12 and attached is the result.
> The attached patch passes make check-world but I have yet to test it
> manually and also prepare the patch for other branches once we agree
> on this proposal.

I think having the additional information in the back branches would be
good. But on the other hand I think this is a somewhat large change
to backpatch, and it hasn't yet much real world exposure.

I'm also uncomfortable with the approach of just copying all of
LVRelStats in several places:

>  /*
> @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
>  	int			uncnt = 0;
>  	TransactionId visibility_cutoff_xid;
>  	bool		all_frozen;
> +	LVRelStats	olderrinfo;
>  
>  	pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
>  
> +	/* Update error traceback information */
> +	olderrinfo = *vacrelstats;
> +	update_vacuum_error_info(vacrelstats, VACUUM_ERRCB_PHASE_VACUUM_HEAP,
> +							 blkno, NULL);
> +
>  	START_CRIT_SECTION();
>  
>  	for (; tupindex < vacrelstats->num_dead_tuples; tupindex++)
> @@ -1659,6 +1733,11 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
>  							  *vmbuffer, visibility_cutoff_xid, flags);
>  	}
>  
> +	/* Revert to the previous phase information for error traceback */
> +	update_vacuum_error_info(vacrelstats,
> +							 olderrinfo.phase,
> +							 olderrinfo.blkno,
> +							 olderrinfo.indname);
>  	return tupindex;
>  }

To me that's a very weird approach. It's fragile because we need to be
sure that there's no updates to the wrong LVRelStats for important
things, and it has a good bit of potential to be inefficient because
LVRelStats isn't exactly small. This pretty much relies on the compiler
doing good enough escape analysis to realize that most parts of
olderrinfo aren't touched.

Greetings,

Andres Freund

  5. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Justin Pryzby <pryzby@telsasoft.com> — 2020-06-22T20:43:11Z

    On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> On 2020-06-22 10:35:47 +0530, Amit Kapila wrote:
> > I propose to backpatch b61d161c14 [1] (Introduce vacuum errcontext to
> > display additional information.).
...
> I think having the additional information in the back branches would be
> good. But on the other hand I think this is a somewhat large change
> to backpatch, and it hasn't yet much real world exposure.

I see that's nontrivial to cherry-pick due to parallel vacuum changes, and due
to re-arranging calls to pgstat_progress.

Since the next minor releases are in August, and PG13 expected to be released
~October, we could defer backpatching until November (or later).

> I'm also uncomfortable with the approach of just copying all of
> LVRelStats in several places:
> 
> >  /*
> > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> >  	int			uncnt = 0;
> >  	TransactionId visibility_cutoff_xid;
> >  	bool		all_frozen;
> > +	LVRelStats	olderrinfo;

I guess the alternative is to write like

LVRelStats	olderrinfo = {
	.phase = vacrelstats.phase,
	.blkno = vacrelstats.blkno,
	.indname = vacrelstats.indname,
};

-- 
Justin

  6. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Andres Freund <andres@anarazel.de> — 2020-06-22T20:57:12Z

    Hi,

On 2020-06-22 15:43:11 -0500, Justin Pryzby wrote:
> On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> > I'm also uncomfortable with the approach of just copying all of
> > LVRelStats in several places:
> > 
> > >  /*
> > > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> > >  	int			uncnt = 0;
> > >  	TransactionId visibility_cutoff_xid;
> > >  	bool		all_frozen;
> > > +	LVRelStats	olderrinfo;
> 
> I guess the alternative is to write like
> 
> LVRelStats	olderrinfo = {
> 	.phase = vacrelstats.phase,
> 	.blkno = vacrelstats.blkno,
> 	.indname = vacrelstats.indname,
> };

No, I don't think that's a solution. I think it's wrong to have
something like olderrinfo in the first place. Using a struct with ~25
members to store the current state of three variables just doesn't make
sense.  Why isn't this just a LVSavedPosition struct or something like
that?

Greetings,

Andres Freund

  7. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-22T22:15:27Z

    Andres Freund <andres@anarazel.de> writes:
> No, I don't think that's a solution. I think it's wrong to have
> something like olderrinfo in the first place. Using a struct with ~25
> members to store the current state of three variables just doesn't make
> sense.  Why isn't this just a LVSavedPosition struct or something like
> that?

That seems like rather pointless micro-optimization really; the struct's
not *that* large.  But I have a different complaint now that I look at
this code: is it safe at all?  I see that the indname field is a pointer
to who-knows-where.  If it's possible in the first place for that to
change while this code runs, then what guarantees that we won't be
restoring a dangling pointer to freed memory?

			regards, tom lane

  8. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Justin Pryzby <pryzby@telsasoft.com> — 2020-06-22T22:53:01Z

    On Mon, Jun 22, 2020 at 06:15:27PM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > No, I don't think that's a solution. I think it's wrong to have
> > something like olderrinfo in the first place. Using a struct with ~25
> > members to store the current state of three variables just doesn't make
> > sense.  Why isn't this just a LVSavedPosition struct or something like
> > that?
> 
> That seems like rather pointless micro-optimization really; the struct's
> not *that* large.  But I have a different complaint now that I look at
> this code: is it safe at all?  I see that the indname field is a pointer
> to who-knows-where.  If it's possible in the first place for that to
> change while this code runs, then what guarantees that we won't be
> restoring a dangling pointer to freed memory?

I'm not sure it addresses your concern, but we talked a bunch about safety
starting here:
https://www.postgresql.org/message-id/20200326150457.GB17431%40telsasoft.com
..and concluding with an explanation about CHECK_FOR_INTERRUPTS.

20200326150457.GB17431@telsasoft.com
|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.

When this function is called by lazy_vacuum_{heap,page,index}, it's also called
a 2nd time to restore the previous phase information.  When it's called the
first time by lazy_vacuum_index(), it does errinfo->indname = pstrdup(indname),
and on the 2nd call then does pfree(errinfo->indame), followed by
errinfo->indname = NULL.

|static void
|update_vacuum_error_info(LVSavedPosition *errinfo, int phase, BlockNumber blkno,
|                                                 char *indname)
|{
|        errinfo->blkno = blkno;
|        errinfo->phase = phase;
|
|        /* Free index name from any previous phase */
|        if (errinfo->indname)
|                pfree(errinfo->indname);
|
|        /* For index phases, save the name of the current index for the callback */
|        errinfo->indname = indname ? pstrdup(indname) : NULL;
|}

If it's inadequately clear, maybe we should do:

         if (errinfo->indname)
+        {
                 pfree(errinfo->indname);
+                Assert(indname == NULL);
+        }

-- 
Justin

  9. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-22T23:03:08Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> On Mon, Jun 22, 2020 at 06:15:27PM -0400, Tom Lane wrote:
>> That seems like rather pointless micro-optimization really; the struct's
>> not *that* large.  But I have a different complaint now that I look at
>> this code: is it safe at all?  I see that the indname field is a pointer
>> to who-knows-where.  If it's possible in the first place for that to
>> change while this code runs, then what guarantees that we won't be
>> restoring a dangling pointer to freed memory?

> I'm not sure it addresses your concern, but we talked a bunch about safety
> starting here:
> https://www.postgresql.org/message-id/20200326150457.GB17431%40telsasoft.com
> ..and concluding with an explanation about CHECK_FOR_INTERRUPTS.

> 20200326150457.GB17431@telsasoft.com
> |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.

If we're relying on that, I'd replace the "save" action by an Assert that
indname is NULL, and the "restore" action by just assigning NULL again.
That eliminates all concern about whether the restored value is valid.

			regards, tom lane

  10. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Justin Pryzby <pryzby@telsasoft.com> — 2020-06-23T01:43:47Z

    On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> On 2020-06-22 15:43:11 -0500, Justin Pryzby wrote:
> > On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> > > I'm also uncomfortable with the approach of just copying all of
> > > LVRelStats in several places:
> > > 
> > > >  /*
> > > > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> > > >  	int			uncnt = 0;
> > > >  	TransactionId visibility_cutoff_xid;
> > > >  	bool		all_frozen;
> > > > +	LVRelStats	olderrinfo;
> > 
> > I guess the alternative is to write like
> > 
> > LVRelStats	olderrinfo = {
> > 	.phase = vacrelstats.phase,
> > 	.blkno = vacrelstats.blkno,
> > 	.indname = vacrelstats.indname,
> > };
> 
> No, I don't think that's a solution. I think it's wrong to have
> something like olderrinfo in the first place. Using a struct with ~25
> members to store the current state of three variables just doesn't make
> sense.  Why isn't this just a LVSavedPosition struct or something like
> that?

I'd used LVRelStats on your suggestion:
https://www.postgresql.org/message-id/20191211165425.4ewww2s5k5cafi4l%40alap3.anarazel.de
https://www.postgresql.org/message-id/20200120191305.sxi44cedhtxwr3ag%40alap3.anarazel.de

I understood the goal to be avoiding the need to add a new struct, when most
functions are already passed LVRelStats *vacrelstats.

But maybe I misunderstood.  (Also, back in January, the callback was only used
for scan-heap phase, so it's increased in scope several times).

Anyway, I put together some patches for discussion purposes.

-- 
Justin

  11. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-23T03:57:47Z

    On Tue, Jun 23, 2020 at 7:13 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> >
> > No, I don't think that's a solution. I think it's wrong to have
> > something like olderrinfo in the first place. Using a struct with ~25
> > members to store the current state of three variables just doesn't make
> > sense.  Why isn't this just a LVSavedPosition struct or something like
> > that?
>
> I'd used LVRelStats on your suggestion:
> https://www.postgresql.org/message-id/20191211165425.4ewww2s5k5cafi4l%40alap3.anarazel.de
> https://www.postgresql.org/message-id/20200120191305.sxi44cedhtxwr3ag%40alap3.anarazel.de
>
> I understood the goal to be avoiding the need to add a new struct, when most
> functions are already passed LVRelStats *vacrelstats.
>

Yeah, I think this is a good point against adding a separate struct.
I also don't think that we can buy much by doing this optimization.
To me, the current code looks good in this regard.

> But maybe I misunderstood.  (Also, back in January, the callback was only used
> for scan-heap phase, so it's increased in scope several times).
>
> Anyway, I put together some patches for discussion purposes.
>

Few comments for 0002-Add-assert-and-document-why-indname-is-safe
-----------------------------------------------------------------------------------------------------
- /* Free index name from any previous phase */
  if (errinfo->indname)
+ {
+ /*
+ * indname is only ever saved during lazy_vacuum_index(), which
+ * during which the phase information is not not further
+ * manipulated, until it's restored before returning from
+ * lazy_vacuum_index().
+ */
+ Assert(indname == NULL);
+
  pfree(errinfo->indname);
+ errinfo->indname = NULL;
+ }

It is not very clear that this is the place where we are saving the
state.  I think it would be better to do in the caller (ex. in before
statement olderrinfo = *vacrelstats; in lazy_vacuum_index()) where it
is clear that we are saving the state for later use.

I guess for the restore case we are already assigning NULL via
"errinfo->indname = indname ? pstrdup(indname) : NULL;" in
update_vacuum_error_info.  I think some more comments in the function
update_vacuum_error_info would explain it better.

0001-Rename-from-errcbarg, looks fine to me but we can see if others
have any opinion on the naming (especially changing  VACUUM_ERRCB* to
VACUUM_ERRINFO*).

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  12. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Andres Freund <andres@anarazel.de> — 2020-06-23T04:05:19Z

    Hi,

On 2020-06-22 20:43:47 -0500, Justin Pryzby wrote:
> On Mon, Jun 22, 2020 at 01:57:12PM -0700, Andres Freund wrote:
> > On 2020-06-22 15:43:11 -0500, Justin Pryzby wrote:
> > > On Mon, Jun 22, 2020 at 01:09:39PM -0700, Andres Freund wrote:
> > > > I'm also uncomfortable with the approach of just copying all of
> > > > LVRelStats in several places:
> > > > 
> > > > >  /*
> > > > > @@ -1580,9 +1648,15 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
> > > > >  	int			uncnt = 0;
> > > > >  	TransactionId visibility_cutoff_xid;
> > > > >  	bool		all_frozen;
> > > > > +	LVRelStats	olderrinfo;
> > > 
> > > I guess the alternative is to write like
> > > 
> > > LVRelStats	olderrinfo = {
> > > 	.phase = vacrelstats.phase,
> > > 	.blkno = vacrelstats.blkno,
> > > 	.indname = vacrelstats.indname,
> > > };
> > 
> > No, I don't think that's a solution. I think it's wrong to have
> > something like olderrinfo in the first place. Using a struct with ~25
> > members to store the current state of three variables just doesn't make
> > sense.  Why isn't this just a LVSavedPosition struct or something like
> > that?
> 
> I'd used LVRelStats on your suggestion:
> https://www.postgresql.org/message-id/20191211165425.4ewww2s5k5cafi4l%40alap3.anarazel.de
> https://www.postgresql.org/message-id/20200120191305.sxi44cedhtxwr3ag%40alap3.anarazel.de
> 
> I understood the goal to be avoiding the need to add a new struct, when most
> functions are already passed LVRelStats *vacrelstats.

> But maybe I misunderstood.  (Also, back in January, the callback was only used
> for scan-heap phase, so it's increased in scope several times).

I am only suggesting that where you save the old location, as currently
done with LVRelStats olderrinfo, you instead use a more specific
type. Not that you should pass that anywhere (except for
update_vacuum_error_info).

Greetings,

Andres Freund

  13. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-23T04:14:40Z

    Andres Freund <andres@anarazel.de> writes:
> I am only suggesting that where you save the old location, as currently
> done with LVRelStats olderrinfo, you instead use a more specific
> type. Not that you should pass that anywhere (except for
> update_vacuum_error_info).

As things currently stand, I don't think we need another struct
type at all.  ISTM we should hard-wire the handling of indname
as I suggested above.  Then there are only two fields to be dealt
with, and we could just as well save them in simple local variables.

If there's a clear future path to needing to save/restore more
fields, then maybe another struct type would be useful ... but
right now the struct type declaration itself would take more
lines of code than it would save.

			regards, tom lane

  14. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Justin Pryzby <pryzby@telsasoft.com> — 2020-06-23T13:53:54Z

    On Tue, Jun 23, 2020 at 12:14:40AM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I am only suggesting that where you save the old location, as currently
> > done with LVRelStats olderrinfo, you instead use a more specific
> > type. Not that you should pass that anywhere (except for
> > update_vacuum_error_info).
> 
> As things currently stand, I don't think we need another struct
> type at all.  ISTM we should hard-wire the handling of indname
> as I suggested above.  Then there are only two fields to be dealt
> with, and we could just as well save them in simple local variables.
> 
> If there's a clear future path to needing to save/restore more
> fields, then maybe another struct type would be useful ... but
> right now the struct type declaration itself would take more
> lines of code than it would save.

Updated patches for consideration.  I left the "struct" patch there to show
what it'd look like.

-- 
Justin

  15. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Andres Freund <andres@anarazel.de> — 2020-06-23T18:19:48Z

    Hi,

On 2020-06-23 00:14:40 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I am only suggesting that where you save the old location, as currently
> > done with LVRelStats olderrinfo, you instead use a more specific
> > type. Not that you should pass that anywhere (except for
> > update_vacuum_error_info).
> 
> As things currently stand, I don't think we need another struct
> type at all.  ISTM we should hard-wire the handling of indname
> as I suggested above.  Then there are only two fields to be dealt
> with, and we could just as well save them in simple local variables.

That's fine with me too.


> If there's a clear future path to needing to save/restore more
> fields, then maybe another struct type would be useful ... but
> right now the struct type declaration itself would take more
> lines of code than it would save.

FWIW, I started to be annoyed by this code when I was addding
prefetching support to vacuum, and wanted to change what's tracked
where. The number of places that needed to be touched was
disproportional.


Here's a *draft* for how I thought this roughly could look like. I think
it's nicer to not specify the exact saved state in multiple places, and
I think it's much clearer to use a separate function to restore the
state than to set a "fresh" state.

I've only applied a hacky fix for the way the indname is tracked, I
thought that'd best be discussed separately.

Greetings,

Andres Freund

  16. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-25T09:01:51Z

    On Tue, Jun 23, 2020 at 11:49 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2020-06-23 00:14:40 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > I am only suggesting that where you save the old location, as currently
> > > done with LVRelStats olderrinfo, you instead use a more specific
> > > type. Not that you should pass that anywhere (except for
> > > update_vacuum_error_info).
> >
> > As things currently stand, I don't think we need another struct
> > type at all.  ISTM we should hard-wire the handling of indname
> > as I suggested above.  Then there are only two fields to be dealt
> > with, and we could just as well save them in simple local variables.
>
> That's fine with me too.
>

I have looked at both the patches (using separate variables (by
Justin) and using a struct (by Andres)) and found the second one bit
better.

>
> > If there's a clear future path to needing to save/restore more
> > fields, then maybe another struct type would be useful ... but
> > right now the struct type declaration itself would take more
> > lines of code than it would save.
>
> FWIW, I started to be annoyed by this code when I was addding
> prefetching support to vacuum, and wanted to change what's tracked
> where. The number of places that needed to be touched was
> disproportional.
>
>
> Here's a *draft* for how I thought this roughly could look like. I think
> it's nicer to not specify the exact saved state in multiple places, and
> I think it's much clearer to use a separate function to restore the
> state than to set a "fresh" state.
>

I think this is a good idea and makes code look better.  I think it is
better to name new struct as LVSavedErrInfo instead of LVSavedPos.

> I've only applied a hacky fix for the way the indname is tracked, I
> thought that'd best be discussed separately.
>

I think it is better to use Tom's idea here to save and restore index
information in-place.  I have used Justin's patch with some
improvements like adding Asserts and initializing with NULL for
indname while restoring to make things unambiguous.

I have improved some comments in the code and for now, kept as two
patches (a) one for improving the error info for index (mostly
Justin's patch based on Tom's idea) and (b) the other to generally
improve the code in this area (mostly Andres's patch).

I have done some testing with both the patches and would like to do
more unless there are objections with these.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  17. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Justin Pryzby <pryzby@telsasoft.com> — 2020-06-26T01:55:17Z

    On Thu, Jun 25, 2020 at 02:31:51PM +0530, Amit Kapila wrote:
> I have looked at both the patches (using separate variables (by
> Justin) and using a struct (by Andres)) and found the second one bit
> better.

Thanks for looking.

> I have improved some comments in the code and for now, kept as two
> patches (a) one for improving the error info for index (mostly
> Justin's patch based on Tom's idea) and (b) the other to generally
> improve the code in this area (mostly Andres's patch).

And thanks for separate patchen :)

> I have done some testing with both the patches and would like to do
> more unless there are objections with these.

Comments:

>         * The index name is saved only during this phase and restored immediately

=> I wouldn't say "only" since it's saved during lazy_vacuum: index AND cleanup.

>update_vacuum_error_info(LVRelStats *errinfo, LVSavedErrInfo *oldpos, int phase,

=> You called your struct "LVSavedErrInfo" but the variables are still called
"pos".  I would call it olderrinfo or just old.

Also, this doesn't (re)rename the "cbarg" stuff that Alvaro didn't like, which
was my 0001 patch.

-- 
Justin

  18. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-26T03:49:38Z

    On Fri, Jun 26, 2020 at 7:25 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
>
> > I have done some testing with both the patches and would like to do
> > more unless there are objections with these.
>
> Comments:
>
> >         * The index name is saved only during this phase and restored immediately
>
> => I wouldn't say "only" since it's saved during lazy_vacuum: index AND cleanup.
>
> >update_vacuum_error_info(LVRelStats *errinfo, LVSavedErrInfo *oldpos, int phase,
>
> => You called your struct "LVSavedErrInfo" but the variables are still called
> "pos".  I would call it olderrinfo or just old.
>

Fixed both of the above comments. I used the variable name as saved_err_info.

> Also, this doesn't (re)rename the "cbarg" stuff that Alvaro didn't like, which
> was my 0001 patch.
>

If I am not missing anything then that change was in
lazy_cleanup_index and after this patch, it won't be required because
we are using a different variable name.

I have combined both the patches now.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  19. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-30T04:00:54Z

    On Fri, Jun 26, 2020 at 9:19 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 7:25 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> >
> > Comments:
> >
> > >         * The index name is saved only during this phase and restored immediately
> >
> > => I wouldn't say "only" since it's saved during lazy_vacuum: index AND cleanup.
> >
> > >update_vacuum_error_info(LVRelStats *errinfo, LVSavedErrInfo *oldpos, int phase,
> >
> > => You called your struct "LVSavedErrInfo" but the variables are still called
> > "pos".  I would call it olderrinfo or just old.
> >
>
> Fixed both of the above comments. I used the variable name as saved_err_info.
>
> > Also, this doesn't (re)rename the "cbarg" stuff that Alvaro didn't like, which
> > was my 0001 patch.
> >
>
> If I am not missing anything then that change was in
> lazy_cleanup_index and after this patch, it won't be required because
> we are using a different variable name.
>
> I have combined both the patches now.
>

I am planning to push this tomorrow if there are no further
suggestions/comments.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  20. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-07-02T03:37:10Z

    On Tue, Jun 30, 2020 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> >
> > If I am not missing anything then that change was in
> > lazy_cleanup_index and after this patch, it won't be required because
> > we are using a different variable name.
> >
> > I have combined both the patches now.
> >
>
> I am planning to push this tomorrow if there are no further
> suggestions/comments.
>

Pushed.  Now, coming back to the question of the back patch.  I see a
point in deferring this for 3-6 months or maybe more after PG13 is
released.  OTOH, this implementation is mainly triggered by issues
reported in this area and this doesn't seem to be a very invasive
patch which can cause some de-stabilization in back-branches. I am not
in a hurry to get this backpatched but still, it would be good if this
can be backpatched earlier as quite a few people (onlist and EDB
customers) have reported issues that could have been narrowed down if
this patch is present in back-branches.

It seems Alvaro and I are in favor of backpatch whereas Andres and
Justin seem to think it should be deferred until this change has seen
some real-world exposure.

Anyone else wants to weigh in?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  21. Re: Backpatch b61d161c14 (Introduce vacuum errcontext ...)

    Amit Kapila <amit.kapila16@gmail.com> — 2020-07-09T03:27:38Z

    On Thu, Jul 2, 2020 at 9:07 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 30, 2020 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > >
> > > If I am not missing anything then that change was in
> > > lazy_cleanup_index and after this patch, it won't be required because
> > > we are using a different variable name.
> > >
> > > I have combined both the patches now.
> > >
> >
> > I am planning to push this tomorrow if there are no further
> > suggestions/comments.
> >
>
> Pushed.  Now, coming back to the question of the back patch.  I see a
> point in deferring this for 3-6 months or maybe more after PG13 is
> released.  OTOH, this implementation is mainly triggered by issues
> reported in this area and this doesn't seem to be a very invasive
> patch which can cause some de-stabilization in back-branches. I am not
> in a hurry to get this backpatched but still, it would be good if this
> can be backpatched earlier as quite a few people (onlist and EDB
> customers) have reported issues that could have been narrowed down if
> this patch is present in back-branches.
>
> It seems Alvaro and I are in favor of backpatch whereas Andres and
> Justin seem to think it should be deferred until this change has seen
> some real-world exposure.
>
> Anyone else wants to weigh in?
>

Seeing no more responses, it seems better to defer this backpatch till
PG13 is out and we get some confidence in this functionality.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com