On 2022/02/08 13:23, Ken Kato wrote:
>
> Thank you for the comments!
>
>> if (FullTransactionIdFollows(fxid1, fxid2))
>> PG_RETURN_FULLTRANSACTIONID(fxid1);
>> else
>> PG_RETURN_FULLTRANSACTIONID(fxid2);
>
>> Isn't it better to use '0xffffffffffffffff'::xid8 instead of
>> '18446744073709551615'::xid8, to more easily understand that this test
>> uses maximum number allowed as xid8?
>
> I updated these two parts as you suggested.
>
>
>> In addition to those two xid8 values, IMO it's better to insert also
>> the xid8 value neither minimum nor maximum xid8 ones, for example,
>> '42'::xid8.
>
> I added '010'::xid8, '42'::xid8, and '-1'::xid8
> in addition to '0'::xid8 and '0xffffffffffffffff'::xid8
> just to have more varieties.
Thanks for updating the patch! It basically looks good to me. I applied the following small changes to the patch.
Updatedversion of the patch attached. Could you review this version?
+ if (FullTransactionIdFollowsOrEquals(fxid1, fxid2))
+ PG_RETURN_FULLTRANSACTIONID(fxid1);
I used FullTransactionIdFollows() and FullTransactionIdPrecedes() in xid8_larger() and xid8_smaller() because other
xxx_larger()and xxx_smaller() functions also use ">" operator instead of ">=".
+create table xid8_tab (x xid8);
+insert into xid8_tab values ('0'::xid8), ('010'::xid8),
+('42'::xid8), ('0xffffffffffffffff'::xid8), ('-1'::xid8);
Since "::xid8" is not necessary here, I got rid of it from the above query.
I also merged this xid8_tab and the existing xid8_t1 table, to reduce the number of table creation.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION