Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination

On Mon, Feb 1, 2021 at 4:05 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> 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.

It seems it is done intentionally to handle some case, I am not sure
which case though. But Setting HEAP_KEYS_UPDATED in case of "for
update" seems wrong.
The comment of this flag clearly says that "tuple was updated and key
cols modified, or tuple deleted " and that is obviously not the case
here.

#define HEAP_KEYS_UPDATED 0x2000 /* tuple was updated and key cols
* modified, or tuple deleted */

> 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;

I see.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com