Thread: PG upgrade 14->15 fails - database contains our own extension

PG upgrade 14->15 fails - database contains our own extension

From
David Turoň
Date:

Hi community,

I have problem with pg_upgrade. Tested from 14.5 to 15.0 rc2 when database contains our extension with one new type. Using pg_dump & restore works well.

We made workaround extension for some usage in javascript library that contains new type that represents bigint as text. So something like auto conversion from SELECT (2^32)::text -> bigint when data is stored and (2^32) -> text when data is retrieved.

Im not sure if this is postgresql bug or we have something wrong in our extension with cast. So becouse this im writing there.

Here is output of pg_upgrade:
(our extension name is lbuid, our type public.lbuid)

command: "/usr/pgsql-15/bin/pg_dump" --host /tmp/pg_upgrade_log --port 50432 --username postgres --schema-only --quote-all-identifiers --binary-upgrade --format=custom  --file="/home/pgsql/data_new/pg_upgrade_output.d/20221013T104924.054/
dump/pg_upgrade_dump_16385.custom" 'dbname=lbstat' >> "/home/pgsql/data_new/pg_upgrade_output.d/20221013T104924.054/log/pg_upgrade_dump_16385.log" 2>&1


command: "/usr/pgsql-15/bin/pg_restore" --host /tmp/pg_upgrade_log --port 50432 --username postgres --create --exit-on-error --verbose --dbname template1 "/home/pgsql/data_new/pg_upgrade_output.d/20221013T104924.054/dump/pg_upgrade_dump_1
6385.custom" >> "/home/pgsql/data_new/pg_upgrade_output.d/20221013T104924.054/log/pg_upgrade_dump_16385.log" 2>&1
pg_restore: connecting to database for restore
pg_restore: creating DATABASE "lbstat"
pg_restore: connecting to new database "lbstat"
pg_restore: creating DATABASE PROPERTIES "lbstat"
pg_restore: connecting to new database "lbstat"
pg_restore: creating pg_largeobject "pg_largeobject"
pg_restore: creating SCHEMA "public"
pg_restore: creating COMMENT "SCHEMA "public""
pg_restore: creating EXTENSION "lbuid"
pg_restore: creating COMMENT "EXTENSION "lbuid""
pg_restore: creating SHELL TYPE "public.lbuid"
pg_restore: creating FUNCTION "public.lbuid_in("cstring")"
pg_restore: creating FUNCTION "public.lbuid_out("public"."lbuid")"
pg_restore: creating TYPE "public.lbuid"
pg_restore: creating CAST "CAST (integer AS "public"."lbuid")"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3751; 2605 16393 CAST CAST (integer AS "public"."lbuid") (no owner)
pg_restore: error: could not execute query: ERROR:  return data type of cast function must match or be binary-coercible to target data type
Command was: CREATE CAST (integer AS "public"."lbuid") WITH FUNCTION "pg_catalog"."int8"(integer) AS IMPLICIT;

-- For binary upgrade, handle extension membership the hard way
ALTER EXTENSION "lbuid" ADD CAST (integer AS "public"."lbuid");


Hint is good, but this cast is already member of our extension:

lbstat=# ALTER EXTENSION lbuid ADD CAST (integer AS public.lbuid);                                                                                                                                                                      
ERROR:  cast from integer to lbuid is already a member of extension "lbuid"


Database contains this:

CREATE EXTENSION lbuid ;
CREATE TABLE test(id lbuild);
INSERT INTO test VALUES ('1344456644646645456');

Tested on our distribution based on centos7.

When i drop this cast from extension manualy a then manualy restore after pg_upgrade, then operation is without failure.

In attachment are extension files.

Thanks.

Best regards. David T.
(See attached file: lbuid.control)(See attached file: lbuid--0.1.0.sql)

--
-------------------------------------
Ing. David TUROŇ
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:    +420 591 166 224
fax:    +420 596 621 273
mobil:  +420 732 589 152
www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------

Attachment

Re: PG upgrade 14->15 fails - database contains our own extension

From
Robert Haas
Date:
On Thu, Oct 13, 2022 at 9:57 AM David Turoň <david.turon@linuxbox.cz> wrote:
> pg_restore: creating TYPE "public.lbuid"
> pg_restore: creating CAST "CAST (integer AS "public"."lbuid")"
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 3751; 2605 16393 CAST CAST (integer AS "public"."lbuid") (no owner)
> pg_restore: error: could not execute query: ERROR:  return data type of cast function must match or be
binary-coercibleto target data type 
> Command was: CREATE CAST (integer AS "public"."lbuid") WITH FUNCTION "pg_catalog"."int8"(integer) AS IMPLICIT;

