Re: BUG #17245: Index corruption involving deduplicated entries

On Thu, Nov 4, 2021 at 3:07 PM Andres Freund <andres@anarazel.de> wrote:
> I'm not quite sure either - I'm worried about followon corruption based on the
> bug. That'll be much more likely be detected with something like this in
> place.

True, but the additional hardening I added to nbtree in commit
a5213adf is definitely worth something. Sure, tools like amcheck
disappointed when it came to detecting the parallel VACUUM bug -- I
don't deny that. To some degree the nbtree side hardening will be a
little like that too. But let's not overstate it; pg_amcheck was still
pretty useful. It has a decent chance of detecting that there are
certain problems with one or more indexes.

> But it's also easy to get some subtlety wrong...

I'm not worried about incorrectly identifying TIDs as corrupt in this
hardening patch. I'm more worried about the patch throwing ERRORs in
cases where it's arguably not really necessary -- which is not a
problem for the nbtree side hardening we have already. With this new
hardening, the insert or update that we end up throwing an error for
might be pretty far removed from the ERROR. And so it seems like
something that would be better left till Postgres 15.

Of course I don't believe for a second that ignorance is bliss. At the
same time, I don't want to act rashly.

> I'd make the error paths unlikely().

I was under the impression that all ERRORs were unlikely(), in effect.
At least following David Rowley's work in commit 913ec71d -- the
"ereport(ERROR) pg_attribute_cold() shim" patch. But now that I think
about it some more, I imagine that compilers are free to interpret
these any way they want. They're certainly not officially equivalent.
And so it couldn't hurt to add some unlikely()s, for good luck. This
is a pretty tight, performance sensitive loop, after all.

> These I'd definitely only do in HEAD.

Works for me. I see the changes to pruneheap.c as developer
documentation more than anything else. It's a nice way to explain what
is possible, without leaving much room for misunderstanding.

> I was a bit confused because I initially read this this as the first tuple
> redirect *to* can't be a heap-only tuple. Perhaps this could be rephrased a
> bit ("redirecting item"?).

That makes sense. What do you think of this?:

        /*
         * The item that we're about to set as an LP_REDIRECT (the 'from'
         * item) must point to an existing item (the 'to' item) that is
         * already a heap-only tuple.  There can be at most one LP_REDIRECT
         * item per HOT chain.
         *
         * We need to keep around an LP_REDIRECT item (after original
         * non-heap-only root tuple gets pruned away) so that it's always
         * possible for VACUUM to easily figure out what TID to delete from
         * indexes when an entire HOT chain becomes dead.  A heap-only tuple
         * can never become LP_DEAD; an LP_REDIRECT item or a regular heap
         * tuple can.
         */
        tolp = PageGetItemId(page, tooff);
        Assert(ItemIdHasStorage(tolp) && ItemIdIsNormal(tolp));
        htup = (HeapTupleHeader) PageGetItem(page, tolp);
        Assert(HeapTupleHeaderIsHeapOnly(htup));

(I changed the first paragraph, not the second. Just showing
everything together for your convenience.)

-- 
Peter Geoghegan