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

2022-02-09 Thread Fujii Masao 




On 2022/02/09 13:04, Kyotaro Horiguchi wrote:

At Wed, 09 Feb 2022 12:04:51 +0900 (JST), Kyotaro Horiguchi 
 wrote in

At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao  
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

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

2022-02-08 Thread Kyotaro Horiguchi 
At Wed, 09 Feb 2022 12:04:51 +0900 (JST), Kyotaro Horiguchi 
 wrote in 
> At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao  
> 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

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

2022-02-08 Thread Kyotaro Horiguchi 
At Wed, 9 Feb 2022 11:01:57 +0900, Fujii Masao  
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

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

2022-02-08 Thread Fujii Masao 




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

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

2022-02-08 Thread Ken Kato 

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

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

2022-02-08 Thread Fujii Masao 




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'), ('0x'), 
('-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 post new 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

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

2022-02-08 Thread Ken Kato 

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'), 
('0x'), ('-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 CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8754f2f89b..1b064b4feb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19973,7 +19973,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
@@ -19992,7 +19992,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 9b4ceaea47..e4b4952a28 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -286,6 +286,30 @@ xid8cmp(PG_FUNCTION_ARGS)
 		PG_RETURN_INT32(-1);
 }
 
+Datum
+xid8_larger(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	if (FullTransactionIdFollows(fxid1, fxid2))
+		PG_RETURN_FULLTRANSACTIONID(fxid1);
+	else
+		PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
+Datum
+xid8_smaller(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	if (FullTransactionIdPrecedes(fxid1, fxid2))
+		PG_RETURN_FULLTRANSACTIONID(fxid1);
+	else
+		PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
 /*
  *	 COMMAND IDENTIFIER ROUTINES			 *
  */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 137f6eef69..2843f4b415 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -149,6 +149,9 @@
 { aggfnoid => 'max(pg_lsn)', aggtransfn => 'pg_lsn_larger',
   aggcombinefn => 'pg_lsn_larger', aggsortop => '>(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'max(xid8)', aggtransfn => 'xid8_larger',
+  aggcombinefn => 'xid8_larger', aggsortop => '>(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # min
 { aggfnoid => 'min(int8)', aggtransfn => 'int8smaller',
@@ -214,6 +217,9 @@
 { aggfnoid => 'min(pg_lsn)', aggtransfn => 'pg_lsn_smaller',
   aggcombinefn => 'pg_lsn_smaller', aggsortop => '<(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'min(xid8)', aggtransfn => 'xid8_smaller',
+  aggcombinefn => 'xid8_smaller', aggsortop => '<(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # count
 { aggfnoid => 'count(any)', aggtransfn => 'int8inc_any',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..62f36daa98 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -203,6 +203,12 @@
 { oid => '5071', descr => 'convert xid8 to xid',
   proname => 'xid', prorettype => 'xid', proargtypes => 'xid8',
   prosrc => 'xid8toxid' },
+{ oid => '5097', descr => 'larger of two',
+  proname => 'xid8_larger', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_larger' },
+{ oid => '5098', descr => 'smaller of two',
+  proname => 'xid8_smaller', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_smaller' },
 { oid => '69',
   proname => 'cideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'cid cid', prosrc => 'cideq' },
@@ -6564,6 +6570,9 @@
 { oid => '4189', descr => 'maximum value of all pg_lsn input values',
   proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'pg_lsn',
   proargtypes => 'pg_lsn', prosrc => 'aggregate_dummy' },
+{ oid => '5099', descr => 'maximum value of all xid8 input values',
+  proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'xid8',
+  proargtypes => 'xid8', prosrc => 'aggregate_dummy' },
 
 { oid => '2131', descr => 'minimum value of all bigint input values',
   proname => 'min', prokind => 'a', proisstrict => 'f', prorettype => 'int8',
@@ -6631,6 +6640,9 @@
 { oid => '4190', descr => 'minimum value of all pg_lsn input values',
   proname => 'min', prokind => 'a',

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

2022-02-07 Thread Fujii Masao 



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 '0x'::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 '0x'::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), ('0x'::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 CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8754f2f89b..1b064b4feb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19973,7 +19973,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
@@ -19992,7 +19992,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 9b4ceaea47..e4b4952a28 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -286,6 +286,30 @@ xid8cmp(PG_FUNCTION_ARGS)
PG_RETURN_INT32(-1);
 }
 
+Datum
+xid8_larger(PG_FUNCTION_ARGS)
+{
+   FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+   FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+   if (FullTransactionIdFollows(fxid1, fxid2))
+   PG_RETURN_FULLTRANSACTIONID(fxid1);
+   else
+   PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
+Datum
+xid8_smaller(PG_FUNCTION_ARGS)
+{
+   FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+   FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+   if (FullTransactionIdPrecedes(fxid1, fxid2))
+   PG_RETURN_FULLTRANSACTIONID(fxid1);
+   else
+   PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
 /*
  *  COMMAND IDENTIFIER ROUTINES
 *
  */
diff --git a/src/include/catalog/pg_aggregate.dat 
b/src/include/catalog/pg_aggregate.dat
index 137f6eef69..2843f4b415 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -149,6 +149,9 @@
 { aggfnoid => 'max(pg_lsn)', aggtransfn => 'pg_lsn_larger',
   aggcombinefn => 'pg_lsn_larger', aggsortop => '>(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'max(xid8)', aggtransfn => 'xid8_larger',
+  aggcombinefn => 'xid8_larger', aggsortop => '>(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # min
 { aggfnoid => 'min(int8)', aggtransfn => 'int8smaller',
@@ -214,6 +217,9 @@
 { aggfnoid => 'min(pg_lsn)', aggtransfn => 'pg_lsn_smaller',
   aggcombinefn => 'pg_lsn_smaller', aggsortop => '<(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'min(xid8)', aggtransfn => 'xid8_smaller',
+  aggcombinefn => 'xid8_smaller', aggsortop => '<(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # count
 { aggfnoid => 'count(any)', aggtransfn => 'int8inc_any',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..62f36daa98 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -203,6 +203,12 @@
 { oid => '5071', descr => 'convert xid8 to xid',
   proname => 'xid', prorettype => 'xid', proargtypes => 'xid8',

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

2022-02-07 Thread Ken Kato 


Thank you for the comments!


if (FullTransactionIdFollows(fxid1, fxid2))
PG_RETURN_FULLTRANSACTIONID(fxid1);
else
PG_RETURN_FULLTRANSACTIONID(fxid2);



Isn't it better to use '0x'::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 '0x'::xid8
just to have more varieties.


Best wishes,

--
Ken Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8754f2f89b..1b064b4feb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19973,7 +19973,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
@@ -19992,7 +19992,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 9b4ceaea47..9f7e1816b0 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -286,6 +286,30 @@ xid8cmp(PG_FUNCTION_ARGS)
 		PG_RETURN_INT32(-1);
 }
 
+Datum
+xid8_larger(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	if (FullTransactionIdFollowsOrEquals(fxid1, fxid2))
+		PG_RETURN_FULLTRANSACTIONID(fxid1);
+	else
+		PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
+Datum
+xid8_smaller(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	if (FullTransactionIdPrecedesOrEquals(fxid1, fxid2))
+		PG_RETURN_FULLTRANSACTIONID(fxid1);
+	else
+		PG_RETURN_FULLTRANSACTIONID(fxid2);
+}
+
 /*
  *	 COMMAND IDENTIFIER ROUTINES			 *
  */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 137f6eef69..2843f4b415 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -149,6 +149,9 @@
 { aggfnoid => 'max(pg_lsn)', aggtransfn => 'pg_lsn_larger',
   aggcombinefn => 'pg_lsn_larger', aggsortop => '>(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'max(xid8)', aggtransfn => 'xid8_larger',
+  aggcombinefn => 'xid8_larger', aggsortop => '>(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # min
 { aggfnoid => 'min(int8)', aggtransfn => 'int8smaller',
@@ -214,6 +217,9 @@
 { aggfnoid => 'min(pg_lsn)', aggtransfn => 'pg_lsn_smaller',
   aggcombinefn => 'pg_lsn_smaller', aggsortop => '<(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'min(xid8)', aggtransfn => 'xid8_smaller',
+  aggcombinefn => 'xid8_smaller', aggsortop => '<(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # count
 { aggfnoid => 'count(any)', aggtransfn => 'int8inc_any',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..62f36daa98 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -203,6 +203,12 @@
 { oid => '5071', descr => 'convert xid8 to xid',
   proname => 'xid', prorettype => 'xid', proargtypes => 'xid8',
   prosrc => 'xid8toxid' },
+{ oid => '5097', descr => 'larger of two',
+  proname => 'xid8_larger', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_larger' },
+{ oid => '5098', descr => 'smaller of two',
+  proname => 'xid8_smaller', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_smaller' },
 { oid => '69',
   proname => 'cideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'cid cid', prosrc => 'cideq' },
@@ -6564,6 +6570,9 @@
 { oid => '4189', descr => 'maximum value of all pg_lsn input values',
   proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'pg_lsn',
   proargtypes => 'pg_lsn', prosrc => 'aggregate_dummy' },
+{ oid => '5099', descr => 'maximum value of all xid8 input values',
+  proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'xid8',
+  proargtypes => 'xid8', prosrc => 'aggregate_dummy' },
 
 { oid => '2131', descr => 'minimum value of all bigint input values',
   proname => 'min', prokind => 'a', proisstrict => 'f',

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

2022-02-06 Thread Fujii Masao 




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 '0x'::xid8 instead of 
'18446744073709551615'::xid8, to more easily understand that this 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

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

2022-02-04 Thread Ken Kato 

+   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 CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8754f2f89b..1b064b4feb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19973,7 +19973,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
@@ -19992,7 +19992,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 9b4ceaea47..32009f81ae 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -286,6 +286,24 @@ xid8cmp(PG_FUNCTION_ARGS)
 		PG_RETURN_INT32(-1);
 }
 
+Datum
+xid8_larger(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	PG_RETURN_FULLTRANSACTIONID((FullTransactionIdFollowsOrEquals(fxid1, fxid2)) ? fxid1 : fxid2);
+}
+
+Datum
+xid8_smaller(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	PG_RETURN_FULLTRANSACTIONID((FullTransactionIdPrecedesOrEquals(fxid1, fxid2)) ? fxid1 : fxid2);
+}
+
 /*
  *	 COMMAND IDENTIFIER ROUTINES			 *
  */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 137f6eef69..2843f4b415 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -149,6 +149,9 @@
 { aggfnoid => 'max(pg_lsn)', aggtransfn => 'pg_lsn_larger',
   aggcombinefn => 'pg_lsn_larger', aggsortop => '>(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'max(xid8)', aggtransfn => 'xid8_larger',
+  aggcombinefn => 'xid8_larger', aggsortop => '>(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # min
 { aggfnoid => 'min(int8)', aggtransfn => 'int8smaller',
@@ -214,6 +217,9 @@
 { aggfnoid => 'min(pg_lsn)', aggtransfn => 'pg_lsn_smaller',
   aggcombinefn => 'pg_lsn_smaller', aggsortop => '<(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'min(xid8)', aggtransfn => 'xid8_smaller',
+  aggcombinefn => 'xid8_smaller', aggsortop => '<(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # count
 { aggfnoid => 'count(any)', aggtransfn => 'int8inc_any',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..62f36daa98 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -203,6 +203,12 @@
 { oid => '5071', descr => 'convert xid8 to xid',
   proname => 'xid', prorettype => 'xid', proargtypes => 'xid8',
   prosrc => 'xid8toxid' },
+{ oid => '5097', descr => 'larger of two',
+  proname => 'xid8_larger', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_larger' },
+{ oid => '5098', descr => 'smaller of two',
+  proname => 'xid8_smaller', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_smaller' },
 { oid => '69',
   proname => 'cideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'cid cid', prosrc => 'cideq' },
@@ -6564,6 +6570,9 @@
 { oid => '4189', descr => 'maximum value of all pg_lsn input values',
   proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'pg_lsn',
   proargtypes => 'pg_lsn', prosrc => 'aggregate_dummy' },
+{ oid => '5099', descr => 'maximum value of all xid8 input values',
+  proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'xid8',
+  proargtypes => 'xid8', prosrc => 'aggregate_dummy' },
 
 { oid => '2131', descr => 'minimum value of all bigint input values',
   proname => 'min', prokind => 'a', proisstrict => 'f', prorettype => 'int8',
@@ -6631,6 +6640,9 @@
 { oid => '4190', descr => 'minimum value of all pg_lsn input values',
   proname => 'min', prokind

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

2022-02-04 Thread Fujii Masao 




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

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

2022-02-02 Thread Ken Kato 

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 CORPORATIONdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8754f2f89b..1b064b4feb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19973,7 +19973,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
@@ -19992,7 +19992,7 @@ SELECT NULLIF(value, '(none)') ...
 values.  Available for any numeric, string, date/time, or enum type,
 as well as inet, interval,
 money, oid, pg_lsn,
-tid,
+tid, xid8,
 and arrays of any of these types.

Yes
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 9b4ceaea47..6e24697782 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -286,6 +286,24 @@ xid8cmp(PG_FUNCTION_ARGS)
 		PG_RETURN_INT32(-1);
 }
 
+Datum
+xid8_larger(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) > U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2);
+}
+
+Datum
+xid8_smaller(PG_FUNCTION_ARGS)
+{
+	FullTransactionId fxid1 = PG_GETARG_FULLTRANSACTIONID(0);
+	FullTransactionId fxid2 = PG_GETARG_FULLTRANSACTIONID(1);
+
+	PG_RETURN_FULLTRANSACTIONID((U64FromFullTransactionId(fxid1) < U64FromFullTransactionId(fxid2)) ? fxid1 : fxid2);
+}
+
 /*
  *	 COMMAND IDENTIFIER ROUTINES			 *
  */
diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg_aggregate.dat
index 137f6eef69..2843f4b415 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -149,6 +149,9 @@
 { aggfnoid => 'max(pg_lsn)', aggtransfn => 'pg_lsn_larger',
   aggcombinefn => 'pg_lsn_larger', aggsortop => '>(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'max(xid8)', aggtransfn => 'xid8_larger',
+  aggcombinefn => 'xid8_larger', aggsortop => '>(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # min
 { aggfnoid => 'min(int8)', aggtransfn => 'int8smaller',
@@ -214,6 +217,9 @@
 { aggfnoid => 'min(pg_lsn)', aggtransfn => 'pg_lsn_smaller',
   aggcombinefn => 'pg_lsn_smaller', aggsortop => '<(pg_lsn,pg_lsn)',
   aggtranstype => 'pg_lsn' },
+{ aggfnoid => 'min(xid8)', aggtransfn => 'xid8_smaller',
+  aggcombinefn => 'xid8_smaller', aggsortop => '<(xid8,xid8)',
+  aggtranstype => 'xid8' },
 
 # count
 { aggfnoid => 'count(any)', aggtransfn => 'int8inc_any',
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7024dbe10a..62f36daa98 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -203,6 +203,12 @@
 { oid => '5071', descr => 'convert xid8 to xid',
   proname => 'xid', prorettype => 'xid', proargtypes => 'xid8',
   prosrc => 'xid8toxid' },
+{ oid => '5097', descr => 'larger of two',
+  proname => 'xid8_larger', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_larger' },
+{ oid => '5098', descr => 'smaller of two',
+  proname => 'xid8_smaller', prorettype => 'xid8', proargtypes => 'xid8 xid8',
+  prosrc => 'xid8_smaller' },
 { oid => '69',
   proname => 'cideq', proleakproof => 't', prorettype => 'bool',
   proargtypes => 'cid cid', prosrc => 'cideq' },
@@ -6564,6 +6570,9 @@
 { oid => '4189', descr => 'maximum value of all pg_lsn input values',
   proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'pg_lsn',
   proargtypes => 'pg_lsn', prosrc => 'aggregate_dummy' },
+{ oid => '5099', descr => 'maximum value of all xid8 input values',
+  proname => 'max', prokind => 'a', proisstrict => 'f', prorettype => 'xid8',
+  proargtypes => 'xid8', prosrc => 'aggregate_dummy' },
 
 { oid => '2131', descr => 'minimum value of all bigint input values',
   proname => 'min', prokind => 'a', proisstrict => 'f', prorettype => 'int8',
@@ -6631,6 +6640,9 @@
 { oid => '4190', descr => 'minimum value of all pg_lsn input values',
   proname => 'min', prokind => 'a', proisstrict => 'f', prorettype => 'pg_lsn',
   proargtypes => 'pg_lsn', prosrc => 'aggregate_dummy' },
+{ oid => '5100', descr => 'minimum value of all xid8 input values',
+  proname => 'min', prokind => 'a', proisstrict => 'f', prorettype => 'xid8',
+  proargtypes => 'xid8', prosrc => 'aggregate_dummy' },
 
 # count has two