Обсуждение: [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.