Re: [PATCH] Add min() and max() aggregate functions for xid8

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. Updated version 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