Re: [PATCH] pgrowlocks: Make mode names consistent with docs

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.