Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination

On Mon, Feb 1, 2021 at 2:05 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sun, Jan 24, 2021 at 9:31 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > On 2021-Jan-24, Julien Rouhaud wrote:
> >
> > > + /*
> > > + * Do not allow tuples with invalid combinations of hint bits to be placed
> > > + * on a page. These combinations are detected as corruption by the
> > > + * contrib/amcheck logic, so if you disable one or both of these
> > > + * assertions, make corresponding changes there.
> > > + */
> > > + Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
> > > + (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)));
> > >
> > >
> > > I attach a simple self contained script to reproduce the problem, the last
> > > UPDATE triggering the Assert.
> > >
> > > I'm not really familiar with this part of the code, so it's not exactly clear
> > > to me if some logic is missing in compute_new_xmax_infomask() /
> > > heap_prepare_insert(), or if this should actually be an allowed combination of
> > > hint bit.
> >
> > Hmm, it's probably a bug in compute_new_xmax_infomask. I don't think
> > the combination is sensible.
> >
>
> If we see the logic of GetMultiXactIdHintBits then it appeared that we
> can get this combination in the case of multi-xact.
>
> switch (members[i].status)
> {
> ...
> case MultiXactStatusForUpdate:
> bits2 |= HEAP_KEYS_UPDATED;
> break;
> }
>
> ....
> if (!has_update)
> bits |= HEAP_XMAX_LOCK_ONLY;
>
> Basically, if it is "select for update" then we will mark infomask2 as
> HEAP_KEYS_UPDATED and the informask as HEAP_XMAX_LOCK_ONLY.

Yes I saw that too, I don't know if the MultiXactStatusForUpdate case
is ok or not.

Note that this hint bit can get cleaned later in heap_update in case
of hot_update or if there's TOAST:

/*
* To prevent concurrent sessions from updating the tuple, we have to
* temporarily mark it locked, while we release the page-level lock.
[...]
/* Clear obsolete visibility flags ... */
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;