From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> |
Cc: | 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-01-24 19:21:55 |
Message-ID: | 20200124192155.GN13621@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for reviewing
On Wed, Jan 22, 2020 at 05:37:06PM +0900, Masahiko Sawada wrote:
> I'm not sure it's worth to have patches separately but I could apply
The later patches expanded on the initial scope, and to my understanding the
1st callback is desirable but the others are maybe still at question.
> 1. * The comment should be updated as we use both relname and
> relnamespace for ereporting.
>
> * We can leave both blkno and stage that are used only for error
> context reporting put both relname and relnamespace to top of
> LVRelStats.
Updated in the 0005 - still not sure if that patch will be desirable, though.
Also, I realized that it's we cannot use vacrelstats->blkno instead of local
blkno variable, since vacrelstats->blkno is used simultaneously by scan heap
and vacuum heap).
> * The 'stage' is missing to support index cleanup.
But the callback isn't used during index cleanup ?
> * It seems to me strange that only initialization of latestRemovedXid
> is done after error callback initialization.
Yes, that was silly - I thought it was just an artifact of diff's inability to
express rearranged code any better.
> * Maybe we can initialize relname and relnamespace in heap_vacuum_rel
> rather than in lazy_scan_heap. BTW variables of vacrelstats are
> initialized different places: some of them in heap_vacuum_rel and
> others in lazy_scan_heap. I think we can gather those that can be
> initialized at that time to heap_vacuum_rel.
I think that's already true ? But topic for a separate patch, if not.
> 3. Maybe we can do like:
done
> 4. These comments can be merged like:
done
> 5. Why do we need to initialize blkno in spite of not using?
removed
> 6.
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> * 'vacrelstats' would be a better name than 'cbarg'.
Really? I'd prefer to avoid repeating long variable three times:
vacrelstats->blkno, vacrelstats->relnamespace, vacrelstats->relname);
> * In index vacuum, how about "while vacuuming index \"%s.%s\""?
Yes. I'm still unclear if this is useful without a block number, or why it
wouldn't be better to write DEBUG1 log with index name before vacuuming each.
Justin
Attachment | Content-Type | Size |
---|---|---|
v13-0001-vacuum-errcontext-to-show-block-being-processed.patch | text/x-diff | 4.0 KB |
v13-0002-add-errcontext-callback-in-lazy_vacuum_heap-too.patch | text/x-diff | 1.8 KB |
v13-0003-Add-vacrelstats.stage-and-distinct-context-messa.patch | text/x-diff | 2.0 KB |
v13-0004-errcontext-for-lazy_vacuum_index.patch | text/x-diff | 2.3 KB |
v13-0005-Avoid-extra-calls-like-GetRelationName.patch | text/x-diff | 7.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2020-01-24 19:39:17 | Re: [Proposal] Global temporary tables |
Previous Message | Alvaro Herrera | 2020-01-24 19:08:24 | Re: making the backend's json parser work in frontend code |