Message-ID: <7d25a313-3f98-b6c2-1aa6-36d194a41c99@enterprisedb.com>
Date: Mon, 21 Mar 2022 19:40:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: XID formatting and SLRU refactorings (was: Add 64-bit XIDs into
 PostgreSQL 15)
Content-Language: en-US
To: Maxim Orlov <orlovmg@gmail.com>, Pavel Borisov <pashkin.elfe@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 Japin Li <japinli@hotmail.com>, Alexander Korotkov <aekorotkov@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Ilya Anfimov <ilan@tzirechnoy.com>
References: 
 <CAJ7c6TPDOYBYrnCAeyndkBktO0WG2xSdYduTF0nxq+vfkmTF5Q@mail.gmail.com>
 <85f2c6c5-f8a6-5da8-67a6-9e48f56c7f77@enterprisedb.com>
 <CAPpHfduUf0UpG9LWXZHBwNhx7iZ1y_YO_u9paAjBi71fVgq3_A@mail.gmail.com>
 <MEYP282MB1669404C314BAD7EA603143FB6129@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CALT9ZEHLGm6dUd2SdpJxDd5t9wLYXDm6OGFgZS_8jDzFSnLvvQ@mail.gmail.com>
 <CACG=ezbZcjsu3JASUKgN-cyFHpB_j2pLrQ62QA7neV5M=XKX4g@mail.gmail.com>
 <CAJ7c6TNb3O2gnzww76PqS==ycUHm6Usg04=4SGHXW06394u4PA@mail.gmail.com>
 <CAJ7c6TNrK9SU5hRC5XCxEcAEFX=hyTU_JWteHfK+S285s8ejsg@mail.gmail.com>
 <CALT9ZEHGyrtuL7-PNdR6B9J+s_5NM24wPtxu5fb+XROPXNRJ8A@mail.gmail.com>
 <CACG=ezaJQikHm=vViF41ta0SMr05nMc5Qh7Ma1LXYX_GteS_Lg@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CACG=ezaJQikHm=vViF41ta0SMr05nMc5Qh7Ma1LXYX_GteS_Lg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/7d25a313-3f98-b6c2-1aa6-36d194a41c99%40enterprisedb.com>
Precedence: bulk

On 18.03.22 16:14, Maxim Orlov wrote:
> Here is v22. It addresses things mentioned by Tom and Kyotaro. Also 
> added Aleksander's changes from v21.

The v22-0002-Update-XID-formatting-in-the-.po-files.patch is not 
necessary.  That is done by a separate procedure.

I'm wondering about things like this:

- psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
-          xmax,
+ psprintf("xmax %llu equals or exceeds next valid transaction ID %u:%llu",
+          (unsigned long long) xmax,
            EpochFromFullTransactionId(ctx->next_fxid),
-          XidFromFullTransactionId(ctx->next_fxid)));
+          (unsigned long long) XidFromFullTransactionId(ctx->next_fxid)));

This %u:%u business is basically an existing workaround for the lack of 
64-bit transaction identifiers.  Presumably, when those are available, 
all of this will be replaced by a single number display, possibly a 
single %llu.  So these sites you change here will have to be touched 
again, and so changing this now doesn't make sense.  At least that's my 
guess.  Maybe there needs to be a fuller explanation of how this is 
meant to be transitioned.

As a more general point, I don't like plastering these bulky casts all 
over the place.  Casts hide problems.  For example, if we currently have

     elog(LOG, "xid is %u", xid);

and then xid is changed to a 64-bit type, this will give a compiler 
warning until you change the format.  If we add a (long long unsigned) 
cast here now and then somehow forget to change the type of xid, nothing 
will warn us about that.  (Note that there is also third-party code 
affected by this.)  Besides, these casts are quite ugly anyway, and I 
don't think the solution for all time should be that we have to add 
these casts just to print xids.

I think there needs to be a bit more soul searching here on how to 
handle that in the future and how to transition it.  I don't think 
targeting this patch for PG15 is useful at this point.