[PATCH] pgrowlocks: Make mode names consistent with docs

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 <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.

On Thu, Sep 7, 2023 at 12:58:29PM -0400, Bruce Momjian wrote:

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.

Patch applied to master.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Only you can decide what is important to you.