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

On Wed, Oct 6, 2021 at 3:47 PM Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> I totally agree that the amcheck functions cannot prove the absence of corruption.
>
> I prefer not to even use language about proving the presence of corruption when discussing pg_amcheck.

I agree that it doesn't usually help. But sometimes it is important.

> > This seems to come up a lot because at various points you seem to be
> > concerned about introducing specific imperfections. But it's not like
> > your starting point was ever perfection, or ever could be.

> From the point of view of detecting corruptions, I agree that it never could be. But I'm not talking about that. I'm talking about whether pg_amcheck issues all the commands that it is supposed to issue. If I work for Daddy Warbucks and he gives me 30 classic cars to take to 10 different mechanics, I can do that job perfectly even if the mechanics do less than perfect work. If I leave three cars in the driveway, that's on me. Likewise, it's not on pg_amcheck if the checking functions can't do perfect work, but it is on pg_amcheck if it doesn't issue all the expected commands. But later on in this email, it appears we don't have any remaining disagreements about that. Read on....

When you say "expected commands", I am entitled to ask: expected by
whom, based on what underlying principle? Similarly, when you suggest
that amcheck should directly deal with !indisvalid indexes itself, it
naturally leads to a tricky discussion of the precise definition of a
relation (in particular in the presence of REINDEX CONCURRENTLY), and
the limits of what is possible with amcheck. That's just where the
discussion has to go.

You cannot say that amcheck must (say) "directly deal with indisvalid
indexes", without at least saying why. pg_amcheck works by querying
pg_class, finding relations to verify. There is no way that that can
work that allows pg_amcheck to completely sidestep these awkward
questions -- just like with pg_dump. There is no safe neutral starting
point for a program like that.

> > I can
> > always describe a scenario in which amcheck misses real corruption --
> > a scenario which may be very contrived. So the mere fact that some new
> > theoretical possibility of corruption is introduced by some action
> > does not in itself mean much. We're dealing with that constantly, and
> > always will be.
>
> I wish we could stop discussing this. I really don't think this ticket has anything to do with how well or how poorly or how completely the amcheck functions work.

It's related to !indisvalid indexes. At one point you were concerned
about not having coverage of them in certain scenarios. Which is fine.
But the inevitable direction of that conversation is towards
fundamental definitional questions.

Quite happy to drop all of this now, though.

> Ok, excellent, that was probably the only thing that had me really hung up. I thought you were still asking for pg_amcheck to filter out the --parent-check option when in recovery, but if you're not asking for that, then I might have enough to go on now.

Sorry about that. I realized my mistake (not specifically addressing
pg_is_in_recovery()) after I hit "send", and should have corrected the
record sooner.

> I was using "downgrading" to mean downgrading from bt_index_parent_check() to bt_index_check() when pg_is_in_recovery() is true, but you've clarified that you're not requesting that downgrade, so I think we've now gotten past the last sticking point about that whole issue.

Right. I never meant anything like making a would-be
bt_index_parent_check() call into a bt_index_check() call, just
because of the state of the system (e.g., it's in recovery). That
seems awful, in fact.

> will complain if no tables match, giving the user the opportunity to notice that they spelled "accounting" wrong. If there happens to be a table named "xyzacountngo", and that matches, too bad. There isn't any way pg_amcheck can be responsible for that. But if there is a temporary table named "xyzacountngo" and that gets skipped because it's a temp table, I don't know what feedback the user should get. That's a thorny user interfaces question, not a corruption checking question, and I don't think you need to weigh in unless you want to. I'll most likely go with whatever is the simplest to code and/or most similar to what is currently in the tree, because I don't see any knock-down arguments one way or the other.

I agree with you that this is a UI thing, since in any case the temp
table is pretty much "not visible to pg_amcheck". I have no particular
feelings about it.

Thanks
--
Peter Geoghegan