Horton

Thread

Commits

  1. doc: add PG 14 relnote item about array function references

  2. Work around cross-version-upgrade issues created by commit 9e38c2bb5.

  1. array_cat anycompatible change is breaking xversion upgrade tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-05T00:43:51Z

    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

  2. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-08T23:47:22Z

    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

  3. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-20T23:29:22Z

    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

  4. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-20T23:35:10Z

    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

  5. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-25T16:14:58Z

    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

  6. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-08T22:56:18Z

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

  7. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-09T00:02:46Z

    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.

  8. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-09T00:10:00Z

    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

  9. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-12T00:32:44Z

    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.

  10. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T00:56:19Z

    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

  11. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-12T01:12:55Z

    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.

  12. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-12T01:17:46Z

    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

  13. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-12T01:19:48Z

    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>

  14. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-12T01:40:07Z

    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.

  15. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-12T01:42:41Z

    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.

  16. Re: array_cat anycompatible change is breaking xversion upgrade tests (v14 release notes)

    Bruce Momjian <bruce@momjian.us> — 2021-06-14T16:49:19Z

    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.

  17. Re: array_cat anycompatible change is breaking xversion upgrade tests

    x4mmm@yandex-team.ru — 2022-06-24T11:09:46Z

    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.

  18. Re: array_cat anycompatible change is breaking xversion upgrade tests

    x4mmm@yandex-team.ru — 2022-06-24T12:46:19Z

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

  19. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Justin Pryzby <pryzby@telsasoft.com> — 2022-06-24T13:30:53Z

    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

  20. Re: array_cat anycompatible change is breaking xversion upgrade tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-06-24T14:06:53Z

    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

  21. Re: array_cat anycompatible change is breaking xversion upgrade tests

    x4mmm@yandex-team.ru — 2022-06-24T18:28:31Z

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