Re: error context for vacuum to include block number

On Thu, Dec 12, 2019 at 09:08:31PM -0600, Justin Pryzby wrote:
> On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:
>> Hm, wonder if could be worthwhile to not use a separate struct here, but
>> instead extend one of the existing structs to contain the necessary
>> information. Or perhaps have one new struct with all the necessary
>> information. There's already quite a few places that do
>> get_namespace_name(), for example.
>
> Didn't find a better struct to use yet.

Yes, I am too wondering what Andres has in mind here. It is not like
you can use VacuumRelation as the OID of the relation may not have
been stored.

> On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:>
> I think that's addressed after deduplicating in attached.
>
> Deduplication revealed 2nd progress call, which seems to have been included
> redundantly at c16dc1aca.
>
> - /* Remove tuples from heap */
> - pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
> - PROGRESS_VACUUM_PHASE_VACUUM_HEAP);

What is the purpose of 0001 in the context of this thread? One could
say the same about 0002 and 0003. Anyway, you are right with 0002 as
the progress value for PROGRESS_VACUUM_PHASE gets updated twice in a
row with the same value. So let's clean up that.

The refactoring in 0003 is interesting, so I would be tempted to merge
it. Now you have one small issue in it:
- /*
- * Forget the now-vacuumed tuples, and press on, but be careful
- * not to reset latestRemovedXid since we want that value to be
- * valid.
- */
+ lazy_vacuum_heap_index(onerel, vacrelstats, Irel, nindexes, indstats);
vacrelstats->num_dead_tuples = 0;
- vacrelstats->num_index_scans++;
You are moving this comment within lazy_vacuum_heap_index, but it
applies to num_dead_tuples and not num_index_scans, no? You should
keep the incrementation of num_index_scans within the routine though.
--
Michael