From 811af9786b919d7acb22ea00ecb63f47de7942cd Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 8 Aug 2024 08:27:26 +0200 Subject: [PATCH] Don't overwrite scan key in systable_beginscan() When systable_beginscan() and systable_beginscan_ordered() choose an index scan, they remap the attribute numbers in the passed-in scan keys to the attribute numbers of the index, and then write those remapped attribute numbers back into the scan key passed by the caller. This second part is surprising and gratuitous. It means that a scan key cannot safely be used more than once (but it might sometimes work, depending on circumstances). Also, there is no value in providing these remapped attribute numbers back to the caller, since they can't do anything with that. Fix that by making a copy of the scan keys passed by the caller and make the modifications there. Also, some code that had to work around the previous situation is simplified. Discussion: https://www.postgresql.org/message-id/flat/f8c739d9-f48d-4187-b214-df3391ba41ab@eisentraut.org --- src/backend/access/index/genam.c | 24 +++++++++---- src/backend/utils/cache/catcache.c | 42 +++++++++++----------- src/backend/utils/cache/relfilenumbermap.c | 7 ++-- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index 43c95d6109b..4ec43e3c0c0 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -372,7 +372,7 @@ index_compute_xid_horizon_for_tuples(Relation irel, * nkeys, key: scan keys * * The attribute numbers in the scan key should be set for the heap case. - * If we choose to index, we reset them to 1..n to reference the index + * If we choose to index, we convert them to 1..n to reference the index * columns. Note this means there must be one scankey qualification per * index column! This is checked by the Asserts in the normal, index-using * case, but won't be checked if the heapscan path is taken. @@ -420,17 +420,22 @@ systable_beginscan(Relation heapRelation, if (irel) { int i; + ScanKey idxkey; - /* Change attribute numbers to be index column numbers. */ + idxkey = palloc_array(ScanKeyData, nkeys); + + /* Convert attribute numbers to be index column numbers. */ for (i = 0; i < nkeys; i++) { int j; + memcpy(&idxkey[i], &key[i], sizeof(ScanKeyData)); + for (j = 0; j < IndexRelationGetNumberOfAttributes(irel); j++) { if (key[i].sk_attno == irel->rd_index->indkey.values[j]) { - key[i].sk_attno = j + 1; + idxkey[i].sk_attno = j + 1; break; } } @@ -440,7 +445,7 @@ systable_beginscan(Relation heapRelation, sysscan->iscan = index_beginscan(heapRelation, irel, snapshot, nkeys, 0); - index_rescan(sysscan->iscan, key, nkeys, NULL, 0); + index_rescan(sysscan->iscan, idxkey, nkeys, NULL, 0); sysscan->scan = NULL; } else @@ -648,6 +653,7 @@ systable_beginscan_ordered(Relation heapRelation, { SysScanDesc sysscan; int i; + ScanKey idxkey; /* REINDEX can probably be a hard error here ... */ if (ReindexIsProcessingIndex(RelationGetRelid(indexRelation))) @@ -679,16 +685,20 @@ systable_beginscan_ordered(Relation heapRelation, sysscan->snapshot = NULL; } - /* Change attribute numbers to be index column numbers. */ + idxkey = palloc_array(ScanKeyData, nkeys); + + /* Convert attribute numbers to be index column numbers. */ for (i = 0; i < nkeys; i++) { int j; + memcpy(&idxkey[i], &key[i], sizeof(ScanKeyData)); + for (j = 0; j < IndexRelationGetNumberOfAttributes(indexRelation); j++) { if (key[i].sk_attno == indexRelation->rd_index->indkey.values[j]) { - key[i].sk_attno = j + 1; + idxkey[i].sk_attno = j + 1; break; } } @@ -698,7 +708,7 @@ systable_beginscan_ordered(Relation heapRelation, sysscan->iscan = index_beginscan(heapRelation, indexRelation, snapshot, nkeys, 0); - index_rescan(sysscan->iscan, key, nkeys, NULL, 0); + index_rescan(sysscan->iscan, idxkey, nkeys, NULL, 0); sysscan->scan = NULL; return sysscan; diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index 492a033aa2c..10276aa1db1 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -1468,19 +1468,18 @@ SearchCatCacheMiss(CatCache *cache, */ relation = table_open(cache->cc_reloid, AccessShareLock); + /* + * Ok, need to make a lookup in the relation, copy the scankey and fill + * out any per-call fields. + */ + memcpy(cur_skey, cache->cc_skey, sizeof(ScanKeyData) * nkeys); + cur_skey[0].sk_argument = v1; + cur_skey[1].sk_argument = v2; + cur_skey[2].sk_argument = v3; + cur_skey[3].sk_argument = v4; + do { - /* - * Ok, need to make a lookup in the relation, copy the scankey and - * fill out any per-call fields. (We must re-do this when retrying, - * because systable_beginscan scribbles on the scankey.) - */ - memcpy(cur_skey, cache->cc_skey, sizeof(ScanKeyData) * nkeys); - cur_skey[0].sk_argument = v1; - cur_skey[1].sk_argument = v2; - cur_skey[2].sk_argument = v3; - cur_skey[3].sk_argument = v4; - scandesc = systable_beginscan(relation, cache->cc_indexoid, IndexScanOK(cache), @@ -1788,19 +1787,18 @@ SearchCatCacheList(CatCache *cache, relation = table_open(cache->cc_reloid, AccessShareLock); + /* + * Ok, need to make a lookup in the relation, copy the scankey and + * fill out any per-call fields. + */ + memcpy(cur_skey, cache->cc_skey, sizeof(ScanKeyData) * cache->cc_nkeys); + cur_skey[0].sk_argument = v1; + cur_skey[1].sk_argument = v2; + cur_skey[2].sk_argument = v3; + cur_skey[3].sk_argument = v4; + do { - /* - * Ok, need to make a lookup in the relation, copy the scankey and - * fill out any per-call fields. (We must re-do this when - * retrying, because systable_beginscan scribbles on the scankey.) - */ - memcpy(cur_skey, cache->cc_skey, sizeof(ScanKeyData) * cache->cc_nkeys); - cur_skey[0].sk_argument = v1; - cur_skey[1].sk_argument = v2; - cur_skey[2].sk_argument = v3; - cur_skey[3].sk_argument = v4; - scandesc = systable_beginscan(relation, cache->cc_indexoid, IndexScanOK(cache), diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c index 9e76f745297..8dbccdb551e 100644 --- a/src/backend/utils/cache/relfilenumbermap.c +++ b/src/backend/utils/cache/relfilenumbermap.c @@ -141,7 +141,6 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber) SysScanDesc scandesc; Relation relation; HeapTuple ntp; - ScanKeyData skey[2]; Oid relid; if (RelfilenumberMapHash == NULL) @@ -181,6 +180,8 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber) } else { + ScanKeyData skey[2]; + /* * Not a shared table, could either be a plain relation or a * non-shared, nailed one, like e.g. pg_class. @@ -189,10 +190,8 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber) /* check for plain relations by looking in pg_class */ relation = table_open(RelationRelationId, AccessShareLock); - /* copy scankey to local copy, it will be modified during the scan */ + /* copy scankey to local copy and set scan arguments */ memcpy(skey, relfilenumber_skey, sizeof(skey)); - - /* set scan arguments */ skey[0].sk_argument = ObjectIdGetDatum(reltablespace); skey[1].sk_argument = ObjectIdGetDatum(relfilenumber);