array_cat anycompatible change is breaking xversion upgrade tests
crake is showing xversion upgrade failures since 9e38c2bb50:
pg_restore: error: could not execute query: ERROR: function array_cat(anyarray, anyarray) does not exist
Command was: CREATE AGGREGATE "public"."array_cat_accum"("anyarray") (
SFUNC = "array_cat",
STYPE = "anyarray",
INITCOND = '{}'
);
As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.
I think the most plausible response is to add this aggregate to the filter
logic that already exists in the xversion tests. Perhaps we could
alternatively change this test case so that it relies on some other
polymorphic function, but I'm not quite sure what a good candidate
would be.
regards, tom lane
I wrote:
I think the most plausible response is to add this aggregate to the filter
logic that already exists in the xversion tests. Perhaps we could
alternatively change this test case so that it relies on some other
polymorphic function, but I'm not quite sure what a good candidate
would be.
After looking at the commit that added array_cat_accum() (65d9aedb1),
I decided that it's fine to replace this aggregate with one using another
anyarray function, such as array_larger(). The point of that test is just
to show that the array argument can be array-of-record, so we don't need
the operation to be array_cat() specifically. So I propose the attached
patches to un-break the xversion tests.
I'll hold off pushing this till after this week's wraps, though.
regards, tom lane
Attachments:
xversion-fix-head.patchtext/x-diff; charset=us-ascii; name=xversion-fix-head.patchDownload
diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 2c3bb0a60b..b33b004a7f 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -729,24 +729,24 @@ select q2, sql_if(q2 > 0, q2, q2 + 1) from int8_tbl;
(5 rows)
-- another sort of polymorphic aggregate
-CREATE AGGREGATE array_cat_accum (anycompatiblearray)
+CREATE AGGREGATE array_larger_accum (anyarray)
(
- sfunc = array_cat,
- stype = anycompatiblearray,
+ sfunc = array_larger,
+ stype = anyarray,
initcond = '{}'
);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[1,2]), (ARRAY[3,4])) as t(i);
- array_cat_accum
------------------
- {1,2,3,4}
+ array_larger_accum
+--------------------
+ {3,4}
(1 row)
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[row(1,2),row(3,4)]), (ARRAY[row(5,6),row(7,8)])) as t(i);
- array_cat_accum
------------------------------------
- {"(1,2)","(3,4)","(5,6)","(7,8)"}
+ array_larger_accum
+--------------------
+ {"(5,6)","(7,8)"}
(1 row)
-- another kind of polymorphic aggregate
diff --git a/src/test/regress/sql/polymorphism.sql b/src/test/regress/sql/polymorphism.sql
index 70a21c8978..527899e8ad 100644
--- a/src/test/regress/sql/polymorphism.sql
+++ b/src/test/regress/sql/polymorphism.sql
@@ -498,17 +498,17 @@ select q2, sql_if(q2 > 0, q2, q2 + 1) from int8_tbl;
-- another sort of polymorphic aggregate
-CREATE AGGREGATE array_cat_accum (anycompatiblearray)
+CREATE AGGREGATE array_larger_accum (anyarray)
(
- sfunc = array_cat,
- stype = anycompatiblearray,
+ sfunc = array_larger,
+ stype = anyarray,
initcond = '{}'
);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[1,2]), (ARRAY[3,4])) as t(i);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[row(1,2),row(3,4)]), (ARRAY[row(5,6),row(7,8)])) as t(i);
-- another kind of polymorphic aggregate
xversion-fix-back.patchtext/x-diff; charset=us-ascii; name=xversion-fix-back.patchDownload
diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 1ff40764d9..980e2e861c 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -729,24 +729,24 @@ select q2, sql_if(q2 > 0, q2, q2 + 1) from int8_tbl;
(5 rows)
-- another sort of polymorphic aggregate
-CREATE AGGREGATE array_cat_accum (anyarray)
+CREATE AGGREGATE array_larger_accum (anyarray)
(
- sfunc = array_cat,
+ sfunc = array_larger,
stype = anyarray,
initcond = '{}'
);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[1,2]), (ARRAY[3,4])) as t(i);
- array_cat_accum
------------------
- {1,2,3,4}
+ array_larger_accum
+--------------------
+ {3,4}
(1 row)
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[row(1,2),row(3,4)]), (ARRAY[row(5,6),row(7,8)])) as t(i);
- array_cat_accum
------------------------------------
- {"(1,2)","(3,4)","(5,6)","(7,8)"}
+ array_larger_accum
+--------------------
+ {"(5,6)","(7,8)"}
(1 row)
-- another kind of polymorphic aggregate
diff --git a/src/test/regress/sql/polymorphism.sql b/src/test/regress/sql/polymorphism.sql
index e5222f1f81..15b8dfc6ce 100644
--- a/src/test/regress/sql/polymorphism.sql
+++ b/src/test/regress/sql/polymorphism.sql
@@ -498,17 +498,17 @@ select q2, sql_if(q2 > 0, q2, q2 + 1) from int8_tbl;
-- another sort of polymorphic aggregate
-CREATE AGGREGATE array_cat_accum (anyarray)
+CREATE AGGREGATE array_larger_accum (anyarray)
(
- sfunc = array_cat,
+ sfunc = array_larger,
stype = anyarray,
initcond = '{}'
);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[1,2]), (ARRAY[3,4])) as t(i);
-SELECT array_cat_accum(i)
+SELECT array_larger_accum(i)
FROM (VALUES (ARRAY[row(1,2),row(3,4)]), (ARRAY[row(5,6),row(7,8)])) as t(i);
-- another kind of polymorphic aggregate
On Wed, Nov 04, 2020 at 07:43:51PM -0500, Tom Lane wrote:
crake is showing xversion upgrade failures since 9e38c2bb50:
pg_restore: error: could not execute query: ERROR: function array_cat(anyarray, anyarray) does not exist
Command was: CREATE AGGREGATE "public"."array_cat_accum"("anyarray") (
SFUNC = "array_cat",
STYPE = "anyarray",
INITCOND = '{}'
);As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.
I upgraded an internal DB to v14b1, but it took several tries, since there were
errors during pg_restore regarding aggregates using polymorphic functions
anyarray, which are now anycompatiblearray.
I succeeded in upgrading after dropping our aggregates.
I have a backup from the v13 DB, and it restores okay on v13.
However it fails with the same errors when restoring into v14.
I think this was all known, so I'm just adding a data point.
It's be easy enough to replace our "anyarrays" with "anycompatiblearrays".
But I think this should be called out as an incompatible change in the release
notes.
pg_restore: error: could not execute query: ERROR: function array_append(anyarray, anyelement) does not exist
Command was: CREATE AGGREGATE public.array_accum(anyelement) (
SFUNC = array_append,
STYPE = anyarray,
INITCOND = '{}',
PARALLEL = safe
);
pg_restore: error: could not execute query: ERROR: function array_append(anyarray, anyelement) does not exist
Command was: CREATE AGGREGATE public.pdp_context_count(anyelement) (
SFUNC = array_append,
STYPE = anyarray,
INITCOND = '{}',
FINALFUNC = public._final_pdp_context_count,
PARALLEL = safe
);
pg_restore: error: could not execute query: ERROR: function array_append(anyarray, anyelement) does not exist
Command was: CREATE AGGREGATE public.ts_mode(anyelement) (
SFUNC = array_append,
STYPE = anyarray,
INITCOND = '{}',
FINALFUNC = public._final_mode
);
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Nov 04, 2020 at 07:43:51PM -0500, Tom Lane wrote:
As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.
But I think this should be called out as an incompatible change in the release
notes.
If it was not, yes it should be.
regards, tom lane
On Thu, May 20, 2021 at 07:35:10PM -0400, Tom Lane wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Nov 04, 2020 at 07:43:51PM -0500, Tom Lane wrote:
As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.But I think this should be called out as an incompatible change in the release
notes.If it was not, yes it should be.
@Bruce, I propose:
Some system functions are changed to accept "anycompatiblearray" arguments.
This causes failures when restoring a database backup or running pg_restore if
there were aggregate functions defined using those functions with their
original argument types.
Such aggregate functions should be dropped before upgrade/restore and then
re-created afterwards using the "anycompatible" functions. The affected
functions are: array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket.
(Re-defining the function before upgrading is possible when upgrading from v13,
only).
--
Justin
@Bruce: Would you add something about this to the release notes before beta2?
I added it as an OpenItem.
Show quoted text
On Tue, May 25, 2021 at 11:14:58AM -0500, Justin Pryzby wrote:
On Thu, May 20, 2021 at 07:35:10PM -0400, Tom Lane wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Nov 04, 2020 at 07:43:51PM -0500, Tom Lane wrote:
As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.But I think this should be called out as an incompatible change in the release
notes.If it was not, yes it should be.
@Bruce, I propose:
Some system functions are changed to accept "anycompatiblearray" arguments.
This causes failures when restoring a database backup or running pg_restore if
there were aggregate functions defined using those functions with their
original argument types.Such aggregate functions should be dropped before upgrade/restore and then
re-created afterwards using the "anycompatible" functions. The affected
functions are: array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket.
On Tue, Jun 8, 2021 at 05:56:18PM -0500, Justin Pryzby wrote:
@Bruce: Would you add something about this to the release notes before beta2?
I added it as an OpenItem.
OK, see below.
On Tue, May 25, 2021 at 11:14:58AM -0500, Justin Pryzby wrote:
On Thu, May 20, 2021 at 07:35:10PM -0400, Tom Lane wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Nov 04, 2020 at 07:43:51PM -0500, Tom Lane wrote:
As was discussed in the thread leading up to that commit, modifying the
signature of array_cat and friends could break user-defined operators
and aggregates based on those functions. It seems to me that the
usability gain from this change is worth that cost, but it is causing
an issue for xversion tests.
Uh, this is _using_ these functions in aggregates, or changing the
system functions' argument types, right? I didn't think we supported
dump/restore of modified system tables.
But I think this should be called out as an incompatible change in the release
notes.If it was not, yes it should be.
@Bruce, I propose:
Some system functions are changed to accept "anycompatiblearray" arguments.
This causes failures when restoring a database backup or running pg_restore if
there were aggregate functions defined using those functions with their
original argument types.Such aggregate functions should be dropped before upgrade/restore and then
re-created afterwards using the "anycompatible" functions. The affected
functions are: array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket.
I read the entire thread and I see:
pg_restore: error: could not execute query: ERROR: function
array_append(anyarray, anyelement) does not exist
Command was: CREATE AGGREGATE public.array_accum(anyelement) (
SFUNC = array_append,
STYPE = anyarray,
INITCOND = '{}',
PARALLEL = safe
);
This involves creating an aggreate that _uses_ these array functions as
their state transition function?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
On Tue, Jun 08, 2021 at 08:02:46PM -0400, Bruce Momjian wrote:
This involves creating an aggreate that _uses_ these array functions as
their state transition function?
Yes
--
Justin
On Tue, Jun 8, 2021 at 07:10:00PM -0500, Justin Pryzby wrote:
On Tue, Jun 08, 2021 at 08:02:46PM -0400, Bruce Momjian wrote:
This involves creating an aggreate that _uses_ these array functions as
their state transition function?Yes
OK, I came up with the attached patch. This is one of the few cases
where the incompatibility is not clearly related to the feature, so I
left the existing item alone and just created a new one with the same
commit message in the incompatibilities section.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Attachments:
agg.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml
index 21659bd184..426d502a63 100644
--- a/doc/src/sgml/release-14.sgml
+++ b/doc/src/sgml/release-14.sgml
@@ -291,6 +291,32 @@ Author: Tom Lane <tgl@sss.pgh.pa.us>
<listitem>
<!--
Author: Tom Lane <tgl@sss.pgh.pa.us>
+2020-11-04 [9e38c2bb5] Declare assorted array functions using anycompatible not
+-->
+
+ <para>
+ User-defined aggregates that reference some built-in array functions
+ will need to be recreated (Tom Lane)
+ </para>
+
+ <para>
+ Specifically, user-defined aggregates that use the functions <link
+ linkend="functions-array"><function>array_append()</function></link>,
+ <function>array_prepend()</function>,
+ <function>array_cat()</function>,
+ <function>array_position()</function>,
+ <function>array_positions()</function>,
+ <function>array_remove()</function>,
+ <function>array_replace()</function>, or <link
+ linkend="functions-math"><function>width_bucket()</function></link>
+ as their state transition function must be dropped before
+ upgrading and recreated once the upgrade is complete.
+ </para>
+ </listitem>
+
+ <listitem>
+<!--
+Author: Tom Lane <tgl@sss.pgh.pa.us>
2020-09-17 [76f412ab3] Remove factorial operators, leaving only the factorial()
-->
Bruce Momjian <bruce@momjian.us> writes:
OK, I came up with the attached patch. This is one of the few cases
where the incompatibility is not clearly related to the feature, so I
left the existing item alone and just created a new one with the same
commit message in the incompatibilities section.
I think phrasing this as though user-defined aggregates are the only
pain point is incorrect. For example, a custom operator based on
array_cat would have issues too.
I suggest a treatment more like
Some built-in array-related functions changed signature (Tom Lane)
Certain functions were redefined to take anycompatiblearray instead
of anyarray. While this does not affect ordinary calls, it does
affect code that directly names these functions along with their
argument types; for example, custom aggregates and operators based
on these functions. The affected functions are [ blah, blah ]
regards, tom lane
On Fri, Jun 11, 2021 at 08:56:19PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
OK, I came up with the attached patch. This is one of the few cases
where the incompatibility is not clearly related to the feature, so I
left the existing item alone and just created a new one with the same
commit message in the incompatibilities section.I think phrasing this as though user-defined aggregates are the only
pain point is incorrect. For example, a custom operator based on
array_cat would have issues too.I suggest a treatment more like
Some built-in array-related functions changed signature (Tom Lane)
Certain functions were redefined to take anycompatiblearray instead
of anyarray. While this does not affect ordinary calls, it does
affect code that directly names these functions along with their
argument types; for example, custom aggregates and operators based
on these functions. The affected functions are [ blah, blah ]
OK, I used some of your ideas and tried for something more general;
patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Attachments:
agg.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml
index 058ba7cd4e..9211115690 100644
--- a/doc/src/sgml/release-14.sgml
+++ b/doc/src/sgml/release-14.sgml
@@ -291,6 +291,34 @@ Author: Tom Lane <tgl@sss.pgh.pa.us>
<listitem>
<!--
Author: Tom Lane <tgl@sss.pgh.pa.us>
+2020-11-04 [9e38c2bb5] Declare assorted array functions using anycompatible not
+-->
+
+ <para>
+ User-defined objects that reference some built-in array functions
+ along with their argument types must be recreated (Tom Lane)
+ </para>
+
+ <para>
+ Specifically, <link
+ linkend="functions-array"><function>array_append()</function></link>,
+ <function>array_prepend()</function>,
+ <function>array_cat()</function>,
+ <function>array_position()</function>,
+ <function>array_positions()</function>,
+ <function>array_remove()</function>,
+ <function>array_replace()</function>, or <link
+ linkend="functions-math"><function>width_bucket()</function></link>
+ used to take <type>anyarray</type> arguments but now take
+ <type>anycompatiblearray</type>. Therefore, user-defined objects
+ that reference the old array function signature must be dropped
+ before upgrading and recreated once the upgrade completes.
+ </para>
+ </listitem>
+
+ <listitem>
+<!--
+Author: Tom Lane <tgl@sss.pgh.pa.us>
2020-09-17 [76f412ab3] Remove factorial operators, leaving only the factorial()
-->
Bruce Momjian <bruce@momjian.us> writes:
OK, I used some of your ideas and tried for something more general;
patch attached.
I think it's a good idea to mention custom aggregates and operators
specifically, as otherwise people will look at this and have little
idea what you're on about. I just want wording like "such as custom
aggregates and operators", in case somebody has done some other creative
thing that breaks.
regards, tom lane
On Fri, Jun 11, 2021 at 09:12:55PM -0400, Bruce Momjian wrote:
OK, I used some of your ideas and tried for something more general;
patch attached.
This is good.
But I wonder if "dropped before upgrading" is too specific to pg_upgrade?
Dropping the aggregate before starting a backup to be restored into a new
version seems like a bad way to do it. More likely, I would restore whatever
backup I had, get errors, and then eventually recreate the aggregates.
+ <para> + User-defined objects that reference some built-in array functions + along with their argument types must be recreated (Tom Lane) + </para> + + <para> + Specifically, <link + linkend="functions-array"><function>array_append()</function></link>,
...
Show quoted text
+ used to take <type>anyarray</type> arguments but now take + <type>anycompatiblearray</type>. Therefore, user-defined objects + that reference the old array function signature must be dropped + before upgrading and recreated once the upgrade completes. + </para> + </listitem>
On Fri, Jun 11, 2021 at 09:17:46PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
OK, I used some of your ideas and tried for something more general;
patch attached.I think it's a good idea to mention custom aggregates and operators
specifically, as otherwise people will look at this and have little
idea what you're on about. I just want wording like "such as custom
aggregates and operators", in case somebody has done some other creative
thing that breaks.
Agreed, updated patch attached.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Attachments:
agg.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/release-14.sgml b/doc/src/sgml/release-14.sgml
index 058ba7cd4e..c2d8941206 100644
--- a/doc/src/sgml/release-14.sgml
+++ b/doc/src/sgml/release-14.sgml
@@ -291,6 +291,35 @@ Author: Tom Lane <tgl@sss.pgh.pa.us>
<listitem>
<!--
Author: Tom Lane <tgl@sss.pgh.pa.us>
+2020-11-04 [9e38c2bb5] Declare assorted array functions using anycompatible not
+-->
+
+ <para>
+ User-defined objects that reference some built-in array functions
+ along with their argument types must be recreated (Tom Lane)
+ </para>
+
+ <para>
+ Specifically, <link
+ linkend="functions-array"><function>array_append()</function></link>,
+ <function>array_prepend()</function>,
+ <function>array_cat()</function>,
+ <function>array_position()</function>,
+ <function>array_positions()</function>,
+ <function>array_remove()</function>,
+ <function>array_replace()</function>, or <link
+ linkend="functions-math"><function>width_bucket()</function></link>
+ used to take <type>anyarray</type> arguments but now take
+ <type>anycompatiblearray</type>. Therefore, user-defined objects
+ like aggregates and operators that reference old array function
+ signatures must be dropped before upgrading and recreated once the
+ upgrade completes.
+ </para>
+ </listitem>
+
+ <listitem>
+<!--
+Author: Tom Lane <tgl@sss.pgh.pa.us>
2020-09-17 [76f412ab3] Remove factorial operators, leaving only the factorial()
-->
On Fri, Jun 11, 2021 at 08:19:48PM -0500, Justin Pryzby wrote:
On Fri, Jun 11, 2021 at 09:12:55PM -0400, Bruce Momjian wrote:
OK, I used some of your ideas and tried for something more general;
patch attached.This is good.
But I wonder if "dropped before upgrading" is too specific to pg_upgrade?
Dropping the aggregate before starting a backup to be restored into a new
version seems like a bad way to do it. More likely, I would restore whatever
backup I had, get errors, and then eventually recreate the aggregates.
I am actually unclear on that. Do people really restore a dump and just
ignore errors, or somehow track them and go back and try to fix them.
Isn't there a cascading effect if other things depend on it? How do
they get the object definitions from a huge dump file? What process
should we recommend? I have just never seen good documentation on how
this handled.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
On Fri, Jun 11, 2021 at 09:40:07PM -0400, Bruce Momjian wrote:
On Fri, Jun 11, 2021 at 09:17:46PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
OK, I used some of your ideas and tried for something more general;
patch attached.I think it's a good idea to mention custom aggregates and operators
specifically, as otherwise people will look at this and have little
idea what you're on about. I just want wording like "such as custom
aggregates and operators", in case somebody has done some other creative
thing that breaks.Agreed, updated patch attached.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Hi everyone!
Sorry for bumping old thread.
On 25 May 2021, at 21:14, Justin Pryzby <pryzby@telsasoft.com> wrote:
Such aggregate functions should be dropped before upgrade/restore and then
re-created afterwards using the "anycompatible" functions. The affected
functions are: array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket.
We've just stumbled upon the problem in our service. Would it be backpatchable to add this check to pg_upgrade?
I want to check something like
select * from pg_aggregate join pg_proc on (aggtransfn = pg_proc.oid)
where proname in ('array_append', 'array_prepend','array_cat', 'array_position','array_positions', 'array_remove', 'array_replace', 'width_bucket') ;
select * from pg_operator join pg_proc on (oprcode = pg_proc.oid)
where proname in ('array_append', 'array_prepend','array_cat', 'array_position','array_positions', 'array_remove', 'array_replace', 'width_bucket') and pg_operator.oid >= 16384;
if pg_upgrade is executed with --check option.
Best regards, Andrey Borodin.
On 24 Jun 2022, at 16:09, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Would it be backpatchable to add this check to pg_upgrade?
Just to be clear of what exactly I propose I drafted a patch. PFA.
I've tested it with PG13 and
CREATE AGGREGATE public.array_accum(anyelement) (
SFUNC = array_append,
STYPE = anyarray,
INITCOND = '{}',
PARALLEL = safe
);
CREATE OPERATOR ##% (leftarg=anyarray, rightarg=anyelement,function=array_append);
Operator output currently look a bit strage, but does it's job. pg_upgrade_output.d/operators.txt:
In database: postgres
(oid=16385) ##% in public
Thanks!
Best regards, Andrey Borodin.
Attachments:
v1-0001-Check-incompatible-aggreagtes-and-operators-befor.patchapplication/octet-stream; name=v1-0001-Check-incompatible-aggreagtes-and-operators-befor.patch; x-unix-mode=0644Download
From 6937da6cb590a1bd565cb935d54d880b8dc6a454 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <x4mmm@flight.local>
Date: Fri, 24 Jun 2022 17:27:24 +0500
Subject: [PATCH v1] Check incompatible aggreagtes and operators before upgrade
to 14
---
src/bin/pg_upgrade/check.c | 178 +++++++++++++++++++++++++++++++------
1 file changed, 150 insertions(+), 28 deletions(-)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 6114303b52..8144d0340a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -30,7 +30,7 @@ static void check_for_reg_data_type_usage(ClusterInfo *cluster);
static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
-static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster);
+static void check_for_user_defined_encoding_conversions_aggregates_and_operators(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -113,7 +113,7 @@ check_and_dump_old_cluster(bool live_check)
* need to be changed to match the new signature.
*/
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
- check_for_user_defined_encoding_conversions(&old_cluster);
+ check_for_user_defined_encoding_conversions_aggregates_and_operators(&old_cluster);
/*
* Pre-PG 14 allowed user defined postfix operators, which are not
@@ -1253,32 +1253,47 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
check_ok();
}
+
/*
* Verify that no user-defined encoding conversions exist.
+ * Also check that there are no aggregates and operators that use functions with
+ * pre-anycompatible prototypes.
*/
static void
-check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
+check_for_user_defined_encoding_conversions_aggregates_and_operators(ClusterInfo *cluster)
{
int dbnum;
- FILE *script = NULL;
- bool found = false;
- char output_path[MAXPGPATH];
-
- prep_status("Checking for user-defined encoding conversions");
-
- snprintf(output_path, sizeof(output_path), "%s/%s",
+ FILE *encodings_script = NULL;
+ bool encodings_found = false;
+ char encodings_output_path[MAXPGPATH];
+ FILE *aggregates_script = NULL;
+ bool aggregates_found = false;
+ char aggregates_output_path[MAXPGPATH];
+ FILE *operators_script = NULL;
+ bool operators_found = false;
+ char operators_output_path[MAXPGPATH];
+
+ prep_status("Checking for user-defined encoding conversions, aggregates and operators");
+
+ snprintf(encodings_output_path, sizeof(encodings_output_path), "%s/%s",
log_opts.basedir,
"encoding_conversions.txt");
+ snprintf(aggregates_output_path, sizeof(aggregates_output_path), "%s/%s",
+ log_opts.basedir,
+ "aggregates.txt");
+ snprintf(operators_output_path, sizeof(operators_output_path), "%s/%s",
+ log_opts.basedir,
+ "operators.txt");
- /* Find any user defined encoding conversions */
+ /* Find any user defined encoding conversions, aggregates and operators */
for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
{
PGresult *res;
bool db_used = false;
int ntups;
int rowno;
- int i_conoid,
- i_conname,
+ int i_oid,
+ i_name,
i_nspname;
DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
PGconn *conn = connectToServer(cluster, active_db->db_name);
@@ -1296,25 +1311,103 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
"WHERE c.connamespace = n.oid AND "
" c.oid >= 16384");
ntups = PQntuples(res);
- i_conoid = PQfnumber(res, "conoid");
- i_conname = PQfnumber(res, "conname");
+ i_oid = PQfnumber(res, "conoid");
+ i_name = PQfnumber(res, "conname");
i_nspname = PQfnumber(res, "nspname");
for (rowno = 0; rowno < ntups; rowno++)
{
- found = true;
- if (script == NULL &&
- (script = fopen_priv(output_path, "w")) == NULL)
+ encodings_found = true;
+ if (encodings_script == NULL &&
+ (encodings_script = fopen_priv(encodings_output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s\n",
- output_path, strerror(errno));
+ encodings_output_path, strerror(errno));
if (!db_used)
{
- fprintf(script, "In database: %s\n", active_db->db_name);
+ fprintf(encodings_script, "In database: %s\n", active_db->db_name);
db_used = true;
}
- fprintf(script, " (oid=%s) %s.%s\n",
- PQgetvalue(res, rowno, i_conoid),
+ fprintf(encodings_script, " (oid=%s) %s.%s\n",
+ PQgetvalue(res, rowno, i_oid),
PQgetvalue(res, rowno, i_nspname),
- PQgetvalue(res, rowno, i_conname));
+ PQgetvalue(res, rowno, i_name));
+ }
+
+ PQclear(res);
+
+ db_used = false;
+
+ res = executeQueryOrDie(conn,
+ "SELECT a.aggfnoid::oid, p1.proname, n.nspname "
+ "FROM pg_catalog.pg_aggregate a, "
+ " pg_catalog.pg_proc p, "
+ " pg_catalog.pg_proc p1, "
+ " pg_catalog.pg_namespace n "
+ "WHERE "
+ " a.aggtransfn = p.oid AND "
+ " p1.oid = a.aggfnoid AND "
+ " p1.pronamespace = n.oid AND"
+ " a.aggfnoid >= 16384 AND"
+ " p.proname in ('array_append', 'array_prepend',"
+ " 'array_cat', 'array_position','array_positions', "
+ " 'array_remove', 'array_replace', 'width_bucket')");
+ ntups = PQntuples(res);
+ i_oid = PQfnumber(res, "aggfnoid");
+ i_name = PQfnumber(res, "proname");
+ i_nspname = PQfnumber(res, "nspname");
+ for (rowno = 0; rowno < ntups; rowno++)
+ {
+ aggregates_found = true;
+ if (aggregates_script == NULL &&
+ (aggregates_script = fopen_priv(aggregates_output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %s\n",
+ aggregates_output_path, strerror(errno));
+ if (!db_used)
+ {
+ fprintf(aggregates_script, "In database: %s\n", active_db->db_name);
+ db_used = true;
+ }
+ fprintf(aggregates_script, " (oid=%s) %s.%s\n",
+ PQgetvalue(res, rowno, i_oid),
+ PQgetvalue(res, rowno, i_nspname),
+ PQgetvalue(res, rowno, i_name));
+ }
+
+ PQclear(res);
+
+ db_used = false;
+
+ res = executeQueryOrDie(conn,
+ "SELECT o.oid::oid, o.oprname, n.nspname "
+ "FROM pg_catalog.pg_operator o, "
+ " pg_catalog.pg_proc p, "
+ " pg_catalog.pg_namespace n "
+ "WHERE "
+ " o.oprcode = p.oid AND "
+ " o.oprnamespace = n.oid AND"
+ " o.oid >= 16384 AND"
+ " p.proname in ('array_append', 'array_prepend',"
+ " 'array_cat', 'array_position','array_positions', "
+ " 'array_remove', 'array_replace', 'width_bucket')");
+ ntups = PQntuples(res);
+ i_oid = PQfnumber(res, "oid");
+ i_name = PQfnumber(res, "oprname");
+ i_nspname = PQfnumber(res, "nspname");
+ for (rowno = 0; rowno < ntups; rowno++)
+ {
+ operators_found = true;
+ if (operators_script == NULL &&
+ (operators_script = fopen_priv(operators_output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %s\n",
+ operators_output_path, strerror(errno));
+ if (!db_used)
+ {
+ fprintf(operators_script, "In database: %s\n", active_db->db_name);
+ db_used = true;
+ }
+ fprintf(operators_script, " (oid=%s) %s in %s\n",
+ PQgetvalue(res, rowno, i_oid),
+ PQgetvalue(res, rowno, i_name),
+ PQgetvalue(res, rowno, i_nspname));
}
PQclear(res);
@@ -1322,10 +1415,14 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
PQfinish(conn);
}
- if (script)
- fclose(script);
+ if (encodings_script)
+ fclose(encodings_script);
+ if (aggregates_script)
+ fclose(aggregates_script);
+ if (operators_script)
+ fclose(operators_script);
- if (found)
+ if (encodings_found)
{
pg_log(PG_REPORT, "fatal\n");
pg_fatal("Your installation contains user-defined encoding conversions.\n"
@@ -1333,9 +1430,34 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
"so this cluster cannot currently be upgraded. You can remove the\n"
"encoding conversions in the old cluster and restart the upgrade.\n"
"A list of user-defined encoding conversions is in the file:\n"
- " %s\n\n", output_path);
+ " %s\n\n", encodings_output_path);
}
- else
+
+ if (aggregates_found)
+ {
+ pg_log(PG_REPORT, "fatal\n");
+ pg_fatal("Your installation contains user-defined aggreagates.\n"
+ "HERE WE NEED A GOOD EXPLANATION OF WHAT IS GOING ON\n"
+ "The conversion function parameters changed in PostgreSQL version 14\n"
+ "so this cluster cannot currently be upgraded. You can remove the\n"
+ "encoding conversions in the old cluster and restart the upgrade.\n"
+ "A list of user-defined encoding conversions is in the file:\n"
+ " %s\n\n", aggregates_output_path);
+ }
+
+ if (operators_found)
+ {
+ pg_log(PG_REPORT, "fatal\n");
+ pg_fatal("Your installation contains user-defined operators.\n"
+ "HERE WE NEED A GOOD EXPLANATION OF WHAT IS GOING ON\n"
+ "The conversion function parameters changed in PostgreSQL version 14\n"
+ "so this cluster cannot currently be upgraded. You can remove the\n"
+ "encoding conversions in the old cluster and restart the upgrade.\n"
+ "A list of user-defined encoding conversions is in the file:\n"
+ " %s\n\n", operators_output_path);
+ }
+
+ if (!(encodings_found || aggregates_found || operators_found))
check_ok();
}
--
2.32.0 (Apple Git-132)
On Fri, Jun 24, 2022 at 04:09:46PM +0500, Andrey Borodin wrote:
Hi everyone!
Sorry for bumping old thread.
Please find this newer thread+patch here ;)
/messages/by-id/20220614230949.GX29853@telsasoft.com
On 25 May 2021, at 21:14, Justin Pryzby <pryzby@telsasoft.com> wrote:
Such aggregate functions should be dropped before upgrade/restore and then
re-created afterwards using the "anycompatible" functions. The affected
functions are: array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket.We've just stumbled upon the problem in our service. Would it be backpatchable to add this check to pg_upgrade?
I guess you mean to backpatch to v14 for people upgrading from v13.
I realized that my latest patch would break upgrades from old servers, which do
not have array_position/s nor width_bucket, so ::reprocedure would fail. Maybe
Andrey's way is better (checking proname rather than its OID).
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
I realized that my latest patch would break upgrades from old servers, which do
not have array_position/s nor width_bucket, so ::reprocedure would fail. Maybe
Andrey's way is better (checking proname rather than its OID).
proname is dangerous, because there's nothing stopping users from
adding more functions with the same name.
Just use a server-version-dependent list of regprocedure OIDs.
regards, tom lane
On 24 Jun 2022, at 18:30, Justin Pryzby <pryzby@telsasoft.com> wrote:
On Fri, Jun 24, 2022 at 04:09:46PM +0500, Andrey Borodin wrote:
Hi everyone!
Sorry for bumping old thread.
Please find this newer thread+patch here ;)
/messages/by-id/20220614230949.GX29853@telsasoft.com
Oops. Let's discard my patch and I'll review yours. Many thanks for fixing this stuff :)
On 24 Jun 2022, at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
I realized that my latest patch would break upgrades from old servers, which do
not have array_position/s nor width_bucket, so ::reprocedure would fail. Maybe
Andrey's way is better (checking proname rather than its OID).proname is dangerous, because there's nothing stopping users from
adding more functions with the same name.Just use a server-version-dependent list of regprocedure OIDs.
Server-version-dependent list of oids seems more error prone. I think we can just check proname by the list and oid < 16384.
Thanks!
Best regards, Andrey Borodin.