Thread: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination

Hi,

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.

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.

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.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"There is evil in the world. There are dark, awful things. Occasionally, we get
a glimpse of them. But there are dark corners; horrors almost impossible to
imagine... even in our worst nightmares." (Van Helsing, Dracula A.D. 1972)

On Mon, Jan 25, 2021 at 12:01 AM Alvaro Herrera <alvherre@alvh.no-ip.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.

Yeah, the combination clearly doesn't make sense, but I'm wondering
what to do about existing data?  Amcheck.verify_am will report
corruption for those, and at least all servers where powa-archivist
extension is installed will be impacted.

On Mon, Jan 25, 2021 at 12:06 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> On Sun, 24 Jan 2021 at 11:48, Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Hi,
> >
> > 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.
>
> Thanks Juliean for reporting this. I am also able to reproduce this assert.

Thanks for looking at it!
>
> Small test case to reproduce:
>>
>> DROP TABLE IF EXISTS t1;
>> CREATE TABLE t1(id integer, val text);
>> INSERT INTO t1 SELECT i, 'val' FROM generate_series(1, 2) i;
>>
>> BEGIN;
>> SAVEPOINT s1;
>> SELECT 1 FROM t1 WHERE id = 2 FOR UPDATE;
>> UPDATE t1 SET val = 'hoho' WHERE id = 2;
>> release s1;
>> SELECT 1 FROM t1 WHERE id = 2 FOR UPDATE;
>> UPDATE t1 SET val = 'hoho' WHERE id = 2;
>
>
> If we remove the "release s1;" step from the test case, then we are not getting this assert failure.

Yes, this is the smallest reproducer that could trigger the problem,
and the release is required.

On Sun, Jan 24, 2021 at 9:31 PM Alvaro Herrera <alvherre@alvh.no-ip.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.

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

On Mon, Feb 1, 2021 at 2:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sun, Jan 24, 2021 at 9:31 PM Alvaro Herrera <alvherre@alvh.no-ip.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;

On Mon, Feb 1, 2021 at 4:05 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 2:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Sun, Jan 24, 2021 at 9:31 PM Alvaro Herrera <alvherre@alvh.no-ip.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

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

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.

Separately, maybe it'd also be good to have a test case based on
Julien's SQL snippet that produces this particular infomask combination
(and other interesting combinations) and passes them through VACUUM etc
to see that everything behaves correctly.

You could also argue the HEAP_KEYS_UPDATED is a misnomer and that we'd
do well to change its name, and update README.tuplock.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

On Mon, Feb 01, 2021 at 02:00:48PM -0300, Alvaro Herrera 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.  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.

Thanks for the clarification, 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.

I'm attaching a patch to do so.

> Separately, maybe it'd also be good to have a test case based on
> Julien's SQL snippet that produces this particular infomask combination
> (and other interesting combinations) and passes them through VACUUM etc
> to see that everything behaves correctly.

I also updated amcheck perl regression tests to generate such a combination,
add added an additional pass of verify_heapam() just after the VACUUM.

> 
> You could also argue the HEAP_KEYS_UPDATED is a misnomer and that we'd
> do well to change its name, and update README.tuplock.

Changing the name may be overkill, but claryfing the hint bit usage in
README.tuplock would definitely be useful, especially since the combination
isn't always produced.  How about adding something like:

 HEAP_KEYS_UPDATED
  This bit lives in t_infomask2.  If set, indicates that the XMAX updated
  this tuple and changed the key values, or it deleted the tuple.
+  It can also be set in combination of HEAP_XMAX_LOCK_ONLY.
  It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.

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

On Tue, 2 Feb 2021 at 09:51, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, Feb 01, 2021 at 02:00:48PM -0300, Alvaro Herrera 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.  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.
>
> Thanks for the clarification, 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.
>
> I'm attaching a patch to do so.

Thanks Julien for the patch.

Patch looks good to me and it is fixing the problem. I think we can
register in CF.

>
> > Separately, maybe it'd also be good to have a test case based on
> > Julien's SQL snippet that produces this particular infomask combination
> > (and other interesting combinations) and passes them through VACUUM etc
> > to see that everything behaves correctly.
>
> I also updated amcheck perl regression tests to generate such a combination,
> add added an additional pass of verify_heapam() just after the VACUUM.
>
> >
> > You could also argue the HEAP_KEYS_UPDATED is a misnomer and that we'd
> > do well to change its name, and update README.tuplock.
>
> Changing the name may be overkill, but claryfing the hint bit usage in
> README.tuplock would definitely be useful, especially since the combination
> isn't always produced.  How about adding something like:
>
>  HEAP_KEYS_UPDATED
>   This bit lives in t_infomask2.  If set, indicates that the XMAX updated
>   this tuple and changed the key values, or it deleted the tuple.
> +  It can also be set in combination of HEAP_XMAX_LOCK_ONLY.
>   It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.
Make sense. Please can you update this?

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Thu, Feb 04, 2021 at 08:34:13PM +0530, Mahendra Singh Thalor wrote:
> On Tue, 2 Feb 2021 at 09:51, Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Mon, Feb 01, 2021 at 02:00:48PM -0300, Alvaro Herrera wrote:
> > > 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.
> >
> > I'm attaching a patch to do so.
> 
> Thanks Julien for the patch.
> 
> Patch looks good to me and it is fixing the problem. I think we can
> register in CF.

