Re: Fixing a couple of buglets in how VACUUM sets visibility map bits

On Mon, Jan 2, 2023 at 1:32 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Sat, Dec 31, 2022 at 4:53 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> > The first patch makes sure that the snapshotConflictHorizon cutoff
> > (XID cutoff for recovery conflicts) is never a special XID, unless
> > that XID is InvalidTransactionId, which is interpreted as a
> > snapshotConflictHorizon value that will never need a recovery conflict
> > (per the general convention for snapshotConflictHorizon values
> > explained above ResolveRecoveryConflictWithSnapshot).
>
> Pushed this just now.
>
> Attached is another very simple refactoring patch for vacuumlazy.c. It
> makes vacuumlazy.c save the result of get_database_name() in vacrel,
> which matches what we already do with things like
> get_namespace_name().
>
> Would be helpful if I could get a +1 on
> v1-0002-Never-just-set-the-all-frozen-bit-in-VM.patch, which is
> somewhat more substantial than the others.

I feel that you should at least have a reproducer for these problems
posted to the thread, and ideally a regression test, before committing
things. I think it's very hard to understand what the problems are
right now.

I don't particularly have a problem with the idea of 0001, because if
we use InvalidTransactionId to mean that there cannot be any
conflicts, we do not need FrozenTransactionId to mean the same thing.
Picking one or the other makes sense. Perhaps we would need two values
if we both needed a value that meant "conflict with nothing" and also
a value that meant "conflict with everything," but in that case I
suppose we would want FrozenTransactionId to be the one that meant
conflict with nothing, since it logically precedes all other XIDs, and
conflicts are with XIDs that precede the value in the record. However,
I don't find the patch very clear, either. It doesn't update any
comments, not even this one:

/*
* It's possible that we froze tuples and made the page's XID cutoff
* (for recovery conflict purposes) FrozenTransactionId. This is okay
* because visibility_cutoff_xid will be logged by our caller in a
* moment.
*/
- Assert(cutoff == FrozenTransactionId ||
+ Assert(!TransactionIdIsValid(cutoff) ||
cutoff == prunestate->visibility_cutoff_xid);

Isn't the comment now incorrect as a direct result of the changes in the patch?

As for 0002, I agree that it's bad if we can get into a state where
the all-frozen bit is set and the all-visible bit is not. I'm not
completely sure what concrete harm that will cause, but it does not
seem good. But I also *entirely* agree with Andres that patches should
run around adjusting nearby code - e.g. variable names - in ways that
aren't truly necessary. That just makes life harder, not only for
anyone who wants to review the patch now, but also for future readers
who may need to understand what the patch changed and why.

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