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

Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Apr 15, 2021 at 10:06:24AM -0400, Tom Lane wrote:
>> 0001 fails for me :-(.  I think that requires default collation to be C.

> Oh right, adding --no-locale to the regress opts I see that create_index is
> failing, and that's not the one I was expecting.

> We could change create_index test to create c2 with a C collation, in order to
> test that we don't track dependency on unversioned locales, and add an extra
> test in collate.linux.utf8 to check that we do track a dependency on the
> default collation as this test isn't run in the --no-locale case.  The only
> case not tested would be default unversioned collation, but I'm not sure where
> to properly test that.  Maybe a short leading test in collate.linux.utf8 that
> would be run on linux in that case (when getdatabaseencoding() != 'UTF8')?  It
> would require an extra alternate file but it wouldn't cause too much
> maintenance problem as there should be only one test.

Since the proposed patch removes the dependency code's special-case
handling of the default collation, I don't feel like we need to jump
through hoops to prove that the default collation is tracked the
same as other collations.  A regression test with alternative outputs
is a significant ongoing maintenance burden, and I do not see where
we're getting a commensurate improvement in test coverage.  Especially
since, AFAICS, the two alternative outputs would essentially have to
accept both the "it works" and "it doesn't work" outcomes.

So I propose that we do 0001 below, which is my first patch plus your
suggestion about fixing up create_index.sql.  This passes check-world
for me under both C and en_US.utf8 prevailing locales.

            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/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 830fdddf24..7f8f91b92c 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2026,10 +2026,10 @@ REINDEX TABLE concur_reindex_tab; -- notice
 NOTICE:  table "concur_reindex_tab" has no indexes to reindex
 REINDEX (CONCURRENTLY) TABLE concur_reindex_tab; -- notice
 NOTICE:  table "concur_reindex_tab" has no indexes that can be reindexed concurrently
-ALTER TABLE concur_reindex_tab ADD COLUMN c2 text; -- add toast index
+ALTER TABLE concur_reindex_tab ADD COLUMN c2 text COLLATE "C"; -- add toast index
 -- Normal index with integer column
 CREATE UNIQUE INDEX concur_reindex_ind1 ON concur_reindex_tab(c1);
--- Normal index with text column
+-- Normal index with text column (with unversioned collation)
 CREATE INDEX concur_reindex_ind2 ON concur_reindex_tab(c2);
 -- UNIQUE index with expression
 CREATE UNIQUE INDEX concur_reindex_ind3 ON concur_reindex_tab(abs(c1));
@@ -2069,16 +2069,14 @@ WHERE classid = 'pg_class'::regclass AND
                    obj                    |                           objref                           | deptype
 ------------------------------------------+------------------------------------------------------------+---------
  index concur_reindex_ind1                | constraint concur_reindex_ind1 on table concur_reindex_tab | i
- index concur_reindex_ind2                | collation "default"                                        | n
  index concur_reindex_ind2                | column c2 of table concur_reindex_tab                      | a
  index concur_reindex_ind3                | column c1 of table concur_reindex_tab                      | a
  index concur_reindex_ind3                | table concur_reindex_tab                                   | a
- index concur_reindex_ind4                | collation "default"                                        | n
  index concur_reindex_ind4                | column c1 of table concur_reindex_tab                      | a
  index concur_reindex_ind4                | column c2 of table concur_reindex_tab                      | a
  materialized view concur_reindex_matview | schema public                                              | n
  table concur_reindex_tab                 | schema public                                              | n
-(10 rows)
+(8 rows)

 REINDEX INDEX CONCURRENTLY concur_reindex_ind1;
 REINDEX TABLE CONCURRENTLY concur_reindex_tab;
@@ -2098,16 +2096,14 @@ WHERE classid = 'pg_class'::regclass AND
                    obj                    |                           objref                           | deptype
 ------------------------------------------+------------------------------------------------------------+---------
  index concur_reindex_ind1                | constraint concur_reindex_ind1 on table concur_reindex_tab | i
- index concur_reindex_ind2                | collation "default"                                        | n
  index concur_reindex_ind2                | column c2 of table concur_reindex_tab                      | a
  index concur_reindex_ind3                | column c1 of table concur_reindex_tab                      | a
  index concur_reindex_ind3                | table concur_reindex_tab                                   | a
- index concur_reindex_ind4                | collation "default"                                        | n
  index concur_reindex_ind4                | column c1 of table concur_reindex_tab                      | a
  index concur_reindex_ind4                | column c2 of table concur_reindex_tab                      | a
  materialized view concur_reindex_matview | schema public                                              | n
  table concur_reindex_tab                 | schema public                                              | n
-(10 rows)
+(8 rows)

 -- Check that comments are preserved
 CREATE TABLE testcomment (i int);
@@ -2487,7 +2483,7 @@ WARNING:  cannot reindex system catalogs concurrently, skipping all
  Column |  Type   | Collation | Nullable | Default
 --------+---------+-----------+----------+---------
  c1     | integer |           | not null |
- c2     | text    |           |          |
+ c2     | text    | C         |          |
 Indexes:
     "concur_reindex_ind1" PRIMARY KEY, btree (c1)
     "concur_reindex_ind2" btree (c2)
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 8bc76f7c6f..51c9a12151 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -797,10 +797,10 @@ CREATE TABLE concur_reindex_tab (c1 int);
 -- REINDEX
 REINDEX TABLE concur_reindex_tab; -- notice
 REINDEX (CONCURRENTLY) TABLE concur_reindex_tab; -- notice
-ALTER TABLE concur_reindex_tab ADD COLUMN c2 text; -- add toast index
+ALTER TABLE concur_reindex_tab ADD COLUMN c2 text COLLATE "C"; -- add toast index
 -- Normal index with integer column
 CREATE UNIQUE INDEX concur_reindex_ind1 ON concur_reindex_tab(c1);
--- Normal index with text column
+-- Normal index with text column (with unversioned collation)
 CREATE INDEX concur_reindex_ind2 ON concur_reindex_tab(c2);
 -- UNIQUE index with expression
 CREATE UNIQUE INDEX concur_reindex_ind3 ON concur_reindex_tab(abs(c1));