Thanks for looking at it!  I just created an entry for the next commitfest.

> >
> > Changing the name may be overkill, but claryfing the hint bit usage in
> > README.tuplock would definitely be useful, especially since the combination
> > isn't always produced.  How about adding something like:
> >
> >  HEAP_KEYS_UPDATED
> >   This bit lives in t_infomask2.  If set, indicates that the XMAX updated
> >   this tuple and changed the key values, or it deleted the tuple.
> > +  It can also be set in combination of HEAP_XMAX_LOCK_ONLY.
> >   It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.
> Make sense. Please can you update this?

Sure, done in attached v2!

On 2021-Feb-05, Julien Rouhaud wrote:

>  - HEAP_KEYS_UPDATED
>    This bit lives in t_infomask2.  If set, indicates that the XMAX updated
> -  this tuple and changed the key values, or it deleted the tuple.
> -  It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.
> +  this tuple and changed the key values, or it deleted the tuple.  It can also
> +  be set in combination of HEAP_XMAX_LOCK_ONLY.  It's set regardless of whether
> +  the XMAX is a TransactionId or a MultiXactId.

I think we should reword this more completely, to avoid saying one thing
(that the op is an update or delete) and then contradicting ourselves
(that it can also be a lock).  I propose this:

    This bit lives in t_infomask2.  If set, it indicates that the
    operation(s) done by the XMAX compromise the tuple key, such as
    a SELECT FOR UPDATE, an UPDATE that modifies the columns of the
    key, or a DELETE.

Also, I just noticed that the paragraph just above this one says that
HEAP_XMAX_EXCL_LOCK is used for both SELECT FOR UPDATE and SELECT FOR NO
KEY UPDATE, and that this bit is what differentiates them.

-- 
Álvaro Herrera       Valdivia, Chile
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

On Thu, Feb 04, 2021 at 01:22:35PM -0300, Alvaro Herrera wrote:
> On 2021-Feb-05, Julien Rouhaud wrote:
> 
> >  - HEAP_KEYS_UPDATED
> >    This bit lives in t_infomask2.  If set, indicates that the XMAX updated
> > -  this tuple and changed the key values, or it deleted the tuple.
> > -  It's set regardless of whether the XMAX is a TransactionId or a MultiXactId.
> > +  this tuple and changed the key values, or it deleted the tuple.  It can also
> > +  be set in combination of HEAP_XMAX_LOCK_ONLY.  It's set regardless of whether
> > +  the XMAX is a TransactionId or a MultiXactId.
> 
> I think we should reword this more completely, to avoid saying one thing
> (that the op is an update or delete) and then contradicting ourselves
> (that it can also be a lock).  I propose this:
> 
>     This bit lives in t_infomask2.  If set, it indicates that the
>     operation(s) done by the XMAX compromise the tuple key, such as
>     a SELECT FOR UPDATE, an UPDATE that modifies the columns of the
>     key, or a DELETE.

Thanks, that's way better, copied in v3.  I'm still a bit worried about that
description though, as that flag isn't consistently set for the FOR UPDATE
case.  Well, to be more precise it's maybe consistently set when the hint bits
are computed, but in some cases the flag is later cleared, so you won't
reliably find it in the tuple.

On 2021-Feb-05, Julien Rouhaud wrote:

> Thanks, that's way better, copied in v3.  I'm still a bit worried about that
> description though, as that flag isn't consistently set for the FOR UPDATE
> case.  Well, to be more precise it's maybe consistently set when the hint bits
> are computed, but in some cases the flag is later cleared, so you won't
> reliably find it in the tuple.

Hmm, that sounds bogus.  I think the resetting of the other bits should
be undone afterwards, but I'm not sure that we correctly set
KEYS_UPDATED again after the TOAST business.  (What stuff does, from
memory, is to make the tuple look as if it is fully updated, which is
necessary during the TOAST handling; if the bits are not correctly set
transiently, that's okay.  But it needs reinstated again later, once the
TOAST stuff is finished).

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)