Bogus collation version recording in recordMultipleDependencies - Mailing list pgsql-hackers

I noticed some broken-looking logic in recordMultipleDependencies
concerning how it records collation versions.  It was a bit harder
than I expected to demonstrate the bugs, but I eventually succeeded
with

u8=# create function foo(varchar) returns bool language sql return false;
CREATE FUNCTION
u8=# create collation mycoll from "en_US";
CREATE COLLATION
u8=# CREATE DOMAIN d4 AS character varying(3) COLLATE "aa_DJ"
    CONSTRAINT yes_or_no_check CHECK (value = 'YES' collate mycoll or foo(value));
CREATE DOMAIN
u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype, refobjversion from pg_depend where objid =
'd4'::regtype;
 objid |   obj   |        ref        | deptype | refobjversion
-------+---------+-------------------+---------+---------------
 37421 | type d4 | schema public     | n       |
 37421 | type d4 | collation "aa_DJ" | n       |
(2 rows)

u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype, refobjversion from pg_depend where refobjid =
'd4'::regtype;
 objid |            obj             |   ref   | deptype | refobjversion
-------+----------------------------+---------+---------+---------------
 37420 | type d4[]                  | type d4 | i       |
 37422 | constraint yes_or_no_check | type d4 | a       |
(2 rows)

u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype, refobjversion from pg_depend where objid = 37422; 
 objid |            obj             |               ref               | deptype | refobjversion
-------+----------------------------+---------------------------------+---------+---------------
 37422 | constraint yes_or_no_check | type d4                         | a       |
 37422 | constraint yes_or_no_check | collation mycoll                | n       | 2.28
 37422 | constraint yes_or_no_check | function foo(character varying) | n       | 2.28
 37422 | constraint yes_or_no_check | collation "default"             | n       |
(4 rows)

(This is in a glibc-based build, with C as the database's default
collation.)

One question here is whether it's correct that the domain's dependency
on collation "aa_DJ" is unversioned.  Maybe that's intentional, but it
seems worth asking.

Anyway, there are two pretty obvious bugs in the dependencies for the
domain's CHECK constraint: the version for collation mycoll leaks
into the entry for function foo, and an entirely useless (because
unversioned) dependency is recorded on the default collation.

... well, it's almost entirely useless.  If we fix things to not do that
(as per patch 0001 below), the results of the create_index regression
test become unstable, because there's two queries that inquire into the
dependencies of indexes, and their results change depending on whether
the default collation has a version or not.  I'd be inclined to just
take out the portions of that test that depend on that question, but
maybe somebody will complain that there's a loss of useful coverage.
I don't agree, but maybe I'll be overruled.

If we do feel we need to stay bug-compatible with that behavior, then
the alternate 0002 patch just fixes the version-leakage-across-entries
problem, while still removing the unnecessary assumption that C, POSIX,
and DEFAULT are the only pinned collations.

(To be clear: 0002 passes check-world as-is, while 0001 is not
committable without some regression-test fiddling.)

Thoughts?

            regards, tom lane

diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 362db7fe91..1217c01b8a 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -73,7 +73,6 @@ recordMultipleDependencies(const ObjectAddress *depender,
                 max_slots,
                 slot_init_count,
                 slot_stored_count;
-    char       *version = NULL;

     if (nreferenced <= 0)
         return;                    /* nothing to do */
@@ -104,31 +103,22 @@ recordMultipleDependencies(const ObjectAddress *depender,
     slot_init_count = 0;
     for (i = 0; i < nreferenced; i++, referenced++)
     {
-        bool        ignore_systempin = false;
+        char       *version = NULL;

         if (record_version)
         {
             /* For now we only know how to deal with collations. */
             if (referenced->classId == CollationRelationId)
             {
-                /* C and POSIX don't need version tracking. */
+                /* These are unversioned, so don't waste cycles on them. */
                 if (referenced->objectId == C_COLLATION_OID ||
                     referenced->objectId == POSIX_COLLATION_OID)
                     continue;

                 version = get_collation_version_for_oid(referenced->objectId,
                                                         false);
-
-                /*
-                 * Default collation is pinned, so we need to force recording
-                 * the dependency to store the version.
-                 */
-                if (referenced->objectId == DEFAULT_COLLATION_OID)
-                    ignore_systempin = true;
             }
         }
-        else
-            Assert(!version);

         /*
          * If the referenced object is pinned by the system, there's no real
@@ -136,7 +126,7 @@ recordMultipleDependencies(const ObjectAddress *depender,
          * version.  This saves lots of space in pg_depend, so it's worth the
          * time taken to check.
          */
-        if (!ignore_systempin && isObjectPinned(referenced, dependDesc))
+        if (version == NULL && isObjectPinned(referenced, dependDesc))
             continue;

         if (slot_init_count < max_slots)
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 362db7fe91..174ce4b7df 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -73,7 +73,6 @@ recordMultipleDependencies(const ObjectAddress *depender,
                 max_slots,
                 slot_init_count,
                 slot_stored_count;
-    char       *version = NULL;

     if (nreferenced <= 0)
         return;                    /* nothing to do */
@@ -104,6 +103,7 @@ recordMultipleDependencies(const ObjectAddress *depender,
     slot_init_count = 0;
     for (i = 0; i < nreferenced; i++, referenced++)
     {
+        char       *version = NULL;
         bool        ignore_systempin = false;

         if (record_version)
@@ -111,7 +111,7 @@ recordMultipleDependencies(const ObjectAddress *depender,
             /* For now we only know how to deal with collations. */
             if (referenced->classId == CollationRelationId)
             {
-                /* C and POSIX don't need version tracking. */
+                /* These are unversioned, so don't waste cycles on them. */
                 if (referenced->objectId == C_COLLATION_OID ||
                     referenced->objectId == POSIX_COLLATION_OID)
                     continue;
@@ -120,15 +120,16 @@ recordMultipleDependencies(const ObjectAddress *depender,
                                                         false);

                 /*
-                 * Default collation is pinned, so we need to force recording
-                 * the dependency to store the version.
+                 * If we have a version, make sure we record it even if the
+                 * collation is pinned.  Also force recording a dependency on
+                 * the "default" collation even if it hasn't got a version;
+                 * this is an ugly kluge to ensure stability of regression
+                 * test results.
                  */
-                if (referenced->objectId == DEFAULT_COLLATION_OID)
+                if (version || referenced->objectId == DEFAULT_COLLATION_OID)
                     ignore_systempin = true;
             }
         }
-        else
-            Assert(!version);

         /*
          * If the referenced object is pinned by the system, there's no real