Re: pg_upgrade (12->14) fails on aggregate
On Tue, Jul 05, 2022 at 11:29:03PM -0400, Tom Lane wrote: > Yeah. I think that 08385ed26 fixes this, but we've had no new > reports yet :-( Indeed. Things are right now. Thanks! -- Michael signature.asc Description: PGP signature
Re: pg_upgrade (12->14) fails on aggregate
Michael Paquier writes: > crake and drongo look unhappy after that, as of the upgrade from 9.6: Yeah. I think that 08385ed26 fixes this, but we've had no new reports yet :-( regards, tom lane
Re: pg_upgrade (12->14) fails on aggregate
On Tue, Jul 05, 2022 at 01:07:48PM -0400, Tom Lane wrote: > It looks about ready to me. Pushed with some minor cosmetic > adjustments. crake and drongo look unhappy after that, as of the upgrade from 9.6: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake=2022-07-05%2020%3A48%3A21 https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2022-07-05%2019%3A06%3A04 Checking for incompatible polymorphic functions fatal The dumps used by the buildfarm may need some adjustments, it seems. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade (12->14) fails on aggregate
Andrey Borodin writes: > The patch is marked WiP, what is in progress as of now? It looks about ready to me. Pushed with some minor cosmetic adjustments. regards, tom lane
Re: pg_upgrade (12->14) fails on aggregate
> On 29 Jun 2022, at 23:07, Justin Pryzby wrote: > > On Wed, Jun 29, 2022 at 10:58:44PM +0500, Andrey Borodin wrote: >>> On 28 Jun 2022, at 04:30, Justin Pryzby wrote: >>> >>> Nope, it's as I said: this would break pg_upgrade from older versions. >> >> As far as I understand 9.5 is not supported. Probably, it makes sense to >> keep pg_upgrade running against 9.5 clusters, but I'm not sure if we do this >> routinely. > > As of last year, there's a reasonably clear policy for support of old > versions: > > https://www.postgresql.org/docs/devel/pgupgrade.html > |pg_upgrade supports upgrades from 9.2.X and later to the current major > release of PostgreSQL, including snapshot and beta releases. This makes sense, thank you for clarification. The patch is marked WiP, what is in progress as of now? Best regards, Andrey Borodin.
Re: pg_upgrade (12->14) fails on aggregate
On Wed, Jun 29, 2022 at 10:58:44PM +0500, Andrey Borodin wrote: > > On 28 Jun 2022, at 04:30, Justin Pryzby wrote: > > > > Nope, it's as I said: this would break pg_upgrade from older versions. > > As far as I understand 9.5 is not supported. Probably, it makes sense to keep > pg_upgrade running against 9.5 clusters, but I'm not sure if we do this > routinely. As of last year, there's a reasonably clear policy for support of old versions: https://www.postgresql.org/docs/devel/pgupgrade.html |pg_upgrade supports upgrades from 9.2.X and later to the current major release of PostgreSQL, including snapshot and beta releases. See: e469f0aaf3c586c8390bd65923f97d4b1683cd9f -- Justin
Re: pg_upgrade (12->14) fails on aggregate
> On 28 Jun 2022, at 04:30, Justin Pryzby wrote: > > Nope, it's as I said: this would break pg_upgrade from older versions. As far as I understand 9.5 is not supported. Probably, it makes sense to keep pg_upgrade running against 9.5 clusters, but I'm not sure if we do this routinely. Besides this the patch seems to be RfC. Best regards, Andrey Borodin.
Re: pg_upgrade (12->14) fails on aggregate
On Sat, Jun 25, 2022 at 03:34:49PM +0500, Andrey Borodin wrote:
> > On 25 Jun 2022, at 01:28, Justin Pryzby wrote:
>
> > this is my latest.
> > <0001-WIP-pg_upgrade-check-detect-old-polymorphics-from-pr.patch>
>
> Let's rename "databases_with_old_polymorphics.txt" to somthing like
> "old_polymorphics.txt" or maybe even "incompatible_polymorphics_usage.txt"?
> I think you will come up with a better name, my point is here everythin is in
> "databases", and "old" doesn't describe essence of the problem.
> Also, let's check that oid of used functions belong to system catalog
> (<16384)? We don't care about user-defined functions with the same name.
Right now, we test
=ANY(ARRAY['array_remove(anyarray,anyelement)',...]::regprocedure)
..which will find the system's array_remove, and not some other one, due to
ALWAYS_SECURE_SEARCH_PATH_SQL (which is also why ::regprocedure prints a
namespace for the non-system functions we're interested in displaying).
I had "transnsp.nspname='pg_catalog'", which was redundant, so I removed it.
I tested that this allows upgrades with aggregates on top of non-system
functions of the same name/args:
postgres=# CREATE FUNCTION array_append(anyarray, anyelement) RETURNS ANYARRAY
LANGUAGE SQL AS $$ $$;
postgres=# CREATE AGGREGATE foo(anyelement) (sfunc=public.array_append,
stype=anyarray, initcond='{}');
> And, probably, we can do this unconditionally:
> if (old_cluster.major_version >= 9500)
> appendPQExpBufferStr(_polymorphics,
> Nothing bad will happen if we blacklist usage of nonexistent functions.
Nope, it's as I said: this would break pg_upgrade from older versions.
> 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).
This fixes several error with the version test.
--
Justin
>From 965deb773dd0170018f4b82d27420c61690ad690 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH] WIP: pg_upgrade --check: detect old polymorphics from pre-14
These fail when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check, and not after dumping the cluster and in the middle of
restoring it.
CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
CREATE OPERATOR !@# (PROCEDURE = array_append, LEFTARG=anyarray, rightarg=anyelement);
See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
src/bin/pg_upgrade/check.c | 131 +
1 file changed, 131 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..20e7923c20c 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ 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_incompatible_polymorphics(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
check_for_user_defined_postfix_ops(_cluster);
+ /*
+ * PG 14 changed polymorphic functions from anyarray to anycompatiblearray.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+ check_for_incompatible_polymorphics(_cluster);
+
/*
* Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
* supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,130 @@ check_proper_datallowconn(ClusterInfo *cluster)
}
+/*
+ * check_for_incompatible_polymorphics()
+ *
+ * Make sure nothing is using old polymorphic functions with
+ * anyarray/anyelement rather than the new anycompatible variants.
+ */
+static void
+check_for_incompatible_polymorphics(ClusterInfo *cluster)
+{
+ PGresult *res;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+ PQExpBufferData old_polymorphics;
+
+ initPQExpBuffer(_polymorphics);
+
+ if (GET_MAJOR_VERSION(old_cluster.major_version) >= 903)
+ appendPQExpBufferStr(_polymorphics,
+ "'array_remove(anyarray,anyelement)', "
+ "'array_replace(anyarray,anyelement,anyelement)', ");
+
+ if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
+ appendPQExpBufferStr(_polymorphics,
+ "'array_position(anyarray,anyelement)', "
+ "'array_position(anyarray,anyelement,integer)', "
+ "'array_positions(anyarray,anyelement)', "
+ "'width_bucket(anyelement,anyarray)', ");
+
+ appendPQExpBufferStr(_polymorphics,
+ "'array_append(anyarray,anyelement)', "
+ "'array_cat(anyarray,anyarray)', "
+
Re: pg_upgrade (12->14) fails on aggregate
> On 25 Jun 2022, at 01:28, Justin Pryzby wrote: > > But what I wrote already shows what you want. Just tested that, you are right. My version was printing name, I didn't know regproc prints so nice definition. > this is my latest. > <0001-WIP-pg_upgrade-check-detect-old-polymorphics-from-pr.patch> Let's rename "databases_with_old_polymorphics.txt" to somthing like "old_polymorphics.txt" or maybe even "incompatible_polymorphics_usage.txt"? I think you will come up with a better name, my point is here everythin is in "databases", and "old" doesn't describe essence of the problem. Also, let's check that oid of used functions belong to system catalog (<16384)? We don't care about user-defined functions with the same name. And, probably, we can do this unconditionally: if (old_cluster.major_version >= 9500) appendPQExpBufferStr(_polymorphics, Nothing bad will happen if we blacklist usage of nonexistent functions. I see there's a lot of code to have a dynamic list, if you think this exclusion for pre-9.5 is justified - OK, from my POV we can keep this code. These comment is unneeded too: // "AND aggtranstype='anyarray'::regtype Thank you! Best regards, Andrey Borodin.
Re: pg_upgrade (12->14) fails on aggregate
On Fri, Jun 24, 2022 at 11:43:18PM +0500, Andrey Borodin wrote:
> > On 23 Jun 2022, at 04:58, Justin Pryzby wrote:
> >
> > On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
> >> Robert Haas writes:
> >>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby
> >>> wrote:
> To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
> >>
> >>> Extensions can be installed into pg_catalog, but they can't get
> >>> low-numbered OIDs.
> >>
> >> Exactly. (To be clear, I had in mind writing something involving
> >> FirstNormalObjectId, not that you should put literal "16384" in the
> >> code.)
> >
> > Actually, 16384 is already used in two other places in check.c, so ...
>
> Yes, but it's a third copy of the comment ("* The query below hardcodes
> FirstNormalObjectId as 16384 rather than") across the file.
>
> Also, we can return slightly more information about found objects. For
> example, operator will look like "operator: ||". At least we can get nspname
> and oid. And, maybe return type for aggregator and leftarg\rightarg types for
> operator?
But what I wrote already shows what you want.
In database: postgres
aggregate: public.array_accum(anyelement)
operator: public.!@#(anyarray,anyelement)
In my testing, this works great - it shows what you need to put in your DROP
command. If you try it and still wanted the OID, I'll add it for consistency
with check_for_user_defined_{encoding_conversions,postfix_ops}
> BTW comment /* Before v11, used proisagg=true, and afterwards uses
> prokind='a' */ seems interesting, but irrelevant. We join with pg_aggregate
> anyway.
Yes, that's why the query doesn't need to include that.
Something is broken in my old clusters and I can't test all the upgrades right
now, but this is my latest.
>From 87132a8eb7310c4f00d33ea09d97fab481ea1173 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH] WIP: pg_upgrade --check: detect old polymorphics from pre-14
These fail when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check, and not after dumping the cluster and in the middle of
restoring it.
CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
CREATE OPERATOR !@# (PROCEDURE = array_append, LEFTARG=anyarray, rightarg=anyelement);
See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
src/bin/pg_upgrade/check.c | 134 +
1 file changed, 134 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..8b8509b6aa5 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ 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_old_polymorphics(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
check_for_user_defined_postfix_ops(_cluster);
+ /*
+ * PG 14 changed polymorphic functions from anyarray to anycompatiblearray.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+ check_for_old_polymorphics(_cluster);
+
/*
* Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
* supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,133 @@ check_proper_datallowconn(ClusterInfo *cluster)
}
+/*
+ * check_for_old_polymorphics()
+ *
+ * Make sure nothing is using old polymorphic functions with
+ * anyarray/anyelement rather than the new anycompatible variants.
+ */
+static void
+check_for_old_polymorphics(ClusterInfo *cluster)
+{
+ PGresult *res;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+ PQExpBufferData old_polymorphics;
+
+ initPQExpBuffer(_polymorphics);
+
+ appendPQExpBufferStr(_polymorphics,
+ "'array_append(anyarray,anyelement)', "
+ "'array_cat(anyarray,anyarray)', "
+ "'array_prepend(anyelement,anyarray)', "
+ "'array_remove(anyarray,anyelement)', "
+ "'array_replace(anyarray,anyelement,anyelement)' ");
+
+ if (old_cluster.major_version >= 9500)
+ appendPQExpBufferStr(_polymorphics,
+ ", "
+ "'array_position(anyarray,anyelement)', "
+ "'array_position(anyarray,anyelement,integer)', "
+ "'array_positions(anyarray,anyelement)', "
+ "'width_bucket(anyelement,anyarray)' ");
+
+ prep_status("Checking for use of old polymorphic functions");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "databases_with_old_polymorphics.txt");
+
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ bool db_used =
Re: pg_upgrade (12->14) fails on aggregate
> On 23 Jun 2022, at 04:58, Justin Pryzby wrote:
>
> On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
>> Robert Haas writes:
>>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote:
To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
>>
>>> Extensions can be installed into pg_catalog, but they can't get
>>> low-numbered OIDs.
>>
>> Exactly. (To be clear, I had in mind writing something involving
>> FirstNormalObjectId, not that you should put literal "16384" in the
>> code.)
>
> Actually, 16384 is already used in two other places in check.c, so ...
Yes, but it's a third copy of the comment ("* The query below hardcodes
FirstNormalObjectId as 16384 rather than") across the file.
Also, we can return slightly more information about found objects. For example,
operator will look like "operator: ||". At least we can get nspname and oid.
And, maybe return type for aggregator and leftarg\rightarg types for operator?
BTW comment /* Before v11, used proisagg=true, and afterwards uses prokind='a'
*/ seems interesting, but irrelevant. We join with pg_aggregate anyway.
Thanks!
Best regards, Andrey Borodin.
Re: pg_upgrade (12->14) fails on aggregate
On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
> Robert Haas writes:
> > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote:
> >> To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
>
> > Extensions can be installed into pg_catalog, but they can't get
> > low-numbered OIDs.
>
> Exactly. (To be clear, I had in mind writing something involving
> FirstNormalObjectId, not that you should put literal "16384" in the
> code.)
Actually, 16384 is already used in two other places in check.c, so ...
done like that for consistency.
Also fixes parenthesis, typos, and renames vars.
>From 44a66cddbc1f32c16ee7229b85fe76aada4689aa Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH v3] WIP: pg_upgrade --check: detect old polymorphics from
pre-14
These fail when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check, and not after dumping the cluster and in the middle of
restoring it.
CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
CREATE OPERATOR !@# (PROCEDURE = array_append, LEFTARG=anyarray, rightarg=anyelement);
See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
src/bin/pg_upgrade/check.c | 124 +
1 file changed, 124 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..da1781237ba 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ 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_old_polymorphics(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
check_for_user_defined_postfix_ops(_cluster);
+ /*
+ * PG 14 changed polymorphic functions from anyarray to anycompatiblearray.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+ check_for_old_polymorphics(_cluster);
+
/*
* Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
* supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,123 @@ check_proper_datallowconn(ClusterInfo *cluster)
}
+/*
+ * check_for_old_polymorphics()
+ *
+ * Make sure nothing is using old polymorphic functions with
+ * anyarray/anyelement rather than the new anycompatible variants.
+ */
+static void
+check_for_old_polymorphics(ClusterInfo *cluster)
+{
+ PGresult *res;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for use of old polymorphic functions");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "databases_with_old_polymorphics.txt");
+
+#define OLD_POLYMORPHICS \
+ "'array_append(anyarray,anyelement)', " \
+ "'array_cat(anyarray,anyarray)', " \
+ "'array_position(anyarray,anyelement)', " \
+ "'array_position(anyarray,anyelement,integer)', " \
+ "'array_positions(anyarray,anyelement)', " \
+ "'array_prepend(anyelement,anyarray)', " \
+ "'array_remove(anyarray,anyelement)', " \
+ "'array_replace(anyarray,anyelement,anyelement)', " \
+ "'width_bucket(anyelement,anyarray)' "
+
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ bool db_used = false;
+ DbInfo *active_db = >dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+ int ntups;
+ int i_objkind,
+ i_objname;
+ /*
+ * The query below hardcodes FirstNormalObjectId as 16384 rather than
+ * interpolating that C #define into the query because, if that
+ * #define is ever changed, the cutoff we want to use is the value
+ * used by pre-version 14 servers, not that of some future version.
+ */
+
+ res = executeQueryOrDie(conn,
+/* Aggregate transition functions */
+"SELECT 'aggregate' AS objkind, p.oid::regprocedure::text AS objname "
+"FROM pg_proc AS p "
+"JOIN pg_aggregate AS a ON a.aggfnoid=p.oid "
+"JOIN pg_proc AS transfn ON transfn.oid=a.aggtransfn "
+"JOIN pg_namespace AS transnsp ON transnsp.oid=transfn.pronamespace "
+"WHERE p.oid >= 16384 "
+/* Before v11, used proisagg=true, and afterwards uses prokind='a' */
+"AND transnsp.nspname = 'pg_catalog' "
+"AND a.aggtransfn = ANY(ARRAY[" OLD_POLYMORPHICS "]::regprocedure[]) "
+// "AND aggtranstype='anyarray'::regtype
+
+/* Aggregate final functions */
+"UNION ALL "
+"SELECT 'aggregate' AS objkind, p.oid::regprocedure::text AS objname "
+"FROM pg_proc AS p "
+"JOIN pg_aggregate AS a ON a.aggfnoid=p.oid "
+"JOIN
Re: pg_upgrade (12->14) fails on aggregate
Robert Haas writes: > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote: >> To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. > Extensions can be installed into pg_catalog, but they can't get > low-numbered OIDs. Exactly. (To be clear, I had in mind writing something involving FirstNormalObjectId, not that you should put literal "16384" in the code.) regards, tom lane
Re: pg_upgrade (12->14) fails on aggregate
pá 17. 6. 2022 v 15:07 odesílatel Robert Haas napsal: > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby > wrote: > > > Also, I'd be inclined to reject system-provided objects by checking > > > for OID >= 16384 rather than hard-wiring assumptions about things > > > being in pg_catalog or not. > > > > To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. > > Extensions can be installed into pg_catalog, but they can't get > low-numbered OIDs. > yes Unfortunately, I did it in Orafce Regards Pavel > -- > Robert Haas > EDB: http://www.enterprisedb.com > > >
Re: pg_upgrade (12->14) fails on aggregate
On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote: > > Also, I'd be inclined to reject system-provided objects by checking > > for OID >= 16384 rather than hard-wiring assumptions about things > > being in pg_catalog or not. > > To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. Extensions can be installed into pg_catalog, but they can't get low-numbered OIDs. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_upgrade (12->14) fails on aggregate
On Wed, Jun 15, 2022 at 03:32:04PM -0400, Tom Lane wrote:
> Justin Pryzby writes:
> > But Petr has a point - pg_upgrade should aspire to catch errors in --check,
> > rather than starting and then leaving a mess behind for the user to clean up
>
> Agreed; pg_upgrade has historically tried to find problems similar to
> this. However, it's not just aggregates that are at risk. People
> might also have built user-defined plain functions, or operators,
> atop these functions. How far do we want to go in looking?
I wasn't yet able to construct a user-defined function which fails to reload.
> As for the query, I think it could be simplified quite a bit by
> relying on regprocedure literals, that is something like
Yes, thanks.
> Also, I think you need to check aggfinalfn too.
Done but maybe needs more cleanup.
> Also, I'd be inclined to reject system-provided objects by checking
> for OID >= 16384 rather than hard-wiring assumptions about things
> being in pg_catalog or not.
To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'.
This patch also resolves an issue with PQfinish()/dangling connections.
--
Justin
>From d8c3a2d4e4971b671347cc1c73c70e67049599c9 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH] WIP: pg_upgrade --check: detect old polymorphics from pre-14
These fail when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check.
CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
CREATE OPERATOR !@# (PROCEDURE = array_append, LEFTARG=anyarray, rightarg=anyelement);
See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
src/bin/pg_upgrade/check.c | 123 +
1 file changed, 123 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..bfdadc4d685 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ 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_old_polymorphics(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
check_for_user_defined_postfix_ops(_cluster);
+ /*
+ * Pre-PG 14 changed from anyarray to anycompatibelarray.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+ check_for_old_polymorphics(_cluster);
+
/*
* Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
* supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,122 @@ check_proper_datallowconn(ClusterInfo *cluster)
}
+/*
+ * check_for_old_polymorphics()
+ *
+ * Make sure nothing is using old polymorphic functions with
+ * anyarray/anyelement rather than the new anycompatible variants.
+ */
+static void
+check_for_old_polymorphics(ClusterInfo *cluster)
+{
+ PGresult *res;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for use of old polymorphic functions");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "databases_with_old_polymorphics.txt");
+
+#define OLD_POLYMORPHICS \
+ "'array_append(anyarray,anyelement)', " \
+ "'array_cat(anyarray,anyarray)', " \
+ "'array_position(anyarray,anyelement)', " \
+ "'array_position(anyarray,anyelement,integer)', " \
+ "'array_positions(anyarray,anyelement)', " \
+ "'array_prepend(anyelement,anyarray)', " \
+ "'array_remove(anyarray,anyelement)', " \
+ "'array_replace(anyarray,anyelement,anyelement)', " \
+ "'width_bucket(anyelement,anyarray)' "
+
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ bool db_used = false;
+ DbInfo *active_db = >dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+ int ntups;
+ int i_what,
+ i_objname;
+
+ res = executeQueryOrDie(conn,
+/* Aggregate transition functions */
+"SELECT 'aggregate' AS what, p.oid::regprocedure::text AS objname "
+"FROM pg_proc p "
+"JOIN pg_aggregate a ON a.aggfnoid=p.oid "
+"JOIN pg_proc transfn ON transfn.oid=a.aggtransfn "
+"JOIN pg_namespace pn ON pn.oid=p.pronamespace "
+"JOIN pg_namespace transnsp ON transnsp.oid=transfn.pronamespace "
+"WHERE pn.nspname != 'pg_catalog' "
+/* Before v11, used proisagg=true, and afterwards uses prokind='a' */
+"AND transnsp.nspname = 'pg_catalog' "
+"AND a.aggtransfn = ANY(ARRAY[" OLD_POLYMORPHICS "]::regprocedure[]) "
+// "AND aggtranstype='anyarray'::regtype
+
+/* Aggregate final functions */
Re: pg_upgrade (12->14) fails on aggregate
Justin Pryzby writes:
> But Petr has a point - pg_upgrade should aspire to catch errors in --check,
> rather than starting and then leaving a mess behind for the user to clean up
Agreed; pg_upgrade has historically tried to find problems similar to
this. However, it's not just aggregates that are at risk. People
might also have built user-defined plain functions, or operators,
atop these functions. How far do we want to go in looking?
As for the query, I think it could be simplified quite a bit by
relying on regprocedure literals, that is something like
WHERE ... a.aggtransfn IN
('array_append(anyarray,anyelement)'::regprocedure,
'array_prepend(anyelement,anyarray)'::regprocedure,
...)
Not sure if it's necessary to stick explicit "pg_catalog." schema
qualifications into this --- IIRC pg_upgrade runs with restrictive
search_path, so that this would be safe as-is.
Also, I think you need to check aggfinalfn too.
Also, I'd be inclined to reject system-provided objects by checking
for OID >= 16384 rather than hard-wiring assumptions about things
being in pg_catalog or not.
regards, tom lane
Re: pg_upgrade (12->14) fails on aggregate
On Wed, May 04, 2022 at 07:34:15AM -0700, David G. Johnston wrote:
> On Wed, May 4, 2022 at 7:29 AM Petr Vejsada wrote:
> > We solved it (in our case) dropping the aggregate before upgrade and
> > re-create in using new syntax in V14:
> >
> > but pg_upgrade shouldn't fail on this.
> >
> > I hope it can help to improve pg_upgrade process.
>
> The release notes say explicitly that one needs to drop and recreate the
> affected functions. Thus, we know about the issue and to date our best
> solution is to have the user do exactly what you did (i.e., it is not
> something pg_upgrade is going to do for you). If you have an alternative
> solution to suggest that would help.
>
> https://www.postgresql.org/docs/current/release-14.html : the first
> compatibility note
David is right that this is documented as a compatibility issue.
But Petr has a point - pg_upgrade should aspire to catch errors in --check,
rather than starting and then leaving a mess behind for the user to clean up
(remove existing dir, rerun initdb, start old cluster, having first moved the
dir back into place if you moved it out of the way as I do). This can take
extra minutes, and exacerbates any other problem one encounters.
$ ./tmp_install/usr/local/pgsql/bin/pg_upgrade -d pg95.dat -D pg15.dat -b
/usr/lib/postgresql/9.5/bin
...
Restoring global objects in the new cluster ok
Restoring database schemas in the new cluster
postgres
*failure*
Consult the last few lines of
"pg15.dat/pg_upgrade_output.d/20220610T104419.303/log/pg_upgrade_dump_12455.log"
for
the probable cause of the failure.
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 = '{}'
);
This patch catches the issue; the query needs to be reviewed.
SELECT pn.nspname, p.proname FROM pg_proc p
JOIN pg_aggregate a ON a.aggfnoid=p.oid
JOIN pg_proc q ON q.oid=a.aggtransfn
JOIN pg_namespace pn ON pn.oid=p.pronamespace
JOIN pg_namespace qn ON qn.oid=q.pronamespace
WHERE pn.nspname != 'pg_catalog' AND qn.nspname = 'pg_catalog'
AND 'anyelement'::regtype = ANY(q.proargtypes)
AND q.proname IN ('array_append', 'array_prepend', 'array_cat',
'array_position', 'array_positions', 'array_remove', 'array_replace',
'width_bucket');
>From 5349d32d91ce0160e08405387a30ec53ea434944 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 10 Jun 2022 11:17:36 -0500
Subject: [PATCH] WIP: pg_upgrade --check: detect aggregates for pre-pg14
This fails when upgrading from pre-14 (as expected), but it should fail during
pg_upgrade --check.
CREATE AGGREGATE array_accum(anyelement) (sfunc=array_append, stype=anyarray, initcond='{}');
See also:
9e38c2bb5093ceb0c04d6315ccd8975bd17add66
97f73a978fc1aca59c6ad765548ce0096d95a923
---
src/bin/pg_upgrade/check.c | 97 ++
1 file changed, 97 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ace7387edaf..4fd47e0b59a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -31,6 +31,7 @@ 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_old_aggregates(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -122,6 +123,12 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
check_for_user_defined_postfix_ops(_cluster);
+ /*
+ * Pre-PG 14 changed from anyarray to anycompatibelarray.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1300)
+ check_for_old_aggregates(_cluster);
+
/*
* Pre-PG 12 allowed tables to be declared WITH OIDS, which is not
* supported anymore. Verify there are none, iff applicable.
@@ -775,6 +782,96 @@ check_proper_datallowconn(ClusterInfo *cluster)
}
+/*
+ * check_for_old_aggregates()
+ *
+ * Make sure there are no aggregates using anyelement which need to be changed
+ * to anycompatible.
+ */
+static void
+check_for_old_aggregates(ClusterInfo *cluster)
+{
+ PGresult *res;
+ PGconn *conn = connectToServer(cluster, "template1");
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for old aggregates using anyelement");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "databases_with_old_aggregates.txt");
+
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ bool db_used = false;
+ DbInfo *active_db = >dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+ int ntups;
+ int i_nspname,
+
