Обсуждение: Fix showing XID of a spectoken lock in an incorrect field of pg_locks view.

Hi,

I realized that pg_locks view shows the transaction id of a
speculative token lock in the database field:

postgres(1:509389)=# select * from pg_locks where locktype = 'spectoken';
 locktype  | database | relation | page | tuple | virtualxid |
transactionid | classid | objid | objsubid | virtualtransaction |  pid
  |     mode      | granted | fastpath | waitstart

-----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+--------+---------------+---------+----------+-----------
 spectoken |      741 |          |      |       |            |
      |       3 |     0 |        0 | 3/5                | 509314 |
ExclusiveLock | t       | f        |
(1 row)

It seems to be confusing and the user won't get the result even if
they search it by transactionid = 741. So I've attached the patch to
fix it. With the patch, the pg_locks views shows like:

 locktype  | database | relation | page | tuple | virtualxid |
transactionid | classid | objid | objsubid | virtualtransaction |  pid
  |     mode      | granted | fastpath | waitstart

-----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+--------+---------------+---------+----------+-----------
 spectoken |          |          |      |       |            |
  746 |         |     1 |          | 3/4                | 535618 |
ExclusiveLock | t       | f        |
(1 row)

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 4, 2023 at 12:16 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> It seems to be confusing and the user won't get the result even if
> they search it by transactionid = 741. So I've attached the patch to
> fix it. With the patch, the pg_locks views shows like:
>
>  locktype  | database | relation | page | tuple | virtualxid |
> transactionid | classid | objid | objsubid | virtualtransaction |  pid
>   |     mode      | granted | fastpath | waitstart
>
-----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+--------+---------------+---------+----------+-----------
>  spectoken |          |          |      |       |            |
>   746 |         |     1 |          | 3/4                | 535618 |
> ExclusiveLock | t       | f        |
> (1 row)
>

Is it a good idea to display spec token as objid, if so, how will
users know? Currently for Advisory locks, we display values in
classid, objid, objsubid different than the original meaning of fields
but those are explained in docs [1]. Wouldn't it be better to mention
this in docs?

[1] - https://www.postgresql.org/docs/devel/view-pg-locks.html

-- 
With Regards,
Amit Kapila.

On Wed, Jan 4, 2023 at 6:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jan 4, 2023 at 12:16 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > It seems to be confusing and the user won't get the result even if
> > they search it by transactionid = 741. So I've attached the patch to
> > fix it. With the patch, the pg_locks views shows like:
> >
> >  locktype  | database | relation | page | tuple | virtualxid |
> > transactionid | classid | objid | objsubid | virtualtransaction |  pid
> >   |     mode      | granted | fastpath | waitstart
> >
-----------+----------+----------+------+-------+------------+---------------+---------+-------+----------+--------------------+--------+---------------+---------+----------+-----------
> >  spectoken |          |          |      |       |            |
> >   746 |         |     1 |          | 3/4                | 535618 |
> > ExclusiveLock | t       | f        |
> > (1 row)
> >
>
> Is it a good idea to display spec token as objid, if so, how will
> users know? Currently for Advisory locks, we display values in
> classid, objid, objsubid different than the original meaning of fields
> but those are explained in docs [1]. Wouldn't it be better to mention
> this in docs?

Agreed. Attached the updated patch.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 5, 2023 at 11:46 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Agreed. Attached the updated patch.
>

Thanks, the patch looks good to me. I think it would be probably good
to backpatch this but it has the potential to break some monitoring
scripts which were using the wrong columns for transaction id and spec
token number. As this is not a very critical issue and is not reported
till now, so it may be better to leave backpatching it. What do you
think?

-- 
With Regards,
Amit Kapila.

On Fri, Jan 6, 2023 at 9:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jan 5, 2023 at 11:46 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Agreed. Attached the updated patch.
> >
>
> Thanks, the patch looks good to me. I think it would be probably good
> to backpatch this but it has the potential to break some monitoring
> scripts which were using the wrong columns for transaction id and spec
> token number.

Right.

> As this is not a very critical issue and is not reported
> till now, so it may be better to leave backpatching it. What do you
> think?

Considering the compatibility, I'm inclined to agree not to backpatch
it. If someone complains about the current behavior in back branches
in the future, we can backpatch it.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com