Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination - Mailing list pgsql-hackers

From Dilip Kumar
Subject Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination
Date
Msg-id CAFiTN-vmWCxgp0e7J8J9A14=a_VmUaW7mmX=gPhs-2e-hXgEYg@mail.gmail.com
Whole thread Raw
In response to Re: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
List pgsql-hackers
On Mon, Feb 1, 2021 at 10:31 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jan-24, Julien Rouhaud wrote:
>
> > While working on pg14 compatibility for an extension relying on an apparently
> > uncommon combination of FOR UPDATE and stored function calls, I hit some new
> > Asserts introduced in 866e24d47db (Extend amcheck to check heap pages):
> >
> > +     /*
> > +      * 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.
>
> Maybe we should contest the idea that this is a sensible thing to Assert
> against.  AFAICS this was originally suggested here:
>
https://www.postgresql.org/message-id/flat/CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA%40mail.gmail.com#ad215d0ee0606b5f67bbc57d011c96b8
> and it appears now to have been a bad idea.

I see, I suggested that :)

  If I recall correctly,
> HEAP_KEYS_UPDATED is supposed to distinguish locks/updates that don't
> modify the key columns from those that do.  Since SELECT FOR UPDATE
> stands for a future update that may modify arbitrary portions of the
> tuple (including "key" columns), then it produces that bit, just as said
> UPDATE or a DELETE; as opposed to SELECT FOR NO KEY UPDATE which stands
> for a future UPDATE that will only change columns that aren't part of
> any keys.

Yeah, that makes sense.

> So I think that I misspoke earlier in this thread when I said this is a
> bug, and that the right fix here is to remove the Assert() and change
> amcheck to match.

+1

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



pgsql-hackers by date:

Previous
From: Peter Geoghegan
Date:
Subject: Re: New IndexAM API controlling index vacuum strategies
Next
From: Michael Paquier
Date:
Subject: Re: row filtering for logical replication