Re: making relfilenodes 56 bits

Not a full review, just a quick skim of 0003.

On 2022-Jul-28, Dilip Kumar wrote:

> +    if (!shutdown)
> +    {
> +        if (ShmemVariableCache->loggedRelFileNumber < checkPoint.nextRelFileNumber)
> +            elog(ERROR, "nextRelFileNumber can not go backward from " INT64_FORMAT "to" INT64_FORMAT,
> +                 checkPoint.nextRelFileNumber, ShmemVariableCache->loggedRelFileNumber);
> +
> +        checkPoint.nextRelFileNumber = ShmemVariableCache->loggedRelFileNumber;
> +    }

Please don't do this; rather use %llu and cast to (long long).
Otherwise the string becomes mangled for translation.  I think there are
many uses of this sort of pattern in strings, but not all of them are
translatable so maybe we don't care -- for example contrib doesn't have
translations.  And the rmgrdesc routines don't translate either, so we
probably don't care about it there; and nothing that uses elog either.
But this one in particular I think should be an ereport, not an elog.
There are several other ereports in various places of the patch also.

> @@ -2378,7 +2378,7 @@ verifyBackupPageConsistency(XLogReaderState *record)
>          if (memcmp(replay_image_masked, primary_image_masked, BLCKSZ) != 0)
>          {
>              elog(FATAL,
> -                 "inconsistent page found, rel %u/%u/%u, forknum %u, blkno %u",
> +                 "inconsistent page found, rel %u/%u/" INT64_FORMAT ", forknum %u, blkno %u",
>                   rlocator.spcOid, rlocator.dbOid, rlocator.relNumber,
>                   forknum, blkno);

Should this one be an ereport, and thus you do need to change it to that
and handle it like that?


> +        if (xlrec->rlocator.relNumber > ShmemVariableCache->nextRelFileNumber)
> +            elog(ERROR, "unexpected relnumber " INT64_FORMAT "that is bigger than nextRelFileNumber " INT64_FORMAT,
> +                 xlrec->rlocator.relNumber, ShmemVariableCache->nextRelFileNumber);

You missed one whitespace here after the INT64_FORMAT.

> diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
> index c390ec5..f727078 100644
> --- a/src/bin/pg_controldata/pg_controldata.c
> +++ b/src/bin/pg_controldata/pg_controldata.c
> @@ -250,6 +250,8 @@ main(int argc, char *argv[])
>      printf(_("Latest checkpoint's NextXID:          %u:%u\n"),
>             EpochFromFullTransactionId(ControlFile->checkPointCopy.nextXid),
>             XidFromFullTransactionId(ControlFile->checkPointCopy.nextXid));
> +    printf(_("Latest checkpoint's NextRelFileNumber:  " INT64_FORMAT "\n"),
> +           ControlFile->checkPointCopy.nextRelFileNumber);

This one must definitely be translatable.

>  /* Characters to allow for an RelFileNumber in a relation path */
> -#define RELNUMBERCHARS    OIDCHARS    /* same as OIDCHARS */
> +#define RELNUMBERCHARS    20        /* max chars printed by %lu */

Maybe say %llu here instead.


I do wonder why do we keep relfilenodes limited to decimal digits.  Why
not use hex digits?  Then we know the limit is 14 chars, as in
0x00FFFFFFFFFFFFFF in the MAX_RELFILENUMBER definition.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)