Feedback-ID: ia2694551:Fastmail
Date: Thu, 28 Jul 2022 17:59:11 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	vignesh C <vignesh21@gmail.com>
Subject: Re: making relfilenodes 56 bits
Message-ID: <20220728155911.wlci3sovhmtrubrk@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAFiTN-uwSER21k_-WehsHe63fJTY+Bizc+Q+6S58xp4YvsJV_Q@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220728155911.wlci3sovhmtrubrk%40alvherre.pgsql>
Precedence: bulk

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)