I think the error is complaining that the return type of
int8(integer), which is bigint, needs to coercible WITHOUT FUNCTION to
lbuid. Your extension contains such a cast, but at the point when the
error occurs, it hasn't been restored yet. That suggests that either
the cast didn't get included in the dump file, or it got included in
the wrong order. A quick test suggest the latter. If I execute your
SQL file and the dump the database, I get:

CREATE CAST (integer AS public.lbuid) WITH FUNCTION
pg_catalog.int8(integer) AS IMPLICIT;
CREATE CAST (bigint AS public.lbuid) WITHOUT FUNCTION AS IMPLICIT;
CREATE CAST (public.lbuid AS bigint) WITHOUT FUNCTION AS IMPLICIT;

That's not a valid dump ordering, and if I drop the casts and try to
recreate them that way, it fails in the same way you saw.

My guess is that this is a bug in Tom's commit
b55f2b6926556115155930c4b2d006c173f45e65, "Adjust pg_dump's priority
ordering for casts."

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: PG upgrade 14->15 fails - database contains our own extension

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> CREATE CAST (integer AS public.lbuid) WITH FUNCTION
> pg_catalog.int8(integer) AS IMPLICIT;
> CREATE CAST (bigint AS public.lbuid) WITHOUT FUNCTION AS IMPLICIT;
> CREATE CAST (public.lbuid AS bigint) WITHOUT FUNCTION AS IMPLICIT;

> That's not a valid dump ordering, and if I drop the casts and try to
> recreate them that way, it fails in the same way you saw.

> My guess is that this is a bug in Tom's commit
> b55f2b6926556115155930c4b2d006c173f45e65, "Adjust pg_dump's priority
> ordering for casts."

Hmm ... I think it's a very ancient bug that somehow David has avoided
tripping over up to now.  Namely, that we require the bigint->lbuid
implicit cast to exist in order to make that WITH FUNCTION cast, but
we fail to record it as a dependency during CastCreate.  So pg_dump
is flying blind as to the required restoration order, and if it ever
worked, you were just lucky.

We might be able to put in some kluge in pg_dump to make it less
likely to fail with existing DBs, but I think the true fix lies
in adding that dependency.

(I'm pretty skeptical about it being a good idea to have a set of
casts like this, but I don't suppose pg_dump is chartered to
editorialize on that.)

            regards, tom lane



Re: PG upgrade 14->15 fails - database contains our own extension

From
Tom Lane
Date:
I wrote:
> Hmm ... I think it's a very ancient bug that somehow David has avoided
> tripping over up to now.

Looking closer, I don't see how b55f2b692 could have changed pg_dump's
opinion of the order to sort these three casts in; that sort ordering
logic is old enough to vote.  So I'm guessing that in fact this *never*
worked.  Perhaps this extension has never been through pg_upgrade before,
or at least not with these casts?

> We might be able to put in some kluge in pg_dump to make it less
> likely to fail with existing DBs, but I think the true fix lies
> in adding that dependency.

I don't see any painless way to fix this in pg_dump, and I'm inclined
not to bother trying if it's not a regression.  Better to spend the
effort on the backend-side fix.

On the backend side, really anyplace that we consult IsBinaryCoercible
during DDL is at hazard.  While there aren't a huge number of such
places, there's certainly more than just CreateCast.  I'm trying to
decide how much trouble it's worth going to there.  I could be wrong,
but I think that only the cast-vs-cast case is really likely to be
problematic for pg_dump, given that it dumps casts pretty early now.
So it might be sufficient to fix that one case.

            regards, tom lane



Re: PG upgrade 14->15 fails - database contains our own extension

From
Tom Lane
Date:
I wrote:
> We might be able to put in some kluge in pg_dump to make it less
> likely to fail with existing DBs, but I think the true fix lies
> in adding that dependency.

Here's a draft patch for that.  I'm unsure whether it's worth
back-patching; even if we did, we couldn't guarantee that existing
databases would have the additional pg_depend entries.

If we do only put it in HEAD, maybe we should break compatibility
to the extent of changing IsBinaryCoercible's API rather than
inventing a separate call.  I'm still not excited about recording
additional dependencies elsewhere, but that path would leave us
with cleaner code if we eventually do that.

            regards, tom lane

diff --git a/src/backend/catalog/pg_cast.c b/src/backend/catalog/pg_cast.c
index 1812bb7fcc..b50a56954d 100644
--- a/src/backend/catalog/pg_cast.c
+++ b/src/backend/catalog/pg_cast.c
@@ -35,13 +35,20 @@
  * Caller must have already checked privileges, and done consistency
  * checks on the given datatypes and cast function (if applicable).
  *
