Thread: 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

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
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

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

@Bruce: Would you add something about this to the release notes before beta2?

I added it as an OpenItem.

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.

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

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>,
...
> +      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 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.

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.

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 ;)
https://www.postgresql.org/message-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 ;)
> https://www.postgresql.org/message-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.