From ef387bed87f2787b1012e68e9a33607a1074c123 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 16 Apr 2021 12:26:50 -0400 Subject: [PATCH] Fix bogus collation-version-recording logic. recordMultipleDependencies had the wrong scope for its "version" variable, allowing a version label to leak from the collation entry it was meant for to subsequent non-collation entries. This is relatively hard to trigger because of the OID-descending order that the inputs will normally arrive in: subsequent non-collation items will tend to be pinned. But it can be exhibited easily with a custom collation. Also, don't special-case the default collation, but instead ignore pinned-ness of a collation when we've found a version for it. This avoids creating useless pg_depend entries, and removes a not-very- future-proof assumption that C, POSIX, and DEFAULT are the only pinned collations. A small problem is that, because the default collation may or may not have a version, the regression tests can't assume anything about whether dependency entries will be made for it. This seems OK though since it's now handled just the same as other collations, and we have test cases for both versioned and unversioned collations. Fixes oversights in commit 257836a75. Thanks to Julien Rouhaud for review. Discussion: https://postgr.es/m/3564817.1618420687@sss.pgh.pa.us --- src/backend/catalog/pg_depend.c | 16 +++------------- src/test/regress/expected/create_index.out | 14 +++++--------- src/test/regress/sql/create_index.sql | 4 ++-- 3 files changed, 10 insertions(+), 24 deletions(-) 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));