On Mon, Feb 1, 2021 at 10:31 PM Alvaro Herrera <alvhe...@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