Re: error context for vacuum to include block number

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Justin Pryzby <pryzby(at)telsasoft(dot)com>
Cc: Masahiko Sawada <masahiko(dot)sawada(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-02-28 00:09:42
Message-ID: 20200228000942.GA16156@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2020-Feb-27, Justin Pryzby wrote:

> Originally, the patch only supported "scanning heap", and set the callback
> strictly, to avoid having callback installed when calling other functions (like
> vacuuming heap/indexes).
> Then incrementally added callbacks in increasing number of places. We only
> need one errcontext. And possibly you're right that the callback could always
> be in place (?). But what about things like vacuuming FSM ? I think we'd need
> another "phase" for that (or else invent a PHASE_IGNORE to do nothing). Would
> VACUUM_FSM be added to progress reporting, too? We're also talking about new

I think we should use a separate enum. It's simple enough, and there's
no reason to use the same enum for two different things if it seems to
complicate matters.

> Regarding the cbarg, at one point I took a suggestion from Andres to use the
> LVRelStats struct. I got rid of that since I didn't like sharing "blkno"
> between heap scanning and heap vacuuming, and needs to be reset when switching
> back to scanning heap. I experimented now going back to that now. The only
> utility is in having an single allocation of relname/space.

I'm unsure about reusing that struct. Not saying don't do it, just ...
unsure. It possibly has other responsibilities.

I don't think there's a reason to keep 0002 separate.

Regarding this,

> + if (BlockNumberIsValid(cbarg->blkno))
> + errcontext("while vacuuming block %u of relation \"%s.%s\"",
> + cbarg->blkno, cbarg->relnamespace, cbarg->relname);
> + break;

I think you should still call errcontext() when blkno is invalid. In
fact, just remove the "if" line altogether and let it show whatever
value is there. It should work okay. We don't expect the value to be
invalid anyway.

Maybe it would make sense to make the LVRelStats struct members be char
arrays rather than pointers. Then you memcpy() or strlcpy() them
instead of palloc/free.

Please don't cuddle your braces.

Álvaro Herrera
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2020-02-28 00:11:14 Re: ALTER INDEX fails on partitioned index
Previous Message Tom Lane 2020-02-27 23:32:17 Re: Rethinking opclass member checks and dependency strength