Re: error context for vacuum to include block number

On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Mar 19, 2020 at 03:29:31PM -0500, Justin Pryzby wrote:
> > I was going to suggest that we could do that by passing in a pointer to a local
> > variable "LVRelStats olderrcbarg", like:
> > |        update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
> > |                                  blkno, NULL, &olderrcbarg);
> >
> > and then later call:
> > |update_vacuum_error_cbarg(vacrelstats, olderrcbarg.phase,
> > |                                       olderrcbarg.blkno,
> > |                                       olderrcbarg.indname,
> > |                                       NULL);
> >
> > I implemented it in a separate patch, but it may be a bad idea, due to freeing
> > indname.  To exercise it, I tried to cause a crash by changing "else if
> > (errcbarg->indname)" to "if" without else, but wasn't able to cause a crash,
> > probably just due to having a narrow timing window.
>
> I realized it was better for the caller to just assign the struct on its own.
>

That makes sense.  I have a few more comments:

1.
+ VACUUM_ERRCB_PHASE_INDEX_CLEANUP,
+} errcb_phase;

Why do you need a comma after the last element in the above enum?

2.
+ /* Setup error traceback support for ereport() */
+ update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP,
+ InvalidBlockNumber, NULL);
+ errcallback.callback = vacuum_error_callback;
+ errcallback.arg = vacrelstats;
+ errcallback.previous = error_context_stack;
+ error_context_stack = &errcallback;

Why do we need to call update_vacuum_error_cbarg at the above place
after we have added a new one inside for.. loop?

3.
+ * free_oldindex is true if the previous "indname" should be freed.  It must be
+ * false if the caller has copied the old LVRelSTats,

/LVRelSTats/LVRelStats

4.
/* Clear the error traceback phase */
  update_vacuum_error_cbarg(vacrelstats,
-   VACUUM_ERRCB_PHASE_UNKNOWN, InvalidBlockNumber,
-   NULL);
+   olderrcbarg.phase,
+   olderrcbarg.blkno,
+   olderrcbarg.indname,
+   true);

At this and similar places, change the comment to something like:
"Reset the old phase information for error traceback".

5.
Subject: [PATCH v28 3/5] Drop reltuples

---
 src/backend/access/heap/vacuumlazy.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Is this patch directly related to the main patch (vacuum errcontext to
show block being processed) or is it an independent improvement of
code?

6.
[PATCH v28 4/5] add callback for truncation

+ VACUUM_ERRCB_PHASE_TRUNCATE,
+ VACUUM_ERRCB_PHASE_TRUNCATE_PREFETCH,

Do we really need separate phases for truncate and truncate_prefetch?
We have only one phase for a progress update, similarly, I think
having one phase for error reporting should be sufficient.  It will
also reduce the number of places where we need to call
update_vacuum_error_cbarg.  I think we can set
VACUUM_ERRCB_PHASE_TRUNCATE before count_nondeletable_pages and reset
it at the place you are doing right now in the patch.

7. Is there a reason to keep the truncate phase patch separate from
the main patch? If not, let's merge them.

8. Can we think of some easy way to add tests for this patch?

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