Re: error context for vacuum to include block number

On Fri, Mar 20, 2020 at 11:24:25AM +0530, Amit Kapila wrote:
> On Fri, Mar 20, 2020 at 5:59 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> 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?

It's not needed but a common convention to avoid needing a two-line patch in
order to add a line at the end, like:

- foo
+ foo,
+ bar

> 2. update_vacuum_error_cbarg(vacrelstats, VACUUM_ERRCB_PHASE_SCAN_HEAP, InvalidBlockNumber, NULL);
>
> Why do we need to call update_vacuum_error_cbarg at the above place
> after we have added a new one inside for.. loop?

If we're going to update the error_context_stack global point to our callback,
without our vacrelstats arg, it'd better be initialized. I changed to do
vacrelstats->phase = UNKNOWN after its allocation in heap_vacuum_rel().
That matches parallel_vacuum_main().

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

I did this:
/* Revert back to the old phase information for error traceback */

> 5. Subject: [PATCH v28 3/5] Drop reltuples
>
> Is this patch directly related to the main patch (vacuum errcontext to
> show block being processed) or is it an independent improvement of
> code?

It's a cleanup after implementing the new feature. I left it as a separate
patch to make review easier of the essential patch and of the cleanup.
See here:
https://www.postgresql.org/message-id/CA%2Bfd4k4JA3YkP6-HUqHOqu6cTGqqZUhBfsMqQ4WXkD0Y8uotUg%40mail.gmail.com

> 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?

The context is that there was a request to add err context for (yet another)
phase, TRUNCATE. But I insisted on adding it to prefetch, too, since it does
ReadBuffer. But there was an objection that the error might be misleading if
it said "while truncating" but it was actually "prefetching to truncate".

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

They were separate since it's the most-recently added part, and (as now)
there's still discussion about it.

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

Is it possible to make an corrupted index which errors during scan during
regress tests ?

Thanks for looking.

--
Justin