On Thu, Jul 28, 2022 at 9:29 PM Alvaro Herrera <alvhe...@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