Re: BUG #17212: pg_amcheck fails on checking temporary relations

On Wed, Oct 6, 2021 at 12:33 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> To me, it doesn't matter which specific option we're talking about. If
> I tell pg_amcheck to pass a certain flag to the underlying functions,
> then it should do that. If the behavior needs to be changed, it should
> be changed in those underlying functions, not in pg_amcheck.

I agree, with the stipulation that the caller (in this case
pg_amcheck) is required to know certain basic things about the
relation in order to get useful behavior. For example, if you use
bt_index_check() with a GIN index, you're going to get an error. That
much we can all agree on, I'm sure.

Where I might go further than you or Mark (not sure) is on this: I
also think that it's the caller's job to not call the functions with
temp relations, or (in the case of the index verification stuff) with
!indisready or !indisvalid relations. I believe that these ought to
also be treated as basic questions about the relation, just like in my
GIN example. But that's as far as I go here.

> If we
> start putting some of the intelligence into amcheck itself, and some
> of it into pg_amcheck, I think it's going to become confusing and in
> fact I think it's going to become unreliable, at least from the user
> point of view. People will get confused if they run pg_amcheck and get
> some result (either pass or fail) and then they do the same thing with
> pg_amcheck and get a different result.

Agreed on all that.

> > --parent-check does present us with the question of what to do in Hot
> > Standby mode, where it will surely fail (because it requires a
> > relation level ShareLock, etc). But I actually don't think it's
> > complicated: we must throw an error, because it's fundamentally not
> > something that will ever work (with any index). Whether the error
> > comes from pg_amcheck or amcheck proper doesn't seem important to me.
>
> That detail, to me, is actually very important.

I believe that you actually reached the same conclusion, though: we
should let it just fail. That makes this question easy.

> > I think it's pretty clear that verify_heapam.c (from amcheck proper)
> > should just follow verify_nbtree.c when directly invoked against an
> > unlogged index in Hot Standby. That is, it should assume that the
> > relation has no storage, but still "verify" it conceptually. Just show
> > a NOTICE about it. Assume no storage to verify.
>
> I haven't checked the code, but that sounds right. I interpret this to
> mean that the different sub-parts of amcheck don't handle this case in
> ways that are consistent with each other, and that seems wrong. We
> should make them consistent.

I agree that nbtree and heapam verification ought to agree here. But
my point was just that this behavior just makes sense: what we have is
something just like an empty relation.

> > Finally, there is the question of what happens inside pg_amcheck (not
> > amcheck proper) deals with unlogged relations in Hot Standby mode.
> > There are two reasonable options: it can either "verify" the indexes
> > (actually just show those NOTICE messages), or skip them entirely. I
> > lean towards the former option, on the grounds that I don't think it
> > should be special-cased. But I don't feel very strongly about it.
>
> I like having it do this:
>
> ereport(NOTICE,
> (errcode(ERRCODE_READ_ONLY_SQL_TRANSACTION),
> errmsg("cannot verify unlogged index \"%s\"
> during recovery, skipping",
> RelationGetRelationName(rel))));
>
> I think the fewer decisions the command-line tool makes, the better.
> We should put the policy decisions in amcheck itself.

Wait, so you're arguing that we should change amcheck (both nbtree and
heapam verification) to simply reject unlogged indexes during
recovery?

That doesn't seem like very friendly or self-consistent behavior. At
first (in hot standby) it fails. As soon as the DB is promoted, we'll
then also have no on-disk storage for the same unlogged relation, but
now suddenly it's okay, just because of that. I find it far more
logical to just assume that there is no relfilenode storage to check
when in hot standby.

This isn't the same as the --parent-check thing at all, because that's
about an implementation restriction of Hot Standby. Whereas this is
about the physical index structure itself.

--
Peter Geoghegan