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