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)