Re: making relfilenodes 56 bits

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