Обсуждение: pgsql: Declare assorted array functions using anycompatible not anyelem

Declare assorted array functions using anycompatible not anyelement.

Convert array_append, array_prepend, array_cat, array_position,
array_positions, array_remove, array_replace, and width_bucket
to use anycompatiblearray.  This is a simple extension of commit
5c292e6b9 to hit some other places where there's a pretty obvious
gain in usability from doing so.

Ideally we'd also modify other functions taking multiple old-style
polymorphic arguments.  But most of the remainder are tied into one
or more operator classes, making any such change a much larger can of
worms than I desire to open right now.

Discussion: https://postgr.es/m/77675130-89da-dab1-51dd-492c93dcf5d1@postgresfriends.org

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/9e38c2bb5093ceb0c04d6315ccd8975bd17add66

Modified Files
--------------
doc/src/sgml/func.sgml                     | 50 ++++++++++++++++--------------
doc/src/sgml/xaggr.sgml                    |  4 +--
src/include/catalog/catversion.h           |  2 +-
src/include/catalog/pg_operator.dat        | 13 ++++----
src/include/catalog/pg_proc.dat            | 40 +++++++++++++++---------
src/test/regress/expected/arrays.out       | 12 +++++++
src/test/regress/expected/polymorphism.out | 10 +++---
src/test/regress/sql/arrays.sql            |  2 ++
src/test/regress/sql/polymorphism.sql      | 10 +++---
9 files changed, 86 insertions(+), 57 deletions(-)

On 11/4/20 4:10 PM, Tom Lane wrote:
> Declare assorted array functions using anycompatible not anyelement.
>
> Convert array_append, array_prepend, array_cat, array_position,
> array_positions, array_remove, array_replace, and width_bucket
> to use anycompatiblearray.  This is a simple extension of commit
> 5c292e6b9 to hit some other places where there's a pretty obvious
> gain in usability from doing so.
>
> Ideally we'd also modify other functions taking multiple old-style
> polymorphic arguments.  But most of the remainder are tied into one
> or more operator classes, making any such change a much larger can of
> worms than I desire to open right now.
>
> Discussion: https://postgr.es/m/77675130-89da-dab1-51dd-492c93dcf5d1@postgresfriends.org
>


This patch broke cross-version pg_upgrade testing. I have cured crake
for the moment by having it execute this in the source database:

    drop aggregate if exists public.array_cat_accum(anyarray);
    drop aggregate if exists public.first_el_agg_any(anyelement);

But I wonder if we really want to make it impossible to upgrade
databases with aggregates that contain these functions?


cheers


andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/4/20 4:10 PM, Tom Lane wrote:
>> Declare assorted array functions using anycompatible not anyelement.

> This patch broke cross-version pg_upgrade testing.

Uh, yeah ... didn't you get my two prior messages about that?

> I have cured crake
> for the moment by having it execute this in the source database:

I think probably the right fix is just to change that test case to
use a different implementation function, per [1].  I'm holding off
pushing the fix till after this week's wraps, though.

> But I wonder if we really want to make it impossible to upgrade
> databases with aggregates that contain these functions?

If I thought that user-defined aggregates relying on array_cat were
really a thing (and not just a test case), I'd be more concerned about
this.  But it's hard to see why users shouldn't use array_agg() instead
of rolling their own.

            regards, tom lane

[1] https://www.postgresql.org/message-id/1915573.1604879242%40sss.pgh.pa.us

On 11/9/20 4:29 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/4/20 4:10 PM, Tom Lane wrote:
>>> Declare assorted array functions using anycompatible not anyelement.
>> This patch broke cross-version pg_upgrade testing.
> Uh, yeah ... didn't you get my two prior messages about that?



Yes, I did, although by the time you sent them out I'd already modified
crake.



>
>> I have cured crake
>> for the moment by having it execute this in the source database:
> I think probably the right fix is just to change that test case to
> use a different implementation function, per [1].  I'm holding off
> pushing the fix till after this week's wraps, though.



I'd be ok with that. Can we devise a fix that will work all the way back
to 9.2, which is where we start upgrade testing?


>
>> But I wonder if we really want to make it impossible to upgrade
>> databases with aggregates that contain these functions?
> If I thought that user-defined aggregates relying on array_cat were
> really a thing (and not just a test case), I'd be more concerned about
> this.  But it's hard to see why users shouldn't use array_agg() instead
> of rolling their own.
>
>     



Possibly something that's been migrated from before we had array_agg.


cheers


andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/9/20 4:29 PM, Tom Lane wrote:
>> I think probably the right fix is just to change that test case to
>> use a different implementation function, per [1].  I'm holding off
>> pushing the fix till after this week's wraps, though.

> I'd be ok with that. Can we devise a fix that will work all the way back
> to 9.2, which is where we start upgrade testing?

