Opclass parameters of indexes lost after REINDEX CONCURRENTLY

Started by Michael Paquierabout 4 years ago5 messages

michael@paquier.xyz

about 4 years ago

1 attachment(s)

Hi all,

While reviewing the code for opclass parameters with indexes, I have
noticed that opclass parameters are lost after a concurrent reindex.
As we use a IndexInfo to hold the information of the new index when
creating a copy of the old one, it is just a matter of making sure
that ii_OpclassOptions is filled appropriately, but that was missed by
911e702.

Attached is a patch to fix the issue. After a concurrent reindex, we
would not finish with a corrupted index, just with one rebuilt with
default opclass parameter values.

Any objections or comments?
--
Michael

Attachments:

reindex-conc-opclass.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 26bfa74ce7..460f84d4e8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1365,6 +1365,15 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
 		newInfo->ii_IndexAttrNumbers[i] = oldInfo->ii_IndexAttrNumbers[i];
 	}
 
+	/* Extract opclass parameters for each attribute, if any */
+	if (oldInfo->ii_OpclassOptions)
+	{
+		newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
+											 newInfo->ii_NumIndexAttrs);
+		for (int i = 0; i < oldInfo->ii_NumIndexAttrs; i++)
+			newInfo->ii_OpclassOptions[i] = oldInfo->ii_OpclassOptions[i];
+	}
+
 	/*
 	 * Now create the new index.
 	 *
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 4750eac359..4702333fd6 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2176,6 +2176,24 @@ SELECT indexrelid::regclass, indisreplident FROM pg_index
 (1 row)
 
 DROP TABLE concur_replident;
+-- Check that opclass parameters are preserved
+CREATE TABLE concur_appclass_tab(i tsvector, j tsvector);
+CREATE INDEX concur_appclass_ind on concur_appclass_tab
+  USING gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'));
+CREATE INDEX concur_appclass_ind_2 on concur_appclass_tab
+  USING gist (i tsvector_ops, j tsvector_ops (siglen='300'));
+REINDEX TABLE CONCURRENTLY concur_appclass_tab;
+\d concur_appclass_tab
+         Table "public.concur_appclass_tab"
+ Column |   Type   | Collation | Nullable | Default 
+--------+----------+-----------+----------+---------
+ i      | tsvector |           |          | 
+ j      | tsvector |           |          | 
+Indexes:
+    "concur_appclass_ind" gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'))
+    "concur_appclass_ind_2" gist (i, j tsvector_ops (siglen='300'))
+
+DROP TABLE concur_appclass_tab;
 -- Partitions
 -- Create some partitioned tables
 CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 22209b0691..c26f6ff70b 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -888,6 +888,15 @@ REINDEX TABLE CONCURRENTLY concur_replident;
 SELECT indexrelid::regclass, indisreplident FROM pg_index
   WHERE indrelid = 'concur_replident'::regclass;
 DROP TABLE concur_replident;
+-- Check that opclass parameters are preserved
+CREATE TABLE concur_appclass_tab(i tsvector, j tsvector);
+CREATE INDEX concur_appclass_ind on concur_appclass_tab
+  USING gist (i tsvector_ops (siglen='1000'), j tsvector_ops (siglen='500'));
+CREATE INDEX concur_appclass_ind_2 on concur_appclass_tab
+  USING gist (i tsvector_ops, j tsvector_ops (siglen='300'));
+REINDEX TABLE CONCURRENTLY concur_appclass_tab;
+\d concur_appclass_tab
+DROP TABLE concur_appclass_tab;
 
 -- Partitions
 -- Create some partitioned tables

Zhihong Yu

zyu@yugabyte.com

about 4 years ago

In reply to: Michael Paquier (#1)

Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier <michael@paquier.xyz> wrote:

Hi all,

While reviewing the code for opclass parameters with indexes, I have
noticed that opclass parameters are lost after a concurrent reindex.
As we use a IndexInfo to hold the information of the new index when
creating a copy of the old one, it is just a matter of making sure
that ii_OpclassOptions is filled appropriately, but that was missed by
911e702.

Attached is a patch to fix the issue. After a concurrent reindex, we
would not finish with a corrupted index, just with one rebuilt with
default opclass parameter values.

Any objections or comments?
--
Michael

Hi,

+       newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
+                                            newInfo->ii_NumIndexAttrs);

It seems we may not need to allocate these many entries (as shown by the
concur_appclass_ind_2 example in the test).
In the previous loop (starting line 1359), we can increment a counter which
would finally tell us how many oldInfo->ii_OpclassOptions[i] is not NULL.

Then that many entries can be allocated in the above code.

Cheers

Zhihong Yu

zyu@yugabyte.com

about 4 years ago

In reply to: Zhihong Yu (#2)

Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

On Sat, Oct 30, 2021 at 3:59 AM Zhihong Yu <zyu@yugabyte.com> wrote:

On Sat, Oct 30, 2021 at 1:28 AM Michael Paquier <michael@paquier.xyz>
wrote:

Hi all,

While reviewing the code for opclass parameters with indexes, I have
noticed that opclass parameters are lost after a concurrent reindex.
As we use a IndexInfo to hold the information of the new index when
creating a copy of the old one, it is just a matter of making sure
that ii_OpclassOptions is filled appropriately, but that was missed by
911e702.

Attached is a patch to fix the issue. After a concurrent reindex, we
would not finish with a corrupted index, just with one rebuilt with
default opclass parameter values.

Any objections or comments?
--
Michael

Hi,
+       newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
+                                            newInfo->ii_NumIndexAttrs);
It seems we may not need to allocate these many entries (as shown by the
concur_appclass_ind_2 example in the test).
In the previous loop (starting line 1359), we can increment a counter
which would finally tell us how many oldInfo->ii_OpclassOptions[i] is not
NULL.

Then that many entries can be allocated in the above code.

Cheers

Hi,
Upon further look, my previous comment was premature. Please ignore that.

+ newInfo->ii_OpclassOptions[i] = oldInfo->ii_OpclassOptions[i];

Should datumCopy() be used inside the loop ? I saw the following
in get_attoptions(Oid relid, int16 attnum):

result = datumCopy(attopts, false, -1); /* text[] */

Cheers

Michael Paquier

michael@paquier.xyz

about 4 years ago

In reply to: Zhihong Yu (#3)

Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY

On Sat, Oct 30, 2021 at 04:11:06AM -0700, Zhihong Yu wrote:

Should datumCopy() be used inside the loop ? I saw the following
in get_attoptions(Oid relid, int16 attnum):

Yeah, you are right that it would be better here to use
get_attoptions() to grab a copy of each attribute's option directly
from the catalogs. We also do that for predicates and expressions.
--
Michael

Michael Paquier

michael@paquier.xyz

about 4 years ago

In reply to: Michael Paquier (#4)

Re: Opclass parameters of indexes lost after REINDEX CONCURRENTLY[

On Sat, Oct 30, 2021 at 09:26:35PM +0900, Michael Paquier wrote:

Yeah, you are right that it would be better here to use
get_attoptions() to grab a copy of each attribute's option directly
from the catalogs. We also do that for predicates and expressions.

While looking again at this one this morning, I have extended the
tests with more columns and some default values, then applied the
patch by using get_attoptions() to grab each attribute's options as of
add5cf2. Predicates and expressions are grabbed through the syscache,
so that's just more consistent.
--
Michael