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)