Hm.  To fix it this way, we'd have to push the test-script change
into the pre-9.5 branches.  There's no technical reason we can't do
that, I don't think, though it's a bit outside our normal practices.

>> If I thought that user-defined aggregates relying on array_cat were
>> really a thing (and not just a test case), I'd be more concerned about
>> this.  But it's hard to see why users shouldn't use array_agg() instead
>> of rolling their own.

> Possibly something that's been migrated from before we had array_agg.

array_agg goes back to 8.4, and for at least most of that time it's
been very much more efficient than anything based on array_cat.  So I
think it's past time to push any such laggards into the 21st century.

            regards, tom lane

On 11/9/20 5:15 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/9/20 4:29 PM, Tom Lane wrote:
>>> I think probably the right fix is just to change that test case to
>>> use a different implementation function, per [1].  I'm holding off
>>> pushing the fix till after this week's wraps, though.
>> I'd be ok with that. Can we devise a fix that will work all the way back
>> to 9.2, which is where we start upgrade testing?
> Hm.  To fix it this way, we'd have to push the test-script change
> into the pre-9.5 branches.  There's no technical reason we can't do
> that, I don't think, though it's a bit outside our normal practices.



I doubt it's necessary. We have provision for adjusting the source
database before we try to upgrade it. Let me know exactly what you have
in mind and I'll try to make it work without having to mangle the
supposedly frozen back branches.


We don't actually run the regression suite regularly for non-live
branches, we just try to upgrade a completely static data dir.



>
>>> If I thought that user-defined aggregates relying on array_cat were
>>> really a thing (and not just a test case), I'd be more concerned about
>>> this.  But it's hard to see why users shouldn't use array_agg() instead
>>> of rolling their own.
>> Possibly something that's been migrated from before we had array_agg.
> array_agg goes back to 8.4, and for at least most of that time it's
> been very much more efficient than anything based on array_cat.  So I
> think it's past time to push any such laggards into the 21st century.
>
>             



fair enough


cheers


andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/9/20 5:15 PM, Tom Lane wrote:
>> Hm.  To fix it this way, we'd have to push the test-script change
>> into the pre-9.5 branches.  There's no technical reason we can't do
>> that, I don't think, though it's a bit outside our normal practices.

> I doubt it's necessary. We have provision for adjusting the source
> database before we try to upgrade it. Let me know exactly what you have
> in mind and I'll try to make it work without having to mangle the
> supposedly frozen back branches.

> We don't actually run the regression suite regularly for non-live
> branches, we just try to upgrade a completely static data dir.

What I have in mind to apply to 9.5 through 13 is per attached.
You could either redefine the aggregate similarly in 9.2-9.4,
or just drop it.  I doubt the latter leads to any interesting
loss of coverage for xversion upgrade, as there's plenty of other
user-defined aggregates in the regression database.

            regards, tom lane

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 11/9/20 5:41 PM, Tom Lane wrote:
>
> What I have in mind to apply to 9.5 through 13 is per attached.
> You could either redefine the aggregate similarly in 9.2-9.4,
> or just drop it.  I doubt the latter leads to any interesting
> loss of coverage for xversion upgrade, as there's plenty of other
> user-defined aggregates in the regression database.



You also need to modify first_el_agg_any.


Here's what I have working on crake::


        if ($oversion le 'REL9_4_STABLE')
        {
            # this is to be fixed in 9.5 and later
            $prstmt = join(';',
                           'drop aggregate if exists
public.array_cat_accum(anyarray)',
                           'CREATE AGGREGATE array_larger_accum
(anyarray) ' .
                           ' ( ' .
                           '   sfunc = array_larger, ' .
                           '   stype = anyarray, ' .
                           '   initcond = $${}$$ ' .
                           '  ) ' );
            system( "$other_branch/inst/bin/psql -X -e "
                      . " -c '" . $prstmt . "' "
                      . "regression "
                      . ">> '$upgrade_loc/$oversion-copy.log' 2>&1");
            return if $?;
        }
        # drop this branch when upstream is renovated
        else
        {
            $prstmt = join(';',
                          'drop aggregate if exists
public.array_cat_accum(anyarray)',
                          'drop aggregate if exists
public.first_el_agg_any(anyelement)');
            system( "$other_branch/inst/bin/psql -X -e "
                      . " -c '$prstmt' "
                      . "regression "
                      . ">> '$upgrade_loc/$oversion-copy.log' 2>&1");
            return if $?;
        }


When you fix the live branches I'll delete the "else" branch and push
the change to the git repo.


cheers


andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/9/20 5:41 PM, Tom Lane wrote:
>> What I have in mind to apply to 9.5 through 13 is per attached.

> You also need to modify first_el_agg_any.

... doh.  That one is a little messier because there's no equally useful
substitute function.  What I'm inclined to do for that is to add a
SQL-language wrapper to reproduce the old signature for array_append.
As attached.  (I'm keeping this separate from the other patch, since
this doesn't go back as far.)

> Here's what I have working on crake::

OK.

            regards, tom lane

diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 2c3bb0a60b..628a39612a 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -786,16 +786,18 @@ create aggregate build_group(int8, integer) (
   STYPE = int8[]
 );
 -- check proper resolution of data types for polymorphic transfn/finalfn
-create function first_el(anycompatiblearray) returns anycompatible as
+create function first_el_transfn(anyarray, anyelement) returns anyarray as
+'select $1 || $2' language sql immutable;
+create function first_el(anyarray) returns anyelement as
 'select $1[1]' language sql strict immutable;
 create aggregate first_el_agg_f8(float8) (
   SFUNC = array_append,
   STYPE = float8[],
   FINALFUNC = first_el
 );
-create aggregate first_el_agg_any(anycompatible) (
-  SFUNC = array_append,
-  STYPE = anycompatiblearray,
+create aggregate first_el_agg_any(anyelement) (
+  SFUNC = first_el_transfn,
+  STYPE = anyarray,
   FINALFUNC = first_el
 );
 select first_el_agg_f8(x::float8) from generate_series(1,10) x;
diff --git a/src/test/regress/sql/polymorphism.sql b/src/test/regress/sql/polymorphism.sql
index 70a21c8978..d98a74c3f6 100644
--- a/src/test/regress/sql/polymorphism.sql
+++ b/src/test/regress/sql/polymorphism.sql
@@ -549,7 +549,10 @@ create aggregate build_group(int8, integer) (

 -- check proper resolution of data types for polymorphic transfn/finalfn

-create function first_el(anycompatiblearray) returns anycompatible as
+create function first_el_transfn(anyarray, anyelement) returns anyarray as
+'select $1 || $2' language sql immutable;
+
+create function first_el(anyarray) returns anyelement as
 'select $1[1]' language sql strict immutable;

 create aggregate first_el_agg_f8(float8) (
@@ -558,9 +561,9 @@ create aggregate first_el_agg_f8(float8) (
   FINALFUNC = first_el
 );

-create aggregate first_el_agg_any(anycompatible) (
-  SFUNC = array_append,
-  STYPE = anycompatiblearray,
+create aggregate first_el_agg_any(anyelement) (
+  SFUNC = first_el_transfn,
+  STYPE = anyarray,
   FINALFUNC = first_el
 );

diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 1ff40764d9..236b2532b3 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -786,6 +786,8 @@ create aggregate build_group(int8, integer) (
   STYPE = int8[]
 );
 -- check proper resolution of data types for polymorphic transfn/finalfn
+create function first_el_transfn(anyarray, anyelement) returns anyarray as
+'select $1 || $2' language sql immutable;
 create function first_el(anyarray) returns anyelement as
 'select $1[1]' language sql strict immutable;
 create aggregate first_el_agg_f8(float8) (
@@ -794,7 +796,7 @@ create aggregate first_el_agg_f8(float8) (
   FINALFUNC = first_el
 );
 create aggregate first_el_agg_any(anyelement) (
-  SFUNC = array_append,
+  SFUNC = first_el_transfn,
   STYPE = anyarray,
   FINALFUNC = first_el
 );
diff --git a/src/test/regress/sql/polymorphism.sql b/src/test/regress/sql/polymorphism.sql
index e5222f1f81..d2e302c330 100644
--- a/src/test/regress/sql/polymorphism.sql
+++ b/src/test/regress/sql/polymorphism.sql
@@ -549,6 +549,9 @@ create aggregate build_group(int8, integer) (

 -- check proper resolution of data types for polymorphic transfn/finalfn

+create function first_el_transfn(anyarray, anyelement) returns anyarray as
+'select $1 || $2' language sql immutable;
+
 create function first_el(anyarray) returns anyelement as
 'select $1[1]' language sql strict immutable;

@@ -559,7 +562,7 @@ create aggregate first_el_agg_f8(float8) (
 );

 create aggregate first_el_agg_any(anyelement) (
-  SFUNC = array_append,
+  SFUNC = first_el_transfn,
   STYPE = anyarray,
   FINALFUNC = first_el
 );

On 11/10/20 3:31 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 11/9/20 5:41 PM, Tom Lane wrote:
>>> What I have in mind to apply to 9.5 through 13 is per attached.
>> You also need to modify first_el_agg_any.
> ... doh.  That one is a little messier because there's no equally useful
> substitute function.  What I'm inclined to do for that is to add a
> SQL-language wrapper to reproduce the old signature for array_append.
> As attached.  (I'm keeping this separate from the other patch, since
> this doesn't go back as far.)
>
>

Seems reasonable.


cheers


andrew