On Fri, Jun 30, 2023 at 10:45:57AM -0500, David Cook wrote:
> I noticed that pgrowlocks will use different names for shared locks depending
> on whether the locks are intermediated by a multixact or not. Particularly, if
> a single transaction has locked a row, it may return "For Key Share" or "For
> Share" in the "modes" array, while if multiple transactions have locked a row,
> it may return "Key Share" or "Share". The documentation of the pgrowlocks
> function only lists "Key Share" and "Share" as possible modes. (The four
> exclusive lock modes use the correct names in both cases)
>
> The attached patch (against the master branch) fixes this discrepancy, by
> using
> "Key Share" and "Share" in the single transaction case, since that matches the
> documentation. I also updated the test's expected output so it passes again.
You are right something is wrong. However, I looked at your patch and I
am thinking we need to go the other way and add "For" in the upper
block, rather than removing it in the lower one. I have two reasons.
Looking at the code block:
case MultiXactStatusUpdate:
snprintf(buf, NCHARS, "Update");
break;
case MultiXactStatusNoKeyUpdate:
snprintf(buf, NCHARS, "No Key Update");
break;
case MultiXactStatusForUpdate:
snprintf(buf, NCHARS, "For Update");
break;
case MultiXactStatusForNoKeyUpdate:
snprintf(buf, NCHARS, "For No Key Update");
break;
case MultiXactStatusForShare:
snprintf(buf, NCHARS, "Share");
break;
case MultiXactStatusForKeyShare:
snprintf(buf, NCHARS, "Key Share");
break;
You will notice there are "For" and non-"For" versions of "Update" and
"No Key Update". Notice that "For" appears in the macro names for the
"For" macro versions of update, but "For" does not appear in the "Share"
and "Key Share" versions, though the macro has "For".
Second, notice that the "For" and non-"For" match in the block below
that, which suggests it is correct, and the non-"For" block is later:
values[Atnum_modes] = palloc(NCHARS);
if (infomask & HEAP_XMAX_LOCK_ONLY)
{
if (HEAP_XMAX_IS_SHR_LOCKED(infomask))
snprintf(values[Atnum_modes], NCHARS, "{For Share}");
else if (HEAP_XMAX_IS_KEYSHR_LOCKED(infomask))
snprintf(values[Atnum_modes], NCHARS, "{For Key Share}");
else if (HEAP_XMAX_IS_EXCL_LOCKED(infomask))
{
if (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)
snprintf(values[Atnum_modes], NCHARS, "{For Update}");
else
snprintf(values[Atnum_modes], NCHARS, "{For No Key Update}");
}
else
/* neither keyshare nor exclusive bit it set */
snprintf(values[Atnum_modes], NCHARS,
"{transient upgrade status}");
}
else
{
if (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED)
snprintf(values[Atnum_modes], NCHARS, "{Update}");
else
snprintf(values[Atnum_modes], NCHARS, "{No Key Update}");
}
I therefore suggest this attached patch, which should be marked as an
incompatibility in PG 17.
--
Bruce Momjian <br...@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/contrib/pgrowlocks/expected/pgrowlocks.out b/contrib/pgrowlocks/expected/pgrowlocks.out
index 725467266a..77431bfe18 100644
--- a/contrib/pgrowlocks/expected/pgrowlocks.out
+++ b/contrib/pgrowlocks/expected/pgrowlocks.out
@@ -136,10 +136,10 @@ a|b
(2 rows)
step s2_rowlocks: SELECT locked_row, multi, modes FROM pgrowlocks('multixact_conflict');
-locked_row|multi|modes
-----------+-----+-------------------
-(0,1) |t |{"Key Share",Share}
-(0,2) |t |{"Key Share",Share}
+locked_row|multi|modes
+----------+-----+-----------------------------
+(0,1) |t |{"For Key Share","For Share"}
+(0,2) |t |{"For Key Share","For Share"}
(2 rows)
step s1_commit: COMMIT;
@@ -161,10 +161,10 @@ a|b
(2 rows)
step s2_rowlocks: SELECT locked_row, multi, modes FROM pgrowlocks('multixact_conflict');
-locked_row|multi|modes
-----------+-----+---------------------------------
-(0,1) |t |{"Key Share","For No Key Update"}
-(0,2) |t |{"Key Share","For No Key Update"}
+locked_row|multi|modes
+----------+-----+-------------------------------------
+(0,1) |t |{"For Key Share","For No Key Update"}
+(0,2) |t |{"For Key Share","For No Key Update"}
(2 rows)
step s1_commit: COMMIT;
@@ -186,10 +186,10 @@ a|b
(2 rows)
step s2_rowlocks: SELECT locked_row, multi, modes FROM pgrowlocks('multixact_conflict');
-locked_row|multi|modes
-----------+-----+--------------------------
-(0,1) |t |{"Key Share","For Update"}
-(0,2) |t |{"Key Share","For Update"}
+locked_row|multi|modes
+----------+-----+------------------------------
+(0,1) |t |{"For Key Share","For Update"}
+(0,2) |t |{"For Key Share","For Update"}
(2 rows)
step s1_commit: COMMIT;
@@ -205,10 +205,10 @@ a|b
step s1_updatea: UPDATE multixact_conflict SET a = 10 WHERE a = 1;
step s2_rowlocks: SELECT locked_row, multi, modes FROM pgrowlocks('multixact_conflict');
-locked_row|multi|modes
-----------+-----+--------------------
-(0,1) |t |{"Key Share",Update}
-(0,2) |f |{"For Key Share"}
+locked_row|multi|modes
+----------+-----+------------------------
+(0,1) |t |{"For Key Share",Update}
+(0,2) |f |{"For Key Share"}
(2 rows)
step s1_commit: COMMIT;
@@ -224,10 +224,10 @@ a|b
step s1_updateb: UPDATE multixact_conflict SET b = 11 WHERE b = 4;
step s2_rowlocks: SELECT locked_row, multi, modes FROM pgrowlocks('multixact_conflict');
-locked_row|multi|modes
-----------+-----+-----------------------------
-(0,1) |f |{"For Key Share"}
-(0,2) |t |{"Key Share","No Key Update"}
+locked_row|multi|modes
+----------+-----+---------------------------------
+(0,1) |f |{"For Key Share"}
+(0,2) |t |{"For Key Share","No Key Update"}
(2 rows)
step s1_commit: COMMIT;
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index c543277b7c..a04e187ec4 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -200,10 +200,10 @@ pgrowlocks(PG_FUNCTION_ARGS)
snprintf(buf, NCHARS, "For No Key Update");
break;
case MultiXactStatusForShare:
- snprintf(buf, NCHARS, "Share");
+ snprintf(buf, NCHARS, "For Share");
break;
case MultiXactStatusForKeyShare:
- snprintf(buf, NCHARS, "Key Share");
+ snprintf(buf, NCHARS, "For Key Share");
break;
}
strcat(values[Atnum_modes], buf);
diff --git a/doc/src/sgml/pgrowlocks.sgml b/doc/src/sgml/pgrowlocks.sgml
index b5e655735a..9c6e86b27e 100644
--- a/doc/src/sgml/pgrowlocks.sgml
+++ b/doc/src/sgml/pgrowlocks.sgml
@@ -74,7 +74,7 @@ pgrowlocks(text) returns setof record
<entry><structfield>modes</structfield></entry>
<entry><type>text[]</type></entry>
<entry>Lock mode of lockers (more than one if multitransaction),
- an array of <literal>Key Share</literal>, <literal>Share</literal>,
+ an array of <literal>For Key Share</literal>, <literal>For Share</literal>,
<literal>For No Key Update</literal>, <literal>No Key Update</literal>,
<literal>For Update</literal>, <literal>Update</literal>.</entry>
</row>
