Feedback-ID: ia2694551:Fastmail
Date: Sat, 30 Jul 2022 13:39:22 +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: <20220730113922.qd7qmenwcmzyacje@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAFiTN-s+_E-3fB9xhSKFo0OgRyQXD+2maqvyr9d9Vttue3UOWA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220730113922.qd7qmenwcmzyacje%40alvherre.pgsql>
Precedence: bulk

On 2022-Jul-30, Dilip Kumar wrote:

> On Thu, Jul 28, 2022 at 9:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Please don't do this; rather use %llu and cast to (long long).
> > Otherwise the string becomes mangled for translation.
> 
> 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?

The point here is that there are two users of the source code: one is
the compiler, and the other is gettext, which extracts the string for
the translation catalog.  The compiler is OK with UINT64_FORMAT, of
course (because the preprocessor deals with it).  But gettext is quite
stupid and doesn't understand that UINT64_FORMAT expands to some
specifier, so it truncates the string at the double quote sign just
before; in other words, it just doesn't work.  So whenever you have a
string that ends up in a translation catalog, you must not use
UINT64_FORMAT or any other preprocessor macro; it has to be a straight
specifier in the format string.

We have found that the most convenient notation is to use %llu in the
string and cast the argument to (unsigned long long), so our convention
is to use that.

For strings that do not end up in a translation catalog, there's no
reason to use %llu-and-cast; UINT64_FORMAT is okay.

> > > @@ -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?

Yes, I think this should be an ereport with errcode(ERRCODE_DATA_CORRUPTED).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/