On Thu, Jul 28, 2022 at 9:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Not a full review, just a quick skim of 0003.
Thanks for the review
> > + 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.
Okay, actually I did not understand the clear logic of when to use
%llu and to use (U)INT64_FORMAT. They are both used for 64-bit
integers. So do you think it is fine to replace all INT64_FORMAT in
my patch with %llu?
> > @@ -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?
Okay, so you mean irrespective of this patch should this be converted
to ereport?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
