Thread: [PATCH] Add min() and max() aggregate functions for xid8
Hi hackers, Unlike xid, xid8 increases monotonically and cannot be reused. This trait makes it possible to support min() and max() aggregate functions for xid8. I thought they would be useful for monitoring. So I made a patch for this. Best wishes, -- Ken Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
On 2022/02/03 16:45, Ken Kato wrote: > Hi hackers, > > Unlike xid, xid8 increases monotonically and cannot be reused. > This trait makes it possible to support min() and max() aggregate functions for xid8. > I thought they would be useful for monitoring. > > So I made a patch for this. Thanks for the patch! +1 with this feature. + PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) > U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2); Shouldn't we use FullTransactionIdFollows() to compare those two fxid values here, instead? + PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) < U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2); Shouldn't we use FullTransactionIdPrecedes() to compare those two fxid values here, instead? Could you add the regression tests for those min() and max() functions for xid8? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
> + PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) > > U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2); > > Shouldn't we use FullTransactionIdFollows() to compare those two fxid > values here, instead? > > + PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) < > U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2); > > Shouldn't we use FullTransactionIdPrecedes() to compare those two fxid > values here, instead? > > Could you add the regression tests for those min() and max() functions > for xid8? Thank you for the comments. I sent my old version of patch by mistake. This is the updated one. Best wishes -- Ken Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
On 2022/02/05 10:46, Ken Kato wrote:
> Thank you for the comments.
> I sent my old version of patch by mistake.
> This is the updated one.
Thanks!
+ PG_RETURN_FULLTRANSACTIONID((FullTransactionIdFollowsOrEquals(fxid1, fxid2)) ? fxid1 : fxid2);
Basically it's better to use less 80 line length for readability. So how about change the format of this to the
following?
if (FullTransactionIdFollows(fxid1, fxid2))
PG_RETURN_FULLTRANSACTIONID(fxid1);
else
PG_RETURN_FULLTRANSACTIONID(fxid2);
+insert into xid8_tab values ('0'::xid8), ('18446744073709551615'::xid8);
Isn't it better to use '0xffffffffffffffff'::xid8 instead of '18446744073709551615'::xid8, to more easily understand
thatthis test uses maximum number allowed as xid8?
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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. Best wishes, -- Ken Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
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
Attachment
On 2022-02-08 15:34, Fujii Masao wrote:
> 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?
Thank you for the patch!
It looks good to me!
I'm not sure how strict coding conventions are, but the following line
is over 80 characters.
+insert into xid8_t1 values ('0'), ('010'), ('42'),
('0xffffffffffffffff'), ('-1');
Therefore, I made a patch which removed ('010') just to fit in 80
characters.
Best wishes,
--
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachment
On 2022/02/08 18:43, Ken Kato wrote:
> On 2022-02-08 15:34, Fujii Masao wrote:
>> 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?
>
> Thank you for the patch!
>
> It looks good to me!
Thanks for the review!
> I'm not sure how strict coding conventions are, but the following line is over 80 characters.
> +insert into xid8_t1 values ('0'), ('010'), ('42'), ('0xffffffffffffffff'), ('-1');
> Therefore, I made a patch which removed ('010') just to fit in 80 characters.
If you want to avoid the line longer than 80 columns, you should break it into two or more rather than remove the test
code,I think. What to test is more important than formatting.
Also the following descriptions about formatting would be helpful.
---------------------------
https://www.postgresql.org/docs/devel/source-format.html
Limit line lengths so that the code is readable in an 80-column window.
(This doesn't mean that you must never go past 80 columns. For instance,
breaking a long error message string in arbitrary places just to keep
the code within 80 columns is probably not a net gain in readability.)
---------------------------
Therefore I'm ok with the patch that I posted upthread. Also I'm ok if you will break that longer line into two and
postnew patch. Or if the value '010' is really useless for the test purpose, I'm also ok if you remove it. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2022-02-08 23:16, Fujii Masao wrote: > If you want to avoid the line longer than 80 columns, you should break > it into two or more rather than remove the test code, I think. What to > test is more important than formatting. > > Also the following descriptions about formatting would be helpful. > > --------------------------- > https://www.postgresql.org/docs/devel/source-format.html > > Limit line lengths so that the code is readable in an 80-column window. > (This doesn't mean that you must never go past 80 columns. For > instance, > breaking a long error message string in arbitrary places just to keep > the code within 80 columns is probably not a net gain in readability.) > --------------------------- > > Therefore I'm ok with the patch that I posted upthread. Also I'm ok if > you will break that longer line into two and post new patch. Or if the > value '010' is really useless for the test purpose, I'm also ok if you > remove it. Thought? Thank you for the explanation! Even though the line is over 80 characters, it makes more sense to put in one line and it enhances readability IMO. Also, '010' is good to have since it is the only octal value in the test. Therefore, I think min_max_aggregates_for_xid8_v4.patch is the best one to go. Best wishes, -- Ken Kato Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2022/02/09 8:49, Ken Kato wrote: > On 2022-02-08 23:16, Fujii Masao wrote: >> If you want to avoid the line longer than 80 columns, you should break >> it into two or more rather than remove the test code, I think. What to >> test is more important than formatting. >> >> Also the following descriptions about formatting would be helpful. >> >> --------------------------- >> https://www.postgresql.org/docs/devel/source-format.html >> >> Limit line lengths so that the code is readable in an 80-column window. >> (This doesn't mean that you must never go past 80 columns. For instance, >> breaking a long error message string in arbitrary places just to keep >> the code within 80 columns is probably not a net gain in readability.) >> --------------------------- >> >> Therefore I'm ok with the patch that I posted upthread. Also I'm ok if >> you will break that longer line into two and post new patch. Or if the >> value '010' is really useless for the test purpose, I'm also ok if you >> remove it. Thought? > > Thank you for the explanation! > > Even though the line is over 80 characters, it makes more sense to put in one line and it enhances readability IMO. > Also, '010' is good to have since it is the only octal value in the test. > > Therefore, I think min_max_aggregates_for_xid8_v4.patch is the best one to go. Agreed. So barring any objection, I will commit that patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > Agreed. So barring any objection, I will commit that patch. Sorry for being late, but I don't like the function names. +xid8_larger(PG_FUNCTION_ARGS) +xid8_smaller(PG_FUNCTION_ARGS) Aren't they named like xid8gt and xid8lt conventionally? At least the name lacks a mention to equality. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 09 Feb 2022 12:04:51 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > Agreed. So barring any objection, I will commit that patch. > > Sorry for being late, but I don't like the function names. > > +xid8_larger(PG_FUNCTION_ARGS) > +xid8_smaller(PG_FUNCTION_ARGS) > > Aren't they named like xid8gt and xid8lt conventionally? At least the > name lacks a mention to equality. Ah, sorry. the function returns larger/smaller one from the parameters. Sorry for the noise. regardes. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2022/02/09 13:04, Kyotaro Horiguchi wrote: > At Wed, 09 Feb 2022 12:04:51 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in >> At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in >>> Agreed. So barring any objection, I will commit that patch. >> >> Sorry for being late, but I don't like the function names. >> >> +xid8_larger(PG_FUNCTION_ARGS) >> +xid8_smaller(PG_FUNCTION_ARGS) >> >> Aren't they named like xid8gt and xid8lt conventionally? At least the >> name lacks a mention to equality. > > Ah, sorry. the function returns larger/smaller one from the > parameters. Sorry for the noise. Thanks for the review! I pushed the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION