From: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: error context for vacuum to include block number |
Date: | 2019-12-13 03:08:31 |
Message-ID: | 20191213030831.GT2082@telsasoft.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Dec 11, 2019 at 12:33:53PM -0300, Alvaro Herrera wrote:
> On 2019-Dec-11, Justin Pryzby wrote:
> > + cbarg.blkno = 0; /* Not known yet */
> Shouldn't you use InvalidBlockNumber for this initialization?
..
> > pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
> > + cbarg.blkno = blkno;
> I would put this before pgstat_progress_update_param, just out of
> paranoia.
..
> Lose this hunk?
Addressed those.
> Or do you intend that this is going to be used for lazy_vacuum_heap too?
> Maybe it should.
Done in a separate patch.
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.
> > + vacuum_error_callback_arg cbarg;
> Not a fan of "cbarg", too generic.
..
> I think this will misattribute errors that happen when in the:
Probably right. Attached should address it.
On Wed, Dec 11, 2019 at 08:54:25AM -0800, Andres Freund wrote:
> > +typedef struct
> > +{
> > + char *relname;
> > + char *relnamespace;
> > + BlockNumber blkno;
> > +} vacuum_error_callback_arg;
>
> 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.
> Not a fan of "cbarg", too generic.
> I think this will misattribute errors that happen when in the:
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);
Justin
Attachment | Content-Type | Size |
---|---|---|
v4-0001-Rename-buf-to-avoid-shadowing-buf-of-another-type.patch | text/x-diff | 2.7 KB |
v4-0002-Remove-redundant-call-to-vacuum-progress.patch | text/x-diff | 977 bytes |
v4-0003-deduplication.patch | text/x-diff | 5.6 KB |
v4-0004-vacuum-errcontext-to-show-block-being-processed.patch | text/x-diff | 3.6 KB |
v4-0005-add-errcontext-callback-in-lazy_vacuum_heap-too.patch | text/x-diff | 1.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro Horiguchi | 2019-12-13 04:07:22 | Re: archive status ".ready" files may be created too early |
Previous Message | Thomas Munro | 2019-12-13 02:59:32 | Re: On disable_cost |