Обсуждение: 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 <pve@paymorrow.com> 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');
 

Justin Pryzby <pryzby@telsasoft.com> 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

On Wed, Jun 15, 2022 at 03:32:04PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> 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

On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby <pryzby@telsasoft.com> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby <pryzby@telsasoft.com> 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

On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby <pryzby@telsasoft.com> 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.

> On 23 Jun 2022, at 04:58, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby <pryzby@telsasoft.com> 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")
acrossthe file. 

Also, we can return slightly more information about found objects. For example, operator will look like "operator: ||".
Atleast 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
joinwith pg_aggregate anyway. 

Thanks!

Best regards, Andrey Borodin.

On Fri, Jun 24, 2022 at 11:43:18PM +0500, Andrey Borodin wrote:
> > On 23 Jun 2022, at 04:58, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 
> > On Fri, Jun 17, 2022 at 10:14:13AM -0400, Tom Lane wrote:
> >> Robert Haas <robertmhaas@gmail.com> writes:
> >>> On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby <pryzby@telsasoft.com> 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")
acrossthe 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.
Wejoin 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.

> On 25 Jun 2022, at 01:28, Justin Pryzby <pryzby@telsasoft.com> 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
essenceof the problem. 

Also, let's check that oid of used functions belong to system catalog (<16384)? We don't care about user-defined
functionswith the same name. 

And, probably, we can do this unconditionally:
if (old_cluster.major_version >= 9500)
        appendPQExpBufferStr(&old_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.

On Sat, Jun 25, 2022 at 03:34:49PM +0500, Andrey Borodin wrote:
> > On 25 Jun 2022, at 01:28, Justin Pryzby <pryzby@telsasoft.com> 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
essenceof the problem.
 

> Also, let's check that oid of used functions belong to system catalog (<16384)? We don't care about user-defined
functionswith 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(&old_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

> On 28 Jun 2022, at 04:30, Justin Pryzby <pryzby@telsasoft.com> 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,
butI'm not sure if we do this routinely. 

Besides this the patch seems to be RfC.

Best regards, Andrey Borodin.

On Wed, Jun 29, 2022 at 10:58:44PM +0500, Andrey Borodin wrote:
> > On 28 Jun 2022, at 04:30, Justin Pryzby <pryzby@telsasoft.com> 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
betareleases.
 

See: e469f0aaf3c586c8390bd65923f97d4b1683cd9f

-- 
Justin

> On 29 Jun 2022, at 23:07, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Jun 29, 2022 at 10:58:44PM +0500, Andrey Borodin wrote:
>>> On 28 Jun 2022, at 04:30, Justin Pryzby <pryzby@telsasoft.com> 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
betareleases. 
This makes sense, thank you for clarification.

The patch is marked WiP, what is in progress as of now?

Best regards, Andrey Borodin.

Andrey Borodin <x4mmm@yandex-team.ru> 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

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&dt=2022-07-05%2020%3A48%3A21
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=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

Michael Paquier <michael@paquier.xyz> 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

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