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

On Wed, Oct 6, 2021 at 2:56 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> --heapallindexed doesn't complicate things for us at all. It changes
> nothing about the locking considerations. It's just an additive thing,
> some extra checks with the same basic underlying requirements. Maybe
> you meant to say --parent-check, not --heapallindexed?

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. 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.

> --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 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.

> 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.

--
Robert Haas
EDB: http://www.enterprisedb.com