+ * Since we allow binary coercibility of the datatypes to the cast
+ * function's input and result, there could be one or two WITHOUT FUNCTION
+ * casts that this one depends on.  We don't record that explicitly
+ * in pg_cast, but we still need to make dependencies on those casts.
+ *
  * 'behavior' indicates the types of the dependencies that the new
- * cast will have on its input and output types and the cast function.
+ * cast will have on its input and output types, the cast function,
+ * and the other casts if any.
  * ----------------------------------------------------------------
  */
 ObjectAddress
-CastCreate(Oid sourcetypeid, Oid targettypeid, Oid funcid, char castcontext,
-           char castmethod, DependencyType behavior)
+CastCreate(Oid sourcetypeid, Oid targettypeid,
+           Oid funcid, Oid incastid, Oid outcastid,
+           char castcontext, char castmethod, DependencyType behavior)
 {
     Relation    relation;
     HeapTuple    tuple;
@@ -102,6 +109,18 @@ CastCreate(Oid sourcetypeid, Oid targettypeid, Oid funcid, char castcontext,
         add_exact_object_address(&referenced, addrs);
     }

+    /* dependencies on casts required for function */
+    if (OidIsValid(incastid))
+    {
+        ObjectAddressSet(referenced, CastRelationId, incastid);
+        add_exact_object_address(&referenced, addrs);
+    }
+    if (OidIsValid(outcastid))
+    {
+        ObjectAddressSet(referenced, CastRelationId, outcastid);
+        add_exact_object_address(&referenced, addrs);
+    }
+
     record_object_address_dependencies(&myself, addrs, behavior);
     free_object_addresses(addrs);

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index e6fcfc23b9..1f820c93e9 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -1526,6 +1526,8 @@ CreateCast(CreateCastStmt *stmt)
     char        sourcetyptype;
     char        targettyptype;
     Oid            funcid;
+    Oid            incastid = InvalidOid;
+    Oid            outcastid = InvalidOid;
     int            nargs;
     char        castcontext;
     char        castmethod;
@@ -1603,7 +1605,9 @@ CreateCast(CreateCastStmt *stmt)
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                      errmsg("cast function must take one to three arguments")));
-        if (!IsBinaryCoercible(sourcetypeid, procstruct->proargtypes.values[0]))
+        if (!IsBinaryCoercibleWithCast(sourcetypeid,
+                                       procstruct->proargtypes.values[0],
+                                       &incastid))
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                      errmsg("argument of cast function must match or be binary-coercible from source data type")));
@@ -1617,7 +1621,9 @@ CreateCast(CreateCastStmt *stmt)
                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                      errmsg("third argument of cast function must be type %s",
                             "boolean")));
-        if (!IsBinaryCoercible(procstruct->prorettype, targettypeid))
+        if (!IsBinaryCoercibleWithCast(procstruct->prorettype,
+                                       targettypeid,
+                                       &outcastid))
             ereport(ERROR,
                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                      errmsg("return data type of cast function must match or be binary-coercible to target data
type")));
@@ -1756,8 +1762,8 @@ CreateCast(CreateCastStmt *stmt)
             break;
     }

-    myself = CastCreate(sourcetypeid, targettypeid, funcid, castcontext,
-                        castmethod, DEPENDENCY_NORMAL);
+    myself = CastCreate(sourcetypeid, targettypeid, funcid, incastid, outcastid,
+                        castcontext, castmethod, DEPENDENCY_NORMAL);
     return myself;
 }

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 33b64fd279..b7c3dded17 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1705,7 +1705,9 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
                                &castFuncOid);

     /* Create cast from the range type to its multirange type */
-    CastCreate(typoid, multirangeOid, castFuncOid, 'e', 'f', DEPENDENCY_INTERNAL);
+    CastCreate(typoid, multirangeOid, castFuncOid, InvalidOid, InvalidOid,
+               COERCION_CODE_EXPLICIT, COERCION_METHOD_FUNCTION,
+               DEPENDENCY_INTERNAL);

     pfree(multirangeArrayName);

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index c4e958e4aa..60908111c8 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -2993,11 +2993,29 @@ IsPreferredType(TYPCATEGORY category, Oid type)
  */
 bool
 IsBinaryCoercible(Oid srctype, Oid targettype)
+{
+    Oid            castoid;
+
+    return IsBinaryCoercibleWithCast(srctype, targettype, &castoid);
+}
+
+/* IsBinaryCoercibleWithCast()
+ *        Check if srctype is binary-coercible to targettype.
+ *
+ * This variant also returns the OID of the pg_cast entry if one is involved.
+ * *castoid is set to InvalidOid if no binary-coercible cast exists, or if
+ * there is a hard-wired rule for it rather than a pg_cast entry.
+ */
+bool
+IsBinaryCoercibleWithCast(Oid srctype, Oid targettype,
+                          Oid *castoid)
 {
     HeapTuple    tuple;
     Form_pg_cast castForm;
     bool        result;

+    *castoid = InvalidOid;
+
     /* Fast path if same type */
     if (srctype == targettype)
         return true;
@@ -3061,6 +3079,9 @@ IsBinaryCoercible(Oid srctype, Oid targettype)
     result = (castForm->castmethod == COERCION_METHOD_BINARY &&
               castForm->castcontext == COERCION_CODE_IMPLICIT);

+    if (result)
+        *castoid = castForm->oid;
+
     ReleaseSysCache(tuple);

     return result;
diff --git a/src/include/catalog/pg_cast.h b/src/include/catalog/pg_cast.h
index 3c15df0053..67b08a4663 100644
--- a/src/include/catalog/pg_cast.h
+++ b/src/include/catalog/pg_cast.h
@@ -95,6 +95,8 @@ typedef enum CoercionMethod
 extern ObjectAddress CastCreate(Oid sourcetypeid,
                                 Oid targettypeid,
                                 Oid funcid,
+                                Oid incastid,
+                                Oid outcastid,
                                 char castcontext,
                                 char castmethod,
                                 DependencyType behavior);
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index b105c7da90..ddbc995077 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -32,6 +32,8 @@ typedef enum CoercionPathType


 extern bool IsBinaryCoercible(Oid srctype, Oid targettype);
+extern bool IsBinaryCoercibleWithCast(Oid srctype, Oid targettype,
+                                      Oid *castoid);
 extern bool IsPreferredType(TYPCATEGORY category, Oid type);
 extern TYPCATEGORY TypeCategory(Oid type);

diff --git a/src/test/regress/expected/create_cast.out b/src/test/regress/expected/create_cast.out
index 88f94a63b4..9a56fe3f0f 100644
--- a/src/test/regress/expected/create_cast.out
+++ b/src/test/regress/expected/create_cast.out
@@ -72,3 +72,32 @@ SELECT 1234::int4::casttesttype; -- Should work now
  foo1234
 (1 row)

+DROP FUNCTION int4_casttesttype(int4) CASCADE;
+NOTICE:  drop cascades to cast from integer to casttesttype
+-- Try it with a function that requires an implicit cast
+CREATE FUNCTION bar_int4_text(int4) RETURNS text LANGUAGE SQL AS
+$$ SELECT ('bar'::text || $1::text); $$;
+CREATE CAST (int4 AS casttesttype) WITH FUNCTION bar_int4_text(int4) AS IMPLICIT;
+SELECT 1234::int4::casttesttype; -- Should work now
+ casttesttype
+--------------
+ bar1234
+(1 row)
+
+-- check dependencies generated for that
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+       pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+       deptype
+FROM pg_depend
+WHERE classid = 'pg_cast'::regclass AND
+      objid = (SELECT oid FROM pg_cast
+               WHERE castsource = 'int4'::regtype
+                 AND casttarget = 'casttesttype'::regtype)
+ORDER BY refclassid;
+                obj                |             objref              | deptype
+-----------------------------------+---------------------------------+---------
+ cast from integer to casttesttype | type casttesttype               | n
+ cast from integer to casttesttype | function bar_int4_text(integer) | n
+ cast from integer to casttesttype | cast from text to casttesttype  | n
+(3 rows)
+
diff --git a/src/test/regress/sql/create_cast.sql b/src/test/regress/sql/create_cast.sql
index b11cf88b06..32187853cc 100644
--- a/src/test/regress/sql/create_cast.sql
+++ b/src/test/regress/sql/create_cast.sql
@@ -52,3 +52,24 @@ $$ SELECT ('foo'::text || $1::text)::casttesttype; $$;

 CREATE CAST (int4 AS casttesttype) WITH FUNCTION int4_casttesttype(int4) AS IMPLICIT;
 SELECT 1234::int4::casttesttype; -- Should work now
+
+DROP FUNCTION int4_casttesttype(int4) CASCADE;
+
+-- Try it with a function that requires an implicit cast
+
+CREATE FUNCTION bar_int4_text(int4) RETURNS text LANGUAGE SQL AS
+$$ SELECT ('bar'::text || $1::text); $$;
+
+CREATE CAST (int4 AS casttesttype) WITH FUNCTION bar_int4_text(int4) AS IMPLICIT;
+SELECT 1234::int4::casttesttype; -- Should work now
+
+-- check dependencies generated for that
+SELECT pg_describe_object(classid, objid, objsubid) as obj,
+       pg_describe_object(refclassid, refobjid, refobjsubid) as objref,
+       deptype
+FROM pg_depend
+WHERE classid = 'pg_cast'::regclass AND
+      objid = (SELECT oid FROM pg_cast
+               WHERE castsource = 'int4'::regtype
+                 AND casttarget = 'casttesttype'::regtype)
+ORDER BY refclassid;
diff --git a/src/tools/valgrind.supp b/src/tools/valgrind.supp
index 4e8c482757..7ea464c809 100644
--- a/src/tools/valgrind.supp
+++ b/src/tools/valgrind.supp
@@ -113,7 +113,7 @@
     overread_tuplestruct_pg_cast
     Memcheck:Addr4

-    fun:IsBinaryCoercible
+    fun:IsBinaryCoercibleWithCast
 }

 # Python's allocator does some low-level tricks for efficiency. Those

Re: PG upgrade 14->15 fails - database contains our own extension

From
David Turoň
Date:

Hi,

I really appreciate your help and very quick response. And WOW, write patch for this in few hours ...that's amazing!

> Looking closer, I don't see how b55f2b692 could have changed pg_dump's
> opinion of the order to sort these three casts in; that sort ordering
> logic is old enough to vote.  So I'm guessing that in fact this *never*
> worked.  Perhaps this extension has never been through pg_upgrade before,
> or at least not with these casts?


Yes its new and I tested right now with upgrade from 9.6 to 15.0 rc2 with same result. So this behavior is probably long time there, but extension is new and not upgraded yet. And probably nobody have this "strange" idea.


>(I'm pretty skeptical about it being a good idea to have a set of
casts like this, but I don't suppose pg_dump is chartered to
editorialize on that.)
Yes, im not proud of the creation this workaround extension and I did what frontend develepers asked me if it's possible. I don't expect a medal of honor:)

The problem was when bigint was taken from DB as json and stored as number JS library cast number automaticaly to integer that cause problem.

lbstat=# SELECT json_agg(test) FROM test;
       json_agg        
-----------------------
 [{"id":"4294967296"}]
(1 row)

-- ID was represnted now as text and JS library can use it and sent back without error. But for DB is still bigint.

This was automatic way to solve this problem without casting on all places to text. I tested and most things works well until upgrade test didn't pass.

Thank you all.

David T.

--
-------------------------------------
Ing. David TUROŇ
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:    +420 591 166 224
fax:    +420 596 621 273
mobil:  +420 732 589 152
www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis@linuxbox.cz
-------------------------------------


Inactive hide details for "Tom Lane" ---13.10.2022 18:06:52---I wrote: > Hmm ... I think it's a very ancient bug that somehow David has avoided

Od: "Tom Lane" <tgl@sss.pgh.pa.us>
Komu: "David Turoň" <david.turon@linuxbox.cz>
Kopie: "Robert Haas" <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org, "Marian Krucina" <marian.krucina@linuxbox.cz>
Datum: 13.10.2022 18:06
Předmět: Re: PG upgrade 14->15 fails - database contains our own extension





I wrote:
> Hmm ... I think it's a very ancient bug that somehow David has avoided
> tripping over up to now.

Looking closer, I don't see how b55f2b692 could have changed pg_dump's
opinion of the order to sort these three casts in; that sort ordering
logic is old enough to vote.  So I'm guessing that in fact this *never*
worked.  Perhaps this extension has never been through pg_upgrade before,
or at least not with these casts?

> We might be able to put in some kluge in pg_dump to make it less
> likely to fail with existing DBs, but I think the true fix lies
> in adding that dependency.

I don't see any painless way to fix this in pg_dump, and I'm inclined
not to bother trying if it's not a regression.  Better to spend the
effort on the backend-side fix.

On the backend side, really anyplace that we consult IsBinaryCoercible
during DDL is at hazard.  While there aren't a huge number of such
places, there's certainly more than just CreateCast.  I'm trying to
decide how much trouble it's worth going to there.  I could be wrong,
but I think that only the cast-vs-cast case is really likely to be
problematic for pg_dump, given that it dumps casts pretty early now.
So it might be sufficient to fix that one case.

regards, tom lane


Attachment