Re: Maybe don't process multi xmax in FreezeMultiXactId() if it is already marked as invalid?

On Wed, Jun 19, 2024 at 1:39 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> FWIW I don't think HEAP_XMAX_INVALID as purely a hint.
> HEAP_XMAX_COMMITTED is a hint, for sure, as is HEAP_XMIN_COMMITTED on
> its own; but as far as I recall, the INVALID flags must persist once
> set.

Seems we disagree on some pretty fundamental things in this area, then.

> Consider the HEAP_XMIN_COMMITTED+ HEAP_XMIN_INVALID combination,
> which we use to represent HEAP_XMIN_FROZEN; if that didn't persist, we'd
> have a pretty serious data corruption issue, because we don't reset the
> Xmin field when freezing the tuple.

That's definitely true, but that's a case where the setting happens during a
WAL-logged operation. It doesn't involve HEAP_XMAX_INVALID at all.

FWIW I also don't think that even HEAP_XMIN_INVALID should be
considered anything more than a hint when it appears on its own. Only
HEAP_XMIN_FROZEN (by which I mean HEAP_XMIN_COMMITTED +
HEAP_XMIN_INVALID) are non-hint xact status infomask bits.

It's slightly annoying that we have this HEAP_XMIN_FROZEN case where a
hint bit isn't just a hint, but that's just a historical detail. And I
suppose that the same thing can be said of HEAP_XMAX_IS_MULTI itself
(we have no other way of distinguishing a Multi from an Xid, so
clearly that also has to be treated as a persistent non-hint by everybody).

> So if we fail to keep the flag, the
> tuple is no longer frozen.  (My point here is that some infomask bits
> are hints, but not all them are only hints.)   So XMAX_INVALID gives
> certainty that the Xmax value must not be read.

"Certainty" seems far too strong here.

> That is to say, I think
> there are (or there were) situations in which we set the bit but don't
> bother to reset the actual Xmax field.

I'm sure that that's true, but that doesn't seem at all equivalent to
what you said about XMAX_INVALID "giving certainty" about the tuple.

> We should never try to read the
> Xmax flag if the bit is set.

But that's exactly what FreezeMultiXactId does. It doesn't pay
attention to XMAX_INVALID (only to !MultiXactIdIsValid()).

Yura is apparently arguing that FreezeMultiXactId should notice
XMAX_INVALID and then tell its caller to "FRM_INVALIDATE_XMAX". That
does seem like a valid optimization. But if we were to do that then
we'd likely do it in a way that still resulted in ordinary processing
of the multi (it would not work by immediately setting
"FRM_INVALIDATE_XMAX" in the !MultiXactIdIsValid() path). That
approach to the optimization makes the most sense, because we'd likely
want to preserve the existing FreezeMultiXactId sanity checks.

> I think the problem being investigated in this thread is that
> HEAP_XMAX_IS_MULTI is being treated as persistent, that is, it can only
> be set if the xmax is not invalid, but apparently that's not always the
> case (or we wouldn't be having this conversation).

A multixact/HEAP_XMAX_IS_MULTI xmax doesn't start out as invalid.
Treating HEAP_XMAX_IS_MULTI as persistent doesn't mean that we should
treat XMAX_INVALID as consistent. In particular, XMAX_INVALID isn't
equivalent to !MultiXactIdIsValid() (you can make a similar statement
about xmax XIDs).


--
Peter Geoghegan