Remove IndexInfo.ii_OpclassOptions field
During some refactoring I noticed that the field
IndexInfo.ii_OpclassOptions is kind of useless. The IndexInfo struct is
notionally an executor support node, but this field is not used in the
executor or by the index AM code. It is really just used in DDL code in
index.c and indexcmds.c to pass information around locally. For that,
it would be clearer to just use local variables, like for other similar
cases. With that change, we can also remove
RelationGetIndexRawAttOptions(), which only had one caller left, for
which it was overkill.
Attachments:
0001-Remove-IndexInfo.ii_OpclassOptions-field.patchtext/plain; charset=UTF-8; name=0001-Remove-IndexInfo.ii_OpclassOptions-field.patchDownload
From f8b629052e75835ab255bfa4e1ce6ada570fa9e2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 24 Aug 2023 08:26:13 +0200
Subject: [PATCH 1/3] Remove IndexInfo.ii_OpclassOptions field
It is unnecessary to include this field in IndexInfo. It is only used
by DDL code, not during execution. It is really only used to pass
local information around between functions in index.c and indexcmds.c,
for which it is clearer to use local variables, like in similar cases.
---
src/backend/catalog/index.c | 31 ++++++++++++++-----------------
src/backend/catalog/toasting.c | 3 +--
src/backend/commands/indexcmds.c | 30 +++++++++++++++++-------------
src/backend/nodes/makefuncs.c | 3 ---
src/include/catalog/index.h | 1 +
src/include/nodes/execnodes.h | 1 -
6 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index fd09378848..9b0cbd8979 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -721,6 +721,7 @@ index_create(Relation heapRelation,
Oid tableSpaceId,
const Oid *collationIds,
const Oid *opclassIds,
+ const Datum *opclassOptions,
const int16 *coloptions,
Datum reloptions,
bits16 flags,
@@ -1016,7 +1017,7 @@ index_create(Relation heapRelation,
/*
* append ATTRIBUTE tuples for the index
*/
- AppendAttributeTuples(indexRelation, indexInfo->ii_OpclassOptions);
+ AppendAttributeTuples(indexRelation, opclassOptions);
/* ----------------
* update pg_index
@@ -1224,10 +1225,10 @@ index_create(Relation heapRelation,
indexRelation->rd_index->indnkeyatts = indexInfo->ii_NumIndexKeyAttrs;
/* Validate opclass-specific options */
- if (indexInfo->ii_OpclassOptions)
+ if (opclassOptions)
for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
(void) index_opclass_options(indexRelation, i + 1,
- indexInfo->ii_OpclassOptions[i],
+ opclassOptions[i],
true);
/*
@@ -1291,7 +1292,8 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
classTuple;
Datum indclassDatum,
colOptionDatum,
- optionDatum;
+ reloptionsDatum;
+ Datum *opclassOptions;
oidvector *indclass;
int2vector *indcoloptions;
bool isnull;
@@ -1325,11 +1327,11 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
Anum_pg_index_indoption);
indcoloptions = (int2vector *) DatumGetPointer(colOptionDatum);
- /* Fetch options of index if any */
+ /* Fetch reloptions of index if any */
classTuple = SearchSysCache1(RELOID, ObjectIdGetDatum(oldIndexId));
if (!HeapTupleIsValid(classTuple))
elog(ERROR, "cache lookup failed for relation %u", oldIndexId);
- optionDatum = SysCacheGetAttr(RELOID, classTuple,
+ reloptionsDatum = SysCacheGetAttr(RELOID, classTuple,
Anum_pg_class_reloptions, &isnull);
/*
@@ -1393,14 +1395,10 @@ 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 != NULL)
- {
- newInfo->ii_OpclassOptions = palloc0(sizeof(Datum) *
- newInfo->ii_NumIndexAttrs);
- for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
- newInfo->ii_OpclassOptions[i] = get_attoptions(oldIndexId, i + 1);
- }
+ /* Extract opclass options for each attribute */
+ opclassOptions = palloc0(sizeof(Datum) * newInfo->ii_NumIndexAttrs);
+ for (int i = 0; i < newInfo->ii_NumIndexAttrs; i++)
+ opclassOptions[i] = get_attoptions(oldIndexId, i + 1);
/*
* Now create the new index.
@@ -1421,8 +1419,9 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
tablespaceOid,
indexRelation->rd_indcollation,
indclass->values,
+ opclassOptions,
indcoloptions->values,
- optionDatum,
+ reloptionsDatum,
INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT,
0,
true, /* allow table to be a system catalog? */
@@ -2465,8 +2464,6 @@ BuildIndexInfo(Relation index)
&ii->ii_ExclusionStrats);
}
- ii->ii_OpclassOptions = RelationGetIndexRawAttOptions(index);
-
return ii;
}
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 3fef593bf1..989f820791 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -299,7 +299,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
- indexInfo->ii_OpclassOptions = NULL;
indexInfo->ii_Unique = true;
indexInfo->ii_NullsNotDistinct = false;
indexInfo->ii_ReadyForInserts = true;
@@ -327,7 +326,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
rel->rd_rel->reltablespace,
- collationIds, opclassIds, coloptions, (Datum) 0,
+ collationIds, opclassIds, NULL, coloptions, (Datum) 0,
INDEX_CREATE_IS_PRIMARY, 0, true, true, NULL);
table_close(toast_rel, NoLock);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ab8b81b302..a53861cecf 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -77,6 +77,7 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
Oid *typeOids,
Oid *collationOids,
Oid *opclassOids,
+ Datum *opclassOptions,
int16 *colOptions,
const List *attList,
const List *exclusionOpNames,
@@ -177,6 +178,7 @@ CheckIndexCompatible(Oid oldId,
Oid *typeIds;
Oid *collationIds;
Oid *opclassIds;
+ Datum *opclassOptions;
Oid accessMethodId;
Oid relationId;
HeapTuple tuple;
@@ -238,9 +240,10 @@ CheckIndexCompatible(Oid oldId,
typeIds = palloc_array(Oid, numberOfAttributes);
collationIds = palloc_array(Oid, numberOfAttributes);
opclassIds = palloc_array(Oid, numberOfAttributes);
+ opclassOptions = palloc_array(Datum, numberOfAttributes);
coloptions = palloc_array(int16, numberOfAttributes);
ComputeIndexAttrs(indexInfo,
- typeIds, collationIds, opclassIds,
+ typeIds, collationIds, opclassIds, opclassOptions,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
@@ -298,13 +301,12 @@ CheckIndexCompatible(Oid oldId,
/* Any change in opclass options break compatibility. */
if (ret)
{
- Datum *opclassOptions = RelationGetIndexRawAttOptions(irel);
+ Datum *oldOpclassOptions = RelationGetIndexRawAttOptions(irel);
- ret = CompareOpclassOptions(opclassOptions,
- indexInfo->ii_OpclassOptions, old_natts);
+ ret = CompareOpclassOptions(oldOpclassOptions, opclassOptions, old_natts);
- if (opclassOptions)
- pfree(opclassOptions);
+ if (oldOpclassOptions)
+ pfree(oldOpclassOptions);
}
/* Any change in exclusion operator selections breaks compatibility. */
@@ -540,6 +542,7 @@ DefineIndex(Oid tableId,
Oid *typeIds;
Oid *collationIds;
Oid *opclassIds;
+ Datum *opclassOptions;
Oid accessMethodId;
Oid namespaceId;
Oid tablespaceId;
@@ -900,9 +903,10 @@ DefineIndex(Oid tableId,
typeIds = palloc_array(Oid, numberOfAttributes);
collationIds = palloc_array(Oid, numberOfAttributes);
opclassIds = palloc_array(Oid, numberOfAttributes);
+ opclassOptions = palloc_array(Datum, numberOfAttributes);
coloptions = palloc_array(int16, numberOfAttributes);
ComputeIndexAttrs(indexInfo,
- typeIds, collationIds, opclassIds,
+ typeIds, collationIds, opclassIds, opclassOptions,
coloptions, allIndexParams,
stmt->excludeOpNames, tableId,
accessMethodName, accessMethodId,
@@ -1179,7 +1183,7 @@ DefineIndex(Oid tableId,
parentConstraintId,
stmt->oldNumber, indexInfo, indexColNames,
accessMethodId, tablespaceId,
- collationIds, opclassIds,
+ collationIds, opclassIds, opclassOptions,
coloptions, reloptions,
flags, constr_flags,
allowSystemTableMods, !check_rights,
@@ -1855,6 +1859,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
Oid *typeOids,
Oid *collationOids,
Oid *opclassOids,
+ Datum *opclassOptions,
int16 *colOptions,
const List *attList, /* list of IndexElem's */
const List *exclusionOpNames,
@@ -2011,6 +2016,7 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
errmsg("including column does not support NULLS FIRST/LAST options")));
opclassOids[attn] = InvalidOid;
+ opclassOptions[attn] = (Datum) 0;
colOptions[attn] = 0;
collationOids[attn] = InvalidOid;
attn++;
@@ -2202,14 +2208,12 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
Assert(attn < nkeycols);
- if (!indexInfo->ii_OpclassOptions)
- indexInfo->ii_OpclassOptions =
- palloc0_array(Datum, indexInfo->ii_NumIndexAttrs);
-
- indexInfo->ii_OpclassOptions[attn] =
+ opclassOptions[attn] =
transformRelOptions((Datum) 0, attribute->opclassopts,
NULL, NULL, false, false);
}
+ else
+ opclassOptions[attn] = (Datum) 0;
attn++;
}
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 0e7e6e46d9..c6fb571982 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -777,9 +777,6 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions,
n->ii_ExclusionProcs = NULL;
n->ii_ExclusionStrats = NULL;
- /* opclass options */
- n->ii_OpclassOptions = NULL;
-
/* speculative inserts */
n->ii_UniqueOps = NULL;
n->ii_UniqueProcs = NULL;
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 4d8ba81f90..096e4830ba 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -78,6 +78,7 @@ extern Oid index_create(Relation heapRelation,
Oid tableSpaceId,
const Oid *collationIds,
const Oid *opclassIds,
+ const Datum *opclassOptions,
const int16 *coloptions,
Datum reloptions,
bits16 flags,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cb714f4a19..8b65deeea9 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -187,7 +187,6 @@ typedef struct IndexInfo
Oid *ii_UniqueOps; /* array with one entry per column */
Oid *ii_UniqueProcs; /* array with one entry per column */
uint16 *ii_UniqueStrats; /* array with one entry per column */
- Datum *ii_OpclassOptions; /* array with one entry per column */
bool ii_Unique;
bool ii_NullsNotDistinct;
bool ii_ReadyForInserts;
--
2.41.0
0002-Remove-unused-include.patchtext/plain; charset=UTF-8; name=0002-Remove-unused-include.patchDownload
From cba1df590e3806b176b57e2854efc30f35d78430 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 24 Aug 2023 08:26:13 +0200
Subject: [PATCH 2/3] Remove unused include
This was added in add5cf28d4 but was apparently never used.
---
src/backend/catalog/index.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 9b0cbd8979..36c19f90ef 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -74,7 +74,6 @@
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "utils/builtins.h"
-#include "utils/datum.h"
#include "utils/fmgroids.h"
#include "utils/guc.h"
#include "utils/inval.h"
--
2.41.0
0003-Remove-RelationGetIndexRawAttOptions.patchtext/plain; charset=UTF-8; name=0003-Remove-RelationGetIndexRawAttOptions.patchDownload
From d123e508f8821e053ad57c5405045e138bc3d405 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 24 Aug 2023 08:26:13 +0200
Subject: [PATCH 3/3] Remove RelationGetIndexRawAttOptions()
There was only one caller left, for which this function was overkill.
Also, having it in relcache.c was inappropriate, since it doesn't work
with the relcache at all.
---
src/backend/commands/indexcmds.c | 8 +++++---
src/backend/utils/cache/relcache.c | 29 -----------------------------
src/include/utils/relcache.h | 1 -
3 files changed, 5 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a53861cecf..c160d8a301 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -301,12 +301,14 @@ CheckIndexCompatible(Oid oldId,
/* Any change in opclass options break compatibility. */
if (ret)
{
- Datum *oldOpclassOptions = RelationGetIndexRawAttOptions(irel);
+ Datum *oldOpclassOptions = palloc_array(Datum, old_natts);
+
+ for (i = 0; i < old_natts; i++)
+ oldOpclassOptions[i] = get_attoptions(oldId, i + 1);
ret = CompareOpclassOptions(oldOpclassOptions, opclassOptions, old_natts);
- if (oldOpclassOptions)
- pfree(oldOpclassOptions);
+ pfree(oldOpclassOptions);
}
/* Any change in exclusion operator selections breaks compatibility. */
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 8e08ca1c68..dd09a5f896 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5799,35 +5799,6 @@ RelationBuildPublicationDesc(Relation relation, PublicationDesc *pubdesc)
MemoryContextSwitchTo(oldcxt);
}
-/*
- * RelationGetIndexRawAttOptions -- get AM/opclass-specific options for the index
- */
-Datum *
-RelationGetIndexRawAttOptions(Relation indexrel)
-{
- Oid indexrelid = RelationGetRelid(indexrel);
- int16 natts = RelationGetNumberOfAttributes(indexrel);
- Datum *options = NULL;
- int16 attnum;
-
- for (attnum = 1; attnum <= natts; attnum++)
- {
- if (indexrel->rd_indam->amoptsprocnum == 0)
- continue;
-
- if (!OidIsValid(index_getprocid(indexrel, attnum,
- indexrel->rd_indam->amoptsprocnum)))
- continue;
-
- if (!options)
- options = palloc0(sizeof(Datum) * natts);
-
- options[attnum - 1] = get_attoptions(indexrelid, attnum);
- }
-
- return options;
-}
-
static bytea **
CopyIndexAttOptions(bytea **srcopts, int natts)
{
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 38524641f4..b0955c0e62 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -51,7 +51,6 @@ extern Oid RelationGetReplicaIndex(Relation relation);
extern List *RelationGetIndexExpressions(Relation relation);
extern List *RelationGetDummyIndexExpressions(Relation relation);
extern List *RelationGetIndexPredicate(Relation relation);
-extern Datum *RelationGetIndexRawAttOptions(Relation indexrel);
extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
/*
--
2.41.0
On Thu, Aug 24, 2023 at 08:57:58AM +0200, Peter Eisentraut wrote:
During some refactoring I noticed that the field IndexInfo.ii_OpclassOptions
is kind of useless. The IndexInfo struct is notionally an executor support
node, but this field is not used in the executor or by the index AM code.
It is really just used in DDL code in index.c and indexcmds.c to pass
information around locally. For that, it would be clearer to just use local
variables, like for other similar cases. With that change, we can also
remove RelationGetIndexRawAttOptions(), which only had one caller left, for
which it was overkill.
I am not so sure. There is a very recent thread where it has been
pointed out that we have zero support for relcache invalidation with
index options, causing various problems:
/messages/by-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg@mail.gmail.com
Perhaps we'd better settle on the other one before deciding if the
change you are proposing here is adapted or not.
--
Michael
On 25.08.23 03:31, Michael Paquier wrote:
On Thu, Aug 24, 2023 at 08:57:58AM +0200, Peter Eisentraut wrote:
During some refactoring I noticed that the field IndexInfo.ii_OpclassOptions
is kind of useless. The IndexInfo struct is notionally an executor support
node, but this field is not used in the executor or by the index AM code.
It is really just used in DDL code in index.c and indexcmds.c to pass
information around locally. For that, it would be clearer to just use local
variables, like for other similar cases. With that change, we can also
remove RelationGetIndexRawAttOptions(), which only had one caller left, for
which it was overkill.I am not so sure. There is a very recent thread where it has been
pointed out that we have zero support for relcache invalidation with
index options, causing various problems:
/messages/by-id/CAGem3qAM7M7B3DdccpgepRxuoKPd2Y74qJ5NSNRjLiN21dPhgg@mail.gmail.comPerhaps we'd better settle on the other one before deciding if the
change you are proposing here is adapted or not.
Ok, I'll wait for the resolution of that.
At a glance, however, I think my patch is (a) not related, and (b) if it
were, it would probably *help*, because the change is to not allocate
any long-lived structures that no one needs and that might get out of date.
On Tue, Aug 29, 2023 at 10:51:10AM +0200, Peter Eisentraut wrote:
At a glance, however, I think my patch is (a) not related, and (b) if it
were, it would probably *help*, because the change is to not allocate any
long-lived structures that no one needs and that might get out of date.
Hmm, yeah, perhaps you're right about (b) here. I have a few other
high-priority items for stable branches on my board before being able
to look at all this in more details, unfortunately, so feel free to
ignore me if you think that this is an improvement anyway even
regarding the other issue discussed.
--
Michael
On 30.08.23 02:51, Michael Paquier wrote:
On Tue, Aug 29, 2023 at 10:51:10AM +0200, Peter Eisentraut wrote:
At a glance, however, I think my patch is (a) not related, and (b) if it
were, it would probably *help*, because the change is to not allocate any
long-lived structures that no one needs and that might get out of date.Hmm, yeah, perhaps you're right about (b) here. I have a few other
high-priority items for stable branches on my board before being able
to look at all this in more details, unfortunately, so feel free to
ignore me if you think that this is an improvement anyway even
regarding the other issue discussed.
I have committed this.