Andres Freund wrote:
> I do wonder if any of the other existing callers of HTSV are affected. I
> don't understand predicate.c well enough to be sure, but it looks to me
> like it'd could in theory lead to missed conflicts. Seems fairly
> unlikely to matter in practice though.
One place where the difference in the new logic would cause a change is
the tupleIsAlive bit in IndexBuildHeapScan(), when building unique
indexes. Right now if the tuple is inserted by a remote running
transaction, and updated by it, we return DELETE_IN_PROGRESS, so we set
tupleIsAlive = false; so we wouldn't block if we see a tuple with that
value elsewhere. If we instead return INSERT_IN_PROGRESS we set
tupleIsAlive = true, and we would block until the inserter is done.
I think it'd be better to ensure that DELETE_IN_PROGRESS is returned as
appropriate in the new XidIsInProgress(xmin) block.
> I have to say I really hate the amount of repetitive code inside the
> individual visibility routines. Obviously it's nothing backpatchable and
> may become moot to a certain degree with the CSN work, but those are
> pretty close to being unmaintainable.
One thing I'm now wondering while reading this code is those cases where
XMAX_INVALID is not set, but the value in Xmax is InvalidXid. We have
discussed these scenarios elsewhere and the conclusion seems to be that
they are rare but possible and therefore we should cater for them.
(IIRC the scenario is a tuple that is frozen without a full-page write
and no checksums; if we crash before writing the buffer, we would
recover the invalid value in Xmax but the hint bit would not become set.
I think this is not possible anymore after we tweaked the freezing
protocol.)
So instead of a check like
if (tuple->t_infomask & HEAP_XMAX_INVALID)
we would have something like
if (HeapTupleHeaderXmaxIsInvalid(tuple))
which checks both things -- and is easier to read, IMHO.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services