Re: XMAX_LOCK_ONLY and XMAX_COMMITTED (fk/multixact code)

On 2020-Aug-20, Jeremy Schneider wrote:

> While working with Nathan Bossart on an extension, we came across an
> interesting quirk and possible inconsistency in the PostgreSQL code
> around infomask flags. I'd like to know if there's something I'm
> misunderstanding here or if this really is a correctness/robustness gap
> in the code.
>
> At the root of it is the relationship between the XMAX_LOCK_ONLY and
> XMAX_COMMITTED infomask bits.

I spent a lot of time wondering about XMAX_COMMITTED. It seemed to me
that it would make no sense to have xacts that were lock-only yet have
XMAX_COMMITTED set; so I looked hard to make sure no place would ever
cause such a situation to arise. However, I still made my best effort
to make the code cope with such a combination correctly if it ever
showed up.

And I may have missed assumptions such as this one, even if they seem
obvious in retrospect, such as in compute_new_xmax_infomask (which, as
you'll notice, changed considerably from what was initially committed):

> But then there's one place in heapam.c where an assumption appears that
> this state will never happen - the compute_new_xmax_infomask() function:
>
> https://github.com/postgres/postgres/blob/9168793d7275b4b318c153d607fba55d14098c19/src/backend/access/heap/heapam.c#L4800
>
> else if (old_infomask & HEAP_XMAX_COMMITTED)
> {
> ...
> if (old_infomask2 & HEAP_KEYS_UPDATED)
> status = MultiXactStatusUpdate;
> else
> status = MultiXactStatusNoKeyUpdate;
> new_status = get_mxact_status_for_lock(mode, is_update);
> ...
> new_xmax = MultiXactIdCreate(xmax, status, add_to_xmax, new_status);
>
> When that code sees XMAX_COMMITTED, it assumes the xmax can't possibly
> be LOCK_ONLY and it sets the status to something sufficiently high
> enough to guarantee that ISUPDATE_from_mxstatus() returns true. That
> means that when you try to update this tuple and
> compute_new_xmax_infomask() is called with an "update" status, two
> "update" statuses are sent to MultiXactIdCreate() which then fails
> (thank goodness) with the error "new multixact has more than one
> updating member".
>
> https://github.com/postgres/postgres/blob/cd5e82256de5895595cdd99ecb03aea15b346f71/src/backend/access/transam/multixact.c#L784

Have you ever observed this error case hit? I've never seen a report of
that.

> The UpdateXmaxHintBits() code to always set the INVALID bit wasn't in
> any patches on the mailing list but it was committed and it seems to
> have worked very well. As a result it seems nearly impossible to get
> into the state where you have both XMAX_LOCK_ONLY and XMAX_COMMITTED
> bits set; thus it seems we've avoided problems in
> compute_new_xmax_infomask().

Phew.

(I guess I should fully expect that somebody, given sufficient time,
would carefully inspect the committed code against submitted patches ...
especially given that I do such inspections myself.)

> But nonetheless it seems worth making the code more robust by having the
> compute_new_xmax_infomask() function handle this state correctly just as
> the visibility code does. It should only require a simple patch like
> this (credit to Nathan Bossart):
>
> diff --git a/src/backend/access/heap/heapam.c
> b/src/backend/access/heap/heapam.c
> index d881f4cd46..371e3e4f61 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -4695,7 +4695,9 @@ compute_new_xmax_infomask(TransactionId xmax,
> uint16 old_infomask,
> l5:
> new_infomask = 0;
> new_infomask2 = 0;
> - if (old_infomask & HEAP_XMAX_INVALID)
> + if (old_infomask & HEAP_XMAX_INVALID ||
> + (old_infomask & HEAP_XMAX_COMMITTED &&
> + HEAP_XMAX_IS_LOCKED_ONLY(old_infomask)))
> {
> /*
> * No previous locker; we just insert our own TransactionId.

We could do this in stable branches, if there were any reports that
this inconsistency is happening in real world databases.

> Alternatively, if we don't want to take this approach, then I'd argue
> that we should update README.tuplock to explicitly state that
> XMAX_LOCK_ONLY and XMAX_COMMITTED are incompatible (just as it already
> states for HEAP_XMAX_IS_MULTI and HEAP_XMAX_COMMITTED) and clean up the
> code in heapam_visibility.c for consistency.

Yeah, I like this approach better for the master branch; not just clean
up as in remove the cases that handle it, but also actively elog(ERROR)
if the condition ever occurs (hopefully with some known way to fix the
problem; maybe by "WITH tup AS (DELETE FROM tab WHERE .. RETURNING *)
INSERT * INTO tab FROM tup" or similar.)

> Might be worth adding a note to README.tuplock either way about
> valid/invalid combinations of infomask flags. Might help avoid some
> confusion as people approach the code base.

Absolutely.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services