Don't overwrite scan key in systable_beginscan()
Started by Peter Eisentrautover 1 year ago11 messages
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.
I propose to fix that by making a copy of the scan keys passed by the
caller and make the modifications there.
In order to prove to myself that there are no other cases where
caller-provided scan keys are modified, I went through and
const-qualified all the APIs. This works out correctly. Several levels
down in the stack, the access methods make their own copy of the scan
keys that they store in their scan descriptors, and they use those in
non-const-clean ways, but that's ok, that's their business. As far as
the top-level callers are concerned, they can rely on their scan keys to
be const after this.
I'm not proposing this second patch for committing at this time, since
that would modify the public access method APIs in an incompatible way.
I've made a proposal of a similar nature in [0]/messages/by-id/14c31f4a-0347-0805-dce8-93a9072c05a5@eisentraut.org. At some point, it
might be worth batching these and other changes together and make the
change. I might come back to that later.
[0]: /messages/by-id/14c31f4a-0347-0805-dce8-93a9072c05a5@eisentraut.org
/messages/by-id/14c31f4a-0347-0805-dce8-93a9072c05a5@eisentraut.org
While researching how the scan keys get copied around, I noticed that
the index access methods all use memmove() to make the above-mentioned
copy into their own scan descriptor. This is fine, but memmove() is
usually only used when something special is going on that would prevent
memcpy() from working, which is not the case there. So to avoid the
confusion for future readers, I changed those to memcpy(). I suspect
that this code has been copied between the different index AM over time.
(The nbtree version of this code is literally unchanged since July 1996.)
Attachments:
0001-Don-t-overwrite-scan-key-in-systable_beginscan.patchtext/plain; charset=UTF-8; name=0001-Don-t-overwrite-scan-key-in-systable_beginscan.patchDownload
From d968667cf4808f0ff5ff2c3bd37b9bf85c9d4a11 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH 1/3] 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.
---
src/backend/access/index/genam.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index de751e8e4a3..b5eba549d3e 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -371,7 +371,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.
@@ -419,17 +419,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;
}
}
@@ -439,7 +444,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
@@ -647,6 +652,7 @@ systable_beginscan_ordered(Relation heapRelation,
{
SysScanDesc sysscan;
int i;
+ ScanKey idxkey;
/* REINDEX can probably be a hard error here ... */
if (ReindexIsProcessingIndex(RelationGetRelid(indexRelation)))
@@ -678,16 +684,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;
}
}
@@ -697,7 +707,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;
base-commit: e56ccc8e4204d9faf86f3bd2e435a0788b3d0429
--
2.46.0
0002-Augment-ScanKey-arguments-with-const-qualifiers.patchtext/plain; charset=UTF-8; name=0002-Augment-ScanKey-arguments-with-const-qualifiers.patchDownload
From 02a9af6d25eb0ddcc18947285317f53ece00f838 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH 2/3] Augment ScanKey arguments with const qualifiers
In most cases, functions do not modify a ScanKey that is passed in.
This is a sensible expection for such APIs. But the API did not
communicate that. This adds const qualifiers to the arguments to
improve that.
There are some subtleties here. For example heapgettup() and
heapgettup_pagemode() cannot guarantee that the ScanKeys are not
modified, so they do not get a const qualifiers. But that is okay,
because they work on keys that are copied into the scan descriptor by
heap_beginscan() (initscan()), so that does not interfere with the
keys passed in from the caller of heap_beginscan(). So this all works
out correctly.
---
contrib/bloom/bloom.h | 4 ++--
contrib/bloom/blscan.c | 4 ++--
src/backend/access/brin/brin.c | 4 ++--
src/backend/access/gin/ginscan.c | 4 ++--
src/backend/access/gist/gistscan.c | 4 ++--
src/backend/access/hash/hash.c | 4 ++--
src/backend/access/heap/heapam.c | 6 +++---
src/backend/access/index/genam.c | 4 ++--
src/backend/access/index/indexam.c | 4 ++--
src/backend/access/nbtree/nbtree.c | 4 ++--
src/backend/access/spgist/spgscan.c | 4 ++--
src/backend/access/table/tableam.c | 2 +-
src/include/access/amapi.h | 4 ++--
src/include/access/brin_internal.h | 4 ++--
src/include/access/genam.h | 8 ++++----
src/include/access/gin_private.h | 4 ++--
src/include/access/gistscan.h | 4 ++--
src/include/access/hash.h | 4 ++--
src/include/access/heapam.h | 4 ++--
src/include/access/nbtree.h | 4 ++--
src/include/access/spgist.h | 4 ++--
src/include/access/tableam.h | 18 +++++++++---------
.../modules/dummy_index_am/dummy_index_am.c | 4 ++--
23 files changed, 55 insertions(+), 55 deletions(-)
diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h
index fba3ba77711..fbd49c08cd4 100644
--- a/contrib/bloom/bloom.h
+++ b/contrib/bloom/bloom.h
@@ -195,8 +195,8 @@ extern bool blinsert(Relation index, Datum *values, bool *isnull,
struct IndexInfo *indexInfo);
extern IndexScanDesc blbeginscan(Relation r, int nkeys, int norderbys);
extern int64 blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void blendscan(IndexScanDesc scan);
extern IndexBuildResult *blbuild(Relation heap, Relation index,
struct IndexInfo *indexInfo);
diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index 0a303a49b24..e29dc93491a 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -45,8 +45,8 @@ blbeginscan(Relation r, int nkeys, int norderbys)
* Rescan a bloom index.
*/
void
-blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
BloomScanOpaque so = (BloomScanOpaque) scan->opaque;
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 6467bed604a..3a739eed029 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -943,8 +943,8 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
* Re-initialize state for a BRIN index scan
*/
void
-brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/*
* Other index AMs preprocess the scan keys at this point, or sometime
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index af24d38544e..7dfaa866b43 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -439,8 +439,8 @@ ginNewScanKey(IndexScanDesc scan)
}
void
-ginrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
GinScanOpaque so = (GinScanOpaque) scan->opaque;
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index e05801e2f5b..1f1c122bf97 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -124,8 +124,8 @@ gistbeginscan(Relation r, int nkeys, int norderbys)
}
void
-gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
- ScanKey orderbys, int norderbys)
+gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/* nkeys and norderbys arguments are ignored */
GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 01d06b7c328..3bb621a893f 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -394,8 +394,8 @@ hashbeginscan(Relation rel, int nkeys, int norderbys)
* hashrescan() -- rescan an index relation
*/
void
-hashrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Relation rel = scan->indexRelation;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 91b20147a00..6bb5805ec74 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -291,7 +291,7 @@ heap_scan_stream_read_next_serial(ReadStream *stream,
* ----------------
*/
static void
-initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
+initscan(HeapScanDesc scan, const ScanKeyData *key, bool keep_startblock)
{
ParallelBlockTableScanDesc bpscan = NULL;
bool allow_strat;
@@ -1036,7 +1036,7 @@ heapgettup_pagemode(HeapScanDesc scan,
TableScanDesc
heap_beginscan(Relation relation, Snapshot snapshot,
- int nkeys, ScanKey key,
+ int nkeys, const ScanKeyData *key,
ParallelTableScanDesc parallel_scan,
uint32 flags)
{
@@ -1149,7 +1149,7 @@ heap_beginscan(Relation relation, Snapshot snapshot,
}
void
-heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
+heap_rescan(TableScanDesc sscan, const ScanKeyData *key, bool set_params,
bool allow_strat, bool allow_sync, bool allow_pagemode)
{
HeapScanDesc scan = (HeapScanDesc) sscan;
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index b5eba549d3e..0f047e1fb73 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -385,7 +385,7 @@ systable_beginscan(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
- int nkeys, ScanKey key)
+ int nkeys, const ScanKeyData *key)
{
SysScanDesc sysscan;
Relation irel;
@@ -648,7 +648,7 @@ SysScanDesc
systable_beginscan_ordered(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, ScanKey key)
+ int nkeys, const ScanKeyData *key)
{
SysScanDesc sysscan;
int i;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index dcd04b813d8..f0b85155d54 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -350,8 +350,8 @@ index_beginscan_internal(Relation indexRelation,
*/
void
index_rescan(IndexScanDesc scan,
- ScanKey keys, int nkeys,
- ScanKey orderbys, int norderbys)
+ const ScanKeyData *keys, int nkeys,
+ const ScanKeyData *orderbys, int norderbys)
{
SCAN_CHECKS;
CHECK_SCAN_PROCEDURE(amrescan);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 686a3206f72..5962596b06b 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -356,8 +356,8 @@ btbeginscan(Relation rel, int nkeys, int norderbys)
* btrescan() -- rescan an index relation
*/
void
-btrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 03293a7816e..d6bea65fde5 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -377,8 +377,8 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
}
void
-spgrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
SpGistScanOpaque so = (SpGistScanOpaque) scan->opaque;
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index e57a0b7ea31..4042087813d 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -109,7 +109,7 @@ table_slot_create(Relation relation, List **reglist)
*/
TableScanDesc
-table_beginscan_catalog(Relation relation, int nkeys, struct ScanKeyData *key)
+table_beginscan_catalog(Relation relation, int nkeys, const struct ScanKeyData *key)
{
uint32 flags = SO_TYPE_SEQSCAN |
SO_ALLOW_STRAT | SO_ALLOW_SYNC | SO_ALLOW_PAGEMODE | SO_TEMP_SNAPSHOT;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index f25c9d58a7d..f10cabadce0 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -168,9 +168,9 @@ typedef IndexScanDesc (*ambeginscan_function) (Relation indexRelation,
/* (re)start index scan */
typedef void (*amrescan_function) (IndexScanDesc scan,
- ScanKey keys,
+ const ScanKeyData *keys,
int nkeys,
- ScanKey orderbys,
+ const ScanKeyData *orderbys,
int norderbys);
/* next valid tuple */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index a5a9772621c..d226a76d93a 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -99,8 +99,8 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
extern void brininsertcleanup(Relation index, struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void brinendscan(IndexScanDesc scan);
extern IndexBulkDeleteResult *brinbulkdelete(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index fdcfbe8db74..87bf823b045 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -160,8 +160,8 @@ extern IndexScanDesc index_beginscan_bitmap(Relation indexRelation,
Snapshot snapshot,
int nkeys);
extern void index_rescan(IndexScanDesc scan,
- ScanKey keys, int nkeys,
- ScanKey orderbys, int norderbys);
+ const ScanKeyData *keys, int nkeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void index_endscan(IndexScanDesc scan);
extern void index_markpos(IndexScanDesc scan);
extern void index_restrpos(IndexScanDesc scan);
@@ -222,14 +222,14 @@ extern SysScanDesc systable_beginscan(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
- int nkeys, ScanKey key);
+ int nkeys, const ScanKeyData *key);
extern HeapTuple systable_getnext(SysScanDesc sysscan);
extern bool systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup);
extern void systable_endscan(SysScanDesc sysscan);
extern SysScanDesc systable_beginscan_ordered(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, ScanKey key);
+ int nkeys, const ScanKeyData *key);
extern HeapTuple systable_getnext_ordered(SysScanDesc sysscan,
ScanDirection direction);
extern void systable_endscan_ordered(SysScanDesc sysscan);
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 3013a44bae1..7cbc7b5ca4c 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -387,8 +387,8 @@ typedef GinScanOpaqueData *GinScanOpaque;
extern IndexScanDesc ginbeginscan(Relation rel, int nkeys, int norderbys);
extern void ginendscan(IndexScanDesc scan);
-extern void ginrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void ginNewScanKey(IndexScanDesc scan);
extern void ginFreeScanKeys(GinScanOpaque so);
diff --git a/src/include/access/gistscan.h b/src/include/access/gistscan.h
index ba2153869a3..e6ea66d0457 100644
--- a/src/include/access/gistscan.h
+++ b/src/include/access/gistscan.h
@@ -17,8 +17,8 @@
#include "access/amapi.h"
extern IndexScanDesc gistbeginscan(Relation r, int nkeys, int norderbys);
-extern void gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
- ScanKey orderbys, int norderbys);
+extern void gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void gistendscan(IndexScanDesc scan);
#endif /* GISTSCAN_H */
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 9c7d81525b4..9ce53f48045 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -371,8 +371,8 @@ extern bool hashinsert(Relation rel, Datum *values, bool *isnull,
extern bool hashgettuple(IndexScanDesc scan, ScanDirection dir);
extern int64 hashgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern IndexScanDesc hashbeginscan(Relation rel, int nkeys, int norderbys);
-extern void hashrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void hashendscan(IndexScanDesc scan);
extern IndexBulkDeleteResult *hashbulkdelete(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 9e9aec88a62..4c03ce7b000 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -287,13 +287,13 @@ typedef enum
#define HeapScanIsValid(scan) PointerIsValid(scan)
extern TableScanDesc heap_beginscan(Relation relation, Snapshot snapshot,
- int nkeys, ScanKey key,
+ int nkeys, const ScanKeyData *key,
ParallelTableScanDesc parallel_scan,
uint32 flags);
extern void heap_setscanlimits(TableScanDesc sscan, BlockNumber startBlk,
BlockNumber numBlks);
extern void heap_prepare_pagescan(TableScanDesc sscan);
-extern void heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
+extern void heap_rescan(TableScanDesc sscan, const ScanKeyData *key, bool set_params,
bool allow_strat, bool allow_sync, bool allow_pagemode);
extern void heap_endscan(TableScanDesc sscan);
extern HeapTuple heap_getnext(TableScanDesc sscan, ScanDirection direction);
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 74930433480..4519a64e072 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1173,8 +1173,8 @@ extern Size btestimateparallelscan(int nkeys, int norderbys);
extern void btinitparallelscan(void *target);
extern bool btgettuple(IndexScanDesc scan, ScanDirection dir);
extern int64 btgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void btrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void btparallelrescan(IndexScanDesc scan);
extern void btendscan(IndexScanDesc scan);
extern void btmarkpos(IndexScanDesc scan);
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d6a49531200..53e2ff992dc 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -205,8 +205,8 @@ extern bool spginsert(Relation index, Datum *values, bool *isnull,
/* spgscan.c */
extern IndexScanDesc spgbeginscan(Relation rel, int keysz, int orderbysz);
extern void spgendscan(IndexScanDesc scan);
-extern void spgrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern int64 spggetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern bool spggettuple(IndexScanDesc scan, ScanDirection dir);
extern bool spgcanreturn(Relation index, int attno);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index da661289c1f..9bf33e1127d 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -327,7 +327,7 @@ typedef struct TableAmRoutine
*/
TableScanDesc (*scan_begin) (Relation rel,
Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
ParallelTableScanDesc pscan,
uint32 flags);
@@ -341,7 +341,7 @@ typedef struct TableAmRoutine
* Restart relation scan. If set_params is set to true, allow_{strat,
* sync, pagemode} (see scan_begin) changes should be taken into account.
*/
- void (*scan_rescan) (TableScanDesc scan, struct ScanKeyData *key,
+ void (*scan_rescan) (TableScanDesc scan, const struct ScanKeyData *key,
bool set_params, bool allow_strat,
bool allow_sync, bool allow_pagemode);
@@ -906,7 +906,7 @@ extern TupleTableSlot *table_slot_create(Relation relation, List **reglist);
*/
static inline TableScanDesc
table_beginscan(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key)
+ int nkeys, const struct ScanKeyData *key)
{
uint32 flags = SO_TYPE_SEQSCAN |
SO_ALLOW_STRAT | SO_ALLOW_SYNC | SO_ALLOW_PAGEMODE;
@@ -919,7 +919,7 @@ table_beginscan(Relation rel, Snapshot snapshot,
* snapshot appropriate for scanning catalog relations.
*/
extern TableScanDesc table_beginscan_catalog(Relation relation, int nkeys,
- struct ScanKeyData *key);
+ const struct ScanKeyData *key);
/*
* Like table_beginscan(), but table_beginscan_strat() offers an extended API
@@ -930,7 +930,7 @@ extern TableScanDesc table_beginscan_catalog(Relation relation, int nkeys,
*/
static inline TableScanDesc
table_beginscan_strat(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync)
{
uint32 flags = SO_TYPE_SEQSCAN | SO_ALLOW_PAGEMODE;
@@ -951,7 +951,7 @@ table_beginscan_strat(Relation rel, Snapshot snapshot,
*/
static inline TableScanDesc
table_beginscan_bm(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key, bool need_tuple)
+ int nkeys, const struct ScanKeyData *key, bool need_tuple)
{
uint32 flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
@@ -970,7 +970,7 @@ table_beginscan_bm(Relation rel, Snapshot snapshot,
*/
static inline TableScanDesc
table_beginscan_sampling(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync,
bool allow_pagemode)
{
@@ -1026,7 +1026,7 @@ table_endscan(TableScanDesc scan)
*/
static inline void
table_rescan(TableScanDesc scan,
- struct ScanKeyData *key)
+ const struct ScanKeyData *key)
{
scan->rs_rd->rd_tableam->scan_rescan(scan, key, false, false, false, false);
}
@@ -1040,7 +1040,7 @@ table_rescan(TableScanDesc scan,
* previously selected startblock will be kept.
*/
static inline void
-table_rescan_set_params(TableScanDesc scan, struct ScanKeyData *key,
+table_rescan_set_params(TableScanDesc scan, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync, bool allow_pagemode)
{
scan->rs_rd->rd_tableam->scan_rescan(scan, key, true,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 0b477116067..3fd041d5293 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -256,8 +256,8 @@ dibeginscan(Relation r, int nkeys, int norderbys)
* Rescan of index AM.
*/
static void
-direscan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+direscan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/* nothing to do */
}
--
2.46.0
0003-Replace-gratuitous-memmove-with-memcpy.patchtext/plain; charset=UTF-8; name=0003-Replace-gratuitous-memmove-with-memcpy.patchDownload
From 39b99d5fa0cd7eeeac48727e802ee97c0f8e61fe Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH 3/3] Replace gratuitous memmove() with memcpy()
The index access methods all had similar code that copied the
passed-in scan keys to local storage. They all used memmove() for
that, which is not wrong, but it seems confusing not to use memcpy()
when that would work. Presumably, this was all once copied from
ancient code and never adjusted.
---
contrib/bloom/blscan.c | 5 +----
src/backend/access/brin/brin.c | 3 +--
src/backend/access/gin/ginscan.c | 5 +----
src/backend/access/gist/gistscan.c | 6 ++----
src/backend/access/hash/hash.c | 6 +-----
src/backend/access/nbtree/nbtree.c | 4 +---
src/backend/access/spgist/spgscan.c | 6 ++----
7 files changed, 9 insertions(+), 26 deletions(-)
diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index e29dc93491a..67899c1369a 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -55,10 +55,7 @@ blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
so->sign = NULL;
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
/*
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3a739eed029..d38441e05ab 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -955,8 +955,7 @@ brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
*/
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
/*
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 7dfaa866b43..678a8ce075a 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -447,10 +447,7 @@ ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
ginFreeScanKeys(so);
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 1f1c122bf97..a10099cf150 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -233,8 +233,7 @@ gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
fn_extras[i] = scan->keyData[i].sk_func.fn_extra;
}
- memmove(scan->keyData, key,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, key, scan->numberOfKeys * sizeof(ScanKeyData));
/*
* Modify the scan key so that the Consistent method is called for all
@@ -289,8 +288,7 @@ gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
fn_extras[i] = scan->orderByData[i].sk_func.fn_extra;
}
- memmove(scan->orderByData, orderbys,
- scan->numberOfOrderBys * sizeof(ScanKeyData));
+ memcpy(scan->orderByData, orderbys, scan->numberOfOrderBys * sizeof(ScanKeyData));
so->orderByTypes = (Oid *) palloc(scan->numberOfOrderBys * sizeof(Oid));
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 3bb621a893f..ea0b95a2af3 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -414,11 +414,7 @@ hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
/* Update scan key, if a new one is given */
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData,
- scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
so->hashso_buc_populated = false;
so->hashso_buc_split = false;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 5962596b06b..24f8a3a721c 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -403,9 +403,7 @@ btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
* Reset the scan keys
*/
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData,
- scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
so->numberOfKeys = 0; /* until _bt_preprocess_keys sets it */
so->numArrayKeys = 0; /* ditto */
}
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index d6bea65fde5..8a990fba1c3 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -384,16 +384,14 @@ spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
/* copy scankeys into local storage */
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
/* initialize order-by data if needed */
if (orderbys && scan->numberOfOrderBys > 0)
{
int i;
- memmove(scan->orderByData, orderbys,
- scan->numberOfOrderBys * sizeof(ScanKeyData));
+ memcpy(scan->orderByData, orderbys, scan->numberOfOrderBys * sizeof(ScanKeyData));
for (i = 0; i < scan->numberOfOrderBys; i++)
{
--
2.46.0
Re: Don't overwrite scan key in systable_beginscan()
On Thu, Aug 8, 2024 at 2:46 AM Peter Eisentraut <peter@eisentraut.org> wrote:
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.I propose to fix that by making a copy of the scan keys passed by the
caller and make the modifications there.
This does have the disadvantage of adding more palloc overhead.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: Don't overwrite scan key in systable_beginscan()
Robert Haas <robertmhaas@gmail.com> writes:
On Thu, Aug 8, 2024 at 2:46 AM Peter Eisentraut <peter@eisentraut.org> wrote:
I propose to fix that by making a copy of the scan keys passed by the
caller and make the modifications there.
This does have the disadvantage of adding more palloc overhead.
It seems hard to believe that one more palloc + memcpy is going to be
noticeable in the context of an index scan.
regards, tom lane
Re: Don't overwrite scan key in systable_beginscan()
On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
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.I propose to fix that by making a copy of the scan keys passed by the caller
and make the modifications there.
No objection, but this would obsolete at least some of these comments (the
catcache.c ones if nothing else):
$ git grep -in scankey | grep -i copy
src/backend/access/gist/gistscan.c:257: * Copy consistent support function to ScanKey structure instead
src/backend/access/gist/gistscan.c:330: * Copy distance support function to ScanKey structure instead of
src/backend/access/nbtree/nbtutils.c:281: ScanKey arrayKeyData; /* modified copy of scan->keyData */
src/backend/access/nbtree/nbtutils.c:3303: * We copy the appropriate indoption value into the scankey sk_flags
src/backend/access/nbtree/nbtutils.c:3318: * It's a bit ugly to modify the caller's copy of the scankey but in practice
src/backend/access/spgist/spgscan.c:385: /* copy scankeys into local storage */
src/backend/utils/cache/catcache.c:1474: * Ok, need to make a lookup in the relation, copy the scankey and
src/backend/utils/cache/catcache.c:1794: * Ok, need to make a lookup in the relation, copy the scankey and
src/backend/utils/cache/relfilenumbermap.c:192: /* copy scankey to local copy, it will be modified during the scan */
In order to prove to myself that there are no other cases where
caller-provided scan keys are modified, I went through and const-qualified
all the APIs. This works out correctly. Several levels down in the stack,
the access methods make their own copy of the scan keys that they store in
their scan descriptors, and they use those in non-const-clean ways, but
that's ok, that's their business. As far as the top-level callers are
concerned, they can rely on their scan keys to be const after this.
We do still have code mutating IndexScanDescData.keyData, but that's fine. We
must be copying function args to form IndexScanDescData.keyData, or else your
proof patch would have noticed an assignment of const to non-const.
Re: Don't overwrite scan key in systable_beginscan()
Noah Misch <noah@leadboat.com> writes:
On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
I propose to fix that by making a copy of the scan keys passed by the caller
and make the modifications there.
No objection, but this would obsolete at least some of these comments (the
catcache.c ones if nothing else):
That ties into something I forgot to ask: aren't there any copy steps
or other overhead that we could remove, given this new API constraint?
That would help address Robert's concern.
regards, tom lane
Re: Don't overwrite scan key in systable_beginscan()
On 09.08.24 06:55, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
I propose to fix that by making a copy of the scan keys passed by the caller
and make the modifications there.No objection, but this would obsolete at least some of these comments (the
catcache.c ones if nothing else):That ties into something I forgot to ask: aren't there any copy steps
or other overhead that we could remove, given this new API constraint?
That would help address Robert's concern.
I added two more patches to the series here.
First (or 0004), some additional cleanup for code that had to workaround
systable_beginscan() overwriting the scan keys, along the lines
suggested by Noah.
Second (or 0005), an alternative to palloc is to make the converted scan
keys a normal local variable. Then it's just a question of whether a
smaller palloc is preferred over an over-allocated local variable. I
think I still prefer the palloc version, but it doesn't matter much
either way I think.
Attachments:
v2-0001-Don-t-overwrite-scan-key-in-systable_beginscan.patchtext/plain; charset=UTF-8; name=v2-0001-Don-t-overwrite-scan-key-in-systable_beginscan.patchDownload
From 2f84e4d21ce611a4e6f428f72a18518e22776400 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH v2 1/5] 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.
---
src/backend/access/index/genam.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index de751e8e4a3..b5eba549d3e 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -371,7 +371,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.
@@ -419,17 +419,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;
}
}
@@ -439,7 +444,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
@@ -647,6 +652,7 @@ systable_beginscan_ordered(Relation heapRelation,
{
SysScanDesc sysscan;
int i;
+ ScanKey idxkey;
/* REINDEX can probably be a hard error here ... */
if (ReindexIsProcessingIndex(RelationGetRelid(indexRelation)))
@@ -678,16 +684,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;
}
}
@@ -697,7 +707,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;
--
2.46.0
v2-0002-Augment-ScanKey-arguments-with-const-qualifiers.patchtext/plain; charset=UTF-8; name=v2-0002-Augment-ScanKey-arguments-with-const-qualifiers.patchDownload
From b69eb077340879600c0c4c03f7bde6996b82c1fc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH v2 2/5] Augment ScanKey arguments with const qualifiers
In most cases, functions do not modify a ScanKey that is passed in.
This is a sensible expection for such APIs. But the API did not
communicate that. This adds const qualifiers to the arguments to
improve that.
There are some subtleties here. For example heapgettup() and
heapgettup_pagemode() cannot guarantee that the ScanKeys are not
modified, so they do not get a const qualifiers. But that is okay,
because they work on keys that are copied into the scan descriptor by
heap_beginscan() (initscan()), so that does not interfere with the
keys passed in from the caller of heap_beginscan(). So this all works
out correctly.
---
contrib/bloom/bloom.h | 4 ++--
contrib/bloom/blscan.c | 4 ++--
src/backend/access/brin/brin.c | 4 ++--
src/backend/access/gin/ginscan.c | 4 ++--
src/backend/access/gist/gistscan.c | 4 ++--
src/backend/access/hash/hash.c | 4 ++--
src/backend/access/heap/heapam.c | 6 +++---
src/backend/access/index/genam.c | 4 ++--
src/backend/access/index/indexam.c | 4 ++--
src/backend/access/nbtree/nbtree.c | 4 ++--
src/backend/access/spgist/spgscan.c | 4 ++--
src/backend/access/table/tableam.c | 2 +-
src/include/access/amapi.h | 4 ++--
src/include/access/brin_internal.h | 4 ++--
src/include/access/genam.h | 8 ++++----
src/include/access/gin_private.h | 4 ++--
src/include/access/gistscan.h | 4 ++--
src/include/access/hash.h | 4 ++--
src/include/access/heapam.h | 4 ++--
src/include/access/nbtree.h | 4 ++--
src/include/access/spgist.h | 4 ++--
src/include/access/tableam.h | 18 +++++++++---------
.../modules/dummy_index_am/dummy_index_am.c | 4 ++--
23 files changed, 55 insertions(+), 55 deletions(-)
diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h
index fba3ba77711..fbd49c08cd4 100644
--- a/contrib/bloom/bloom.h
+++ b/contrib/bloom/bloom.h
@@ -195,8 +195,8 @@ extern bool blinsert(Relation index, Datum *values, bool *isnull,
struct IndexInfo *indexInfo);
extern IndexScanDesc blbeginscan(Relation r, int nkeys, int norderbys);
extern int64 blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void blendscan(IndexScanDesc scan);
extern IndexBuildResult *blbuild(Relation heap, Relation index,
struct IndexInfo *indexInfo);
diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index 0a303a49b24..e29dc93491a 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -45,8 +45,8 @@ blbeginscan(Relation r, int nkeys, int norderbys)
* Rescan a bloom index.
*/
void
-blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
BloomScanOpaque so = (BloomScanOpaque) scan->opaque;
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 6467bed604a..3a739eed029 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -943,8 +943,8 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
* Re-initialize state for a BRIN index scan
*/
void
-brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/*
* Other index AMs preprocess the scan keys at this point, or sometime
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index af24d38544e..7dfaa866b43 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -439,8 +439,8 @@ ginNewScanKey(IndexScanDesc scan)
}
void
-ginrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
GinScanOpaque so = (GinScanOpaque) scan->opaque;
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index e05801e2f5b..1f1c122bf97 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -124,8 +124,8 @@ gistbeginscan(Relation r, int nkeys, int norderbys)
}
void
-gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
- ScanKey orderbys, int norderbys)
+gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/* nkeys and norderbys arguments are ignored */
GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 01d06b7c328..3bb621a893f 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -394,8 +394,8 @@ hashbeginscan(Relation rel, int nkeys, int norderbys)
* hashrescan() -- rescan an index relation
*/
void
-hashrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
HashScanOpaque so = (HashScanOpaque) scan->opaque;
Relation rel = scan->indexRelation;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 91b20147a00..6bb5805ec74 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -291,7 +291,7 @@ heap_scan_stream_read_next_serial(ReadStream *stream,
* ----------------
*/
static void
-initscan(HeapScanDesc scan, ScanKey key, bool keep_startblock)
+initscan(HeapScanDesc scan, const ScanKeyData *key, bool keep_startblock)
{
ParallelBlockTableScanDesc bpscan = NULL;
bool allow_strat;
@@ -1036,7 +1036,7 @@ heapgettup_pagemode(HeapScanDesc scan,
TableScanDesc
heap_beginscan(Relation relation, Snapshot snapshot,
- int nkeys, ScanKey key,
+ int nkeys, const ScanKeyData *key,
ParallelTableScanDesc parallel_scan,
uint32 flags)
{
@@ -1149,7 +1149,7 @@ heap_beginscan(Relation relation, Snapshot snapshot,
}
void
-heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
+heap_rescan(TableScanDesc sscan, const ScanKeyData *key, bool set_params,
bool allow_strat, bool allow_sync, bool allow_pagemode)
{
HeapScanDesc scan = (HeapScanDesc) sscan;
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index b5eba549d3e..0f047e1fb73 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -385,7 +385,7 @@ systable_beginscan(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
- int nkeys, ScanKey key)
+ int nkeys, const ScanKeyData *key)
{
SysScanDesc sysscan;
Relation irel;
@@ -648,7 +648,7 @@ SysScanDesc
systable_beginscan_ordered(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, ScanKey key)
+ int nkeys, const ScanKeyData *key)
{
SysScanDesc sysscan;
int i;
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index dcd04b813d8..f0b85155d54 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -350,8 +350,8 @@ index_beginscan_internal(Relation indexRelation,
*/
void
index_rescan(IndexScanDesc scan,
- ScanKey keys, int nkeys,
- ScanKey orderbys, int norderbys)
+ const ScanKeyData *keys, int nkeys,
+ const ScanKeyData *orderbys, int norderbys)
{
SCAN_CHECKS;
CHECK_SCAN_PROCEDURE(amrescan);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 686a3206f72..5962596b06b 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -356,8 +356,8 @@ btbeginscan(Relation rel, int nkeys, int norderbys)
* btrescan() -- rescan an index relation
*/
void
-btrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 03293a7816e..d6bea65fde5 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -377,8 +377,8 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
}
void
-spgrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
SpGistScanOpaque so = (SpGistScanOpaque) scan->opaque;
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index e57a0b7ea31..4042087813d 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -109,7 +109,7 @@ table_slot_create(Relation relation, List **reglist)
*/
TableScanDesc
-table_beginscan_catalog(Relation relation, int nkeys, struct ScanKeyData *key)
+table_beginscan_catalog(Relation relation, int nkeys, const struct ScanKeyData *key)
{
uint32 flags = SO_TYPE_SEQSCAN |
SO_ALLOW_STRAT | SO_ALLOW_SYNC | SO_ALLOW_PAGEMODE | SO_TEMP_SNAPSHOT;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index f25c9d58a7d..f10cabadce0 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -168,9 +168,9 @@ typedef IndexScanDesc (*ambeginscan_function) (Relation indexRelation,
/* (re)start index scan */
typedef void (*amrescan_function) (IndexScanDesc scan,
- ScanKey keys,
+ const ScanKeyData *keys,
int nkeys,
- ScanKey orderbys,
+ const ScanKeyData *orderbys,
int norderbys);
/* next valid tuple */
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index a5a9772621c..d226a76d93a 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -99,8 +99,8 @@ extern bool brininsert(Relation idxRel, Datum *values, bool *nulls,
extern void brininsertcleanup(Relation index, struct IndexInfo *indexInfo);
extern IndexScanDesc brinbeginscan(Relation r, int nkeys, int norderbys);
extern int64 bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void brinrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void brinendscan(IndexScanDesc scan);
extern IndexBulkDeleteResult *brinbulkdelete(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index fdcfbe8db74..87bf823b045 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -160,8 +160,8 @@ extern IndexScanDesc index_beginscan_bitmap(Relation indexRelation,
Snapshot snapshot,
int nkeys);
extern void index_rescan(IndexScanDesc scan,
- ScanKey keys, int nkeys,
- ScanKey orderbys, int norderbys);
+ const ScanKeyData *keys, int nkeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void index_endscan(IndexScanDesc scan);
extern void index_markpos(IndexScanDesc scan);
extern void index_restrpos(IndexScanDesc scan);
@@ -222,14 +222,14 @@ extern SysScanDesc systable_beginscan(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
- int nkeys, ScanKey key);
+ int nkeys, const ScanKeyData *key);
extern HeapTuple systable_getnext(SysScanDesc sysscan);
extern bool systable_recheck_tuple(SysScanDesc sysscan, HeapTuple tup);
extern void systable_endscan(SysScanDesc sysscan);
extern SysScanDesc systable_beginscan_ordered(Relation heapRelation,
Relation indexRelation,
Snapshot snapshot,
- int nkeys, ScanKey key);
+ int nkeys, const ScanKeyData *key);
extern HeapTuple systable_getnext_ordered(SysScanDesc sysscan,
ScanDirection direction);
extern void systable_endscan_ordered(SysScanDesc sysscan);
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 3013a44bae1..7cbc7b5ca4c 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -387,8 +387,8 @@ typedef GinScanOpaqueData *GinScanOpaque;
extern IndexScanDesc ginbeginscan(Relation rel, int nkeys, int norderbys);
extern void ginendscan(IndexScanDesc scan);
-extern void ginrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void ginNewScanKey(IndexScanDesc scan);
extern void ginFreeScanKeys(GinScanOpaque so);
diff --git a/src/include/access/gistscan.h b/src/include/access/gistscan.h
index ba2153869a3..e6ea66d0457 100644
--- a/src/include/access/gistscan.h
+++ b/src/include/access/gistscan.h
@@ -17,8 +17,8 @@
#include "access/amapi.h"
extern IndexScanDesc gistbeginscan(Relation r, int nkeys, int norderbys);
-extern void gistrescan(IndexScanDesc scan, ScanKey key, int nkeys,
- ScanKey orderbys, int norderbys);
+extern void gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void gistendscan(IndexScanDesc scan);
#endif /* GISTSCAN_H */
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 9c7d81525b4..9ce53f48045 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -371,8 +371,8 @@ extern bool hashinsert(Relation rel, Datum *values, bool *isnull,
extern bool hashgettuple(IndexScanDesc scan, ScanDirection dir);
extern int64 hashgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern IndexScanDesc hashbeginscan(Relation rel, int nkeys, int norderbys);
-extern void hashrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void hashendscan(IndexScanDesc scan);
extern IndexBulkDeleteResult *hashbulkdelete(IndexVacuumInfo *info,
IndexBulkDeleteResult *stats,
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 9e9aec88a62..4c03ce7b000 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -287,13 +287,13 @@ typedef enum
#define HeapScanIsValid(scan) PointerIsValid(scan)
extern TableScanDesc heap_beginscan(Relation relation, Snapshot snapshot,
- int nkeys, ScanKey key,
+ int nkeys, const ScanKeyData *key,
ParallelTableScanDesc parallel_scan,
uint32 flags);
extern void heap_setscanlimits(TableScanDesc sscan, BlockNumber startBlk,
BlockNumber numBlks);
extern void heap_prepare_pagescan(TableScanDesc sscan);
-extern void heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
+extern void heap_rescan(TableScanDesc sscan, const ScanKeyData *key, bool set_params,
bool allow_strat, bool allow_sync, bool allow_pagemode);
extern void heap_endscan(TableScanDesc sscan);
extern HeapTuple heap_getnext(TableScanDesc sscan, ScanDirection direction);
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 74930433480..4519a64e072 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1173,8 +1173,8 @@ extern Size btestimateparallelscan(int nkeys, int norderbys);
extern void btinitparallelscan(void *target);
extern bool btgettuple(IndexScanDesc scan, ScanDirection dir);
extern int64 btgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
-extern void btrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern void btparallelrescan(IndexScanDesc scan);
extern void btendscan(IndexScanDesc scan);
extern void btmarkpos(IndexScanDesc scan);
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d6a49531200..53e2ff992dc 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -205,8 +205,8 @@ extern bool spginsert(Relation index, Datum *values, bool *isnull,
/* spgscan.c */
extern IndexScanDesc spgbeginscan(Relation rel, int keysz, int orderbysz);
extern void spgendscan(IndexScanDesc scan);
-extern void spgrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys);
+extern void spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys);
extern int64 spggetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern bool spggettuple(IndexScanDesc scan, ScanDirection dir);
extern bool spgcanreturn(Relation index, int attno);
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index da661289c1f..9bf33e1127d 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -327,7 +327,7 @@ typedef struct TableAmRoutine
*/
TableScanDesc (*scan_begin) (Relation rel,
Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
ParallelTableScanDesc pscan,
uint32 flags);
@@ -341,7 +341,7 @@ typedef struct TableAmRoutine
* Restart relation scan. If set_params is set to true, allow_{strat,
* sync, pagemode} (see scan_begin) changes should be taken into account.
*/
- void (*scan_rescan) (TableScanDesc scan, struct ScanKeyData *key,
+ void (*scan_rescan) (TableScanDesc scan, const struct ScanKeyData *key,
bool set_params, bool allow_strat,
bool allow_sync, bool allow_pagemode);
@@ -906,7 +906,7 @@ extern TupleTableSlot *table_slot_create(Relation relation, List **reglist);
*/
static inline TableScanDesc
table_beginscan(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key)
+ int nkeys, const struct ScanKeyData *key)
{
uint32 flags = SO_TYPE_SEQSCAN |
SO_ALLOW_STRAT | SO_ALLOW_SYNC | SO_ALLOW_PAGEMODE;
@@ -919,7 +919,7 @@ table_beginscan(Relation rel, Snapshot snapshot,
* snapshot appropriate for scanning catalog relations.
*/
extern TableScanDesc table_beginscan_catalog(Relation relation, int nkeys,
- struct ScanKeyData *key);
+ const struct ScanKeyData *key);
/*
* Like table_beginscan(), but table_beginscan_strat() offers an extended API
@@ -930,7 +930,7 @@ extern TableScanDesc table_beginscan_catalog(Relation relation, int nkeys,
*/
static inline TableScanDesc
table_beginscan_strat(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync)
{
uint32 flags = SO_TYPE_SEQSCAN | SO_ALLOW_PAGEMODE;
@@ -951,7 +951,7 @@ table_beginscan_strat(Relation rel, Snapshot snapshot,
*/
static inline TableScanDesc
table_beginscan_bm(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key, bool need_tuple)
+ int nkeys, const struct ScanKeyData *key, bool need_tuple)
{
uint32 flags = SO_TYPE_BITMAPSCAN | SO_ALLOW_PAGEMODE;
@@ -970,7 +970,7 @@ table_beginscan_bm(Relation rel, Snapshot snapshot,
*/
static inline TableScanDesc
table_beginscan_sampling(Relation rel, Snapshot snapshot,
- int nkeys, struct ScanKeyData *key,
+ int nkeys, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync,
bool allow_pagemode)
{
@@ -1026,7 +1026,7 @@ table_endscan(TableScanDesc scan)
*/
static inline void
table_rescan(TableScanDesc scan,
- struct ScanKeyData *key)
+ const struct ScanKeyData *key)
{
scan->rs_rd->rd_tableam->scan_rescan(scan, key, false, false, false, false);
}
@@ -1040,7 +1040,7 @@ table_rescan(TableScanDesc scan,
* previously selected startblock will be kept.
*/
static inline void
-table_rescan_set_params(TableScanDesc scan, struct ScanKeyData *key,
+table_rescan_set_params(TableScanDesc scan, const struct ScanKeyData *key,
bool allow_strat, bool allow_sync, bool allow_pagemode)
{
scan->rs_rd->rd_tableam->scan_rescan(scan, key, true,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 0b477116067..3fd041d5293 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -256,8 +256,8 @@ dibeginscan(Relation r, int nkeys, int norderbys)
* Rescan of index AM.
*/
static void
-direscan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
- ScanKey orderbys, int norderbys)
+direscan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
+ const ScanKeyData *orderbys, int norderbys)
{
/* nothing to do */
}
--
2.46.0
v2-0003-Replace-gratuitous-memmove-with-memcpy.patchtext/plain; charset=UTF-8; name=v2-0003-Replace-gratuitous-memmove-with-memcpy.patchDownload
From 9d091ad6b5a109e0973d32541c42ccc2b86dcbd1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH v2 3/5] Replace gratuitous memmove() with memcpy()
The index access methods all had similar code that copied the
passed-in scan keys to local storage. They all used memmove() for
that, which is not wrong, but it seems confusing not to use memcpy()
when that would work. Presumably, this was all once copied from
ancient code and never adjusted.
---
contrib/bloom/blscan.c | 5 +----
src/backend/access/brin/brin.c | 3 +--
src/backend/access/gin/ginscan.c | 5 +----
src/backend/access/gist/gistscan.c | 6 ++----
src/backend/access/hash/hash.c | 6 +-----
src/backend/access/nbtree/nbtree.c | 4 +---
src/backend/access/spgist/spgscan.c | 6 ++----
7 files changed, 9 insertions(+), 26 deletions(-)
diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index e29dc93491a..67899c1369a 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -55,10 +55,7 @@ blrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
so->sign = NULL;
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
/*
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 3a739eed029..d38441e05ab 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -955,8 +955,7 @@ brinrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
*/
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
/*
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 7dfaa866b43..678a8ce075a 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -447,10 +447,7 @@ ginrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
ginFreeScanKeys(so);
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
}
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index 1f1c122bf97..a10099cf150 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -233,8 +233,7 @@ gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
fn_extras[i] = scan->keyData[i].sk_func.fn_extra;
}
- memmove(scan->keyData, key,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, key, scan->numberOfKeys * sizeof(ScanKeyData));
/*
* Modify the scan key so that the Consistent method is called for all
@@ -289,8 +288,7 @@ gistrescan(IndexScanDesc scan, const ScanKeyData *key, int nkeys,
fn_extras[i] = scan->orderByData[i].sk_func.fn_extra;
}
- memmove(scan->orderByData, orderbys,
- scan->numberOfOrderBys * sizeof(ScanKeyData));
+ memcpy(scan->orderByData, orderbys, scan->numberOfOrderBys * sizeof(ScanKeyData));
so->orderByTypes = (Oid *) palloc(scan->numberOfOrderBys * sizeof(Oid));
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 3bb621a893f..ea0b95a2af3 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -414,11 +414,7 @@ hashrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
/* Update scan key, if a new one is given */
if (scankey && scan->numberOfKeys > 0)
- {
- memmove(scan->keyData,
- scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
- }
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
so->hashso_buc_populated = false;
so->hashso_buc_split = false;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 5962596b06b..24f8a3a721c 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -403,9 +403,7 @@ btrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
* Reset the scan keys
*/
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData,
- scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
so->numberOfKeys = 0; /* until _bt_preprocess_keys sets it */
so->numArrayKeys = 0; /* ditto */
}
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index d6bea65fde5..8a990fba1c3 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -384,16 +384,14 @@ spgrescan(IndexScanDesc scan, const ScanKeyData *scankey, int nscankeys,
/* copy scankeys into local storage */
if (scankey && scan->numberOfKeys > 0)
- memmove(scan->keyData, scankey,
- scan->numberOfKeys * sizeof(ScanKeyData));
+ memcpy(scan->keyData, scankey, scan->numberOfKeys * sizeof(ScanKeyData));
/* initialize order-by data if needed */
if (orderbys && scan->numberOfOrderBys > 0)
{
int i;
- memmove(scan->orderByData, orderbys,
- scan->numberOfOrderBys * sizeof(ScanKeyData));
+ memcpy(scan->orderByData, orderbys, scan->numberOfOrderBys * sizeof(ScanKeyData));
for (i = 0; i < scan->numberOfOrderBys; i++)
{
--
2.46.0
v2-0004-Update-some-code-that-handled-systable_beginscan-.patchtext/plain; charset=UTF-8; name=v2-0004-Update-some-code-that-handled-systable_beginscan-.patchDownload
From c14d4860eea754573eb28e86ee108e1a7e176c52 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 10 Aug 2024 16:56:45 +0200
Subject: [PATCH v2 4/5] Update some code that handled systable_beginscan()
overwriting scan key
Some code made extra copies of a scan key before systable_beginscan()
in a loop because it used to modify the scan key. This is no longer
the case, so that code can be simplified and some comments updated.
---
src/backend/utils/cache/catcache.c | 42 +++++++++++-----------
src/backend/utils/cache/relfilenumbermap.c | 7 ++--
2 files changed, 23 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 111d8a280a0..348ebf81d37 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, cur_skey),
@@ -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, cur_skey),
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);
--
2.46.0
v2-0005-Alternative-Store-converted-key-in-local-variable.patchtext/plain; charset=UTF-8; name=v2-0005-Alternative-Store-converted-key-in-local-variable.patchDownload
From e131e182698986839c5190389bd75983c2a54e35 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 12 Aug 2024 09:30:24 +0200
Subject: [PATCH v2 5/5] Alternative: Store converted key in local variable
---
src/backend/access/index/genam.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 0f047e1fb73..087d5c1b6dd 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -419,17 +419,15 @@ systable_beginscan(Relation heapRelation,
if (irel)
{
int i;
- ScanKey idxkey;
-
- idxkey = palloc_array(ScanKeyData, nkeys);
+ ScanKeyData idxkey[INDEX_MAX_KEYS];
/* Convert attribute numbers to be index column numbers. */
+ memcpy(idxkey, key, sizeof(ScanKeyData) * nkeys);
+
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])
@@ -652,7 +650,7 @@ systable_beginscan_ordered(Relation heapRelation,
{
SysScanDesc sysscan;
int i;
- ScanKey idxkey;
+ ScanKeyData idxkey[INDEX_MAX_KEYS];
/* REINDEX can probably be a hard error here ... */
if (ReindexIsProcessingIndex(RelationGetRelid(indexRelation)))
@@ -684,15 +682,13 @@ systable_beginscan_ordered(Relation heapRelation,
sysscan->snapshot = NULL;
}
- idxkey = palloc_array(ScanKeyData, nkeys);
-
/* Convert attribute numbers to be index column numbers. */
+ memcpy(idxkey, key, sizeof(ScanKeyData) * nkeys);
+
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])
--
2.46.0
Re: Don't overwrite scan key in systable_beginscan()
On 12.08.24 09:44, Peter Eisentraut wrote:
On 09.08.24 06:55, Tom Lane wrote:
Noah Misch <noah@leadboat.com> writes:
On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
I propose to fix that by making a copy of the scan keys passed by
the caller
and make the modifications there.No objection, but this would obsolete at least some of these comments
(the
catcache.c ones if nothing else):That ties into something I forgot to ask: aren't there any copy steps
or other overhead that we could remove, given this new API constraint?
That would help address Robert's concern.I added two more patches to the series here.
First (or 0004), some additional cleanup for code that had to workaround
systable_beginscan() overwriting the scan keys, along the lines
suggested by Noah.Second (or 0005), an alternative to palloc is to make the converted scan
keys a normal local variable. Then it's just a question of whether a
smaller palloc is preferred over an over-allocated local variable. I
think I still prefer the palloc version, but it doesn't matter much
either way I think.
Looks like the discussion here has settled, so I plan to go head with
patches
[PATCH v2 1/5] Don't overwrite scan key in systable_beginscan()
[PATCH v2 3/5] Replace gratuitous memmove() with memcpy()
[PATCH v2 4/5] Update some code that handled systable_beginscan()
overwriting scan key
(folding patch 4 into patch 1)
Re: Don't overwrite scan key in systable_beginscan()
On Mon, Aug 12, 2024 at 09:44:02AM +0200, Peter Eisentraut wrote:
Second (or 0005), an alternative to palloc is to make the converted scan
keys a normal local variable. Then it's just a question of whether a
smaller palloc is preferred over an over-allocated local variable. I think
I still prefer the palloc version, but it doesn't matter much either way I
think.
Since 811af9786b, the palloc'd idxkey's seem to be leaking/accumulating
throughout the command.
I noticed this on the master branch while running ANALYZE on partitioned
table with 600 attributes, even though only 6 were being analyzed.
LOG: level: 3; BuildRelationExtStatistics: 1239963512 total in 278 blocks; 5082984 free (296 chunks); 1234880528 used
Several indexes are being scanned many thousands of times.
310690 beginscan 2659
310689 beginscan 2662
77672 beginscan 2678
postgres=# SELECT 2659::REGCLASS;
regclass | pg_attribute_relid_attnum_index
postgres=# SELECT 2662::REGCLASS;
regclass | pg_class_oid_index
postgres=# SELECT 2678::REGCLASS;
regclass | pg_index_indrelid_index
From 2f84e4d21ce611a4e6f428f72a18518e22776400 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 8 Aug 2024 08:27:26 +0200
Subject: [PATCH v2 1/5] Don't overwrite scan key in systable_beginscan()
...
Show quoted text
Fix that by making a copy of the scan keys passed by the caller and
make the modifications there.diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index de751e8e4a3..b5eba549d3e 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -419,17 +419,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++)
Re: Don't overwrite scan key in systable_beginscan()
On 26.11.24 14:56, Justin Pryzby wrote:
Since 811af9786b, the palloc'd idxkey's seem to be leaking/accumulating
throughout the command.I noticed this on the master branch while running ANALYZE on partitioned
table with 600 attributes, even though only 6 were being analyzed.LOG: level: 3; BuildRelationExtStatistics: 1239963512 total in 278 blocks; 5082984 free (296 chunks); 1234880528 used
Several indexes are being scanned many thousands of times.
Hmm, this patch inserts one additional palloc() call per
systable_beginscan(). So it won't have much of an impact for isolated
calls, but for thousands of scans you get thousands of small chunks of
memory.
Does your test case get better if you insert corresponding pfree() calls?
A higher-level question would be whether there should be better memory
management in the caller. But it would be okay to address it with a
pfree() as well, I think.
Re: Don't overwrite scan key in systable_beginscan()
On Wed, Nov 27, 2024 at 04:33:25PM +0100, Peter Eisentraut wrote:
On 26.11.24 14:56, Justin Pryzby wrote:
Since 811af9786b, the palloc'd idxkey's seem to be leaking/accumulating
throughout the command.I noticed this on the master branch while running ANALYZE on partitioned
table with 600 attributes, even though only 6 were being analyzed.LOG: level: 3; BuildRelationExtStatistics: 1239963512 total in 278 blocks; 5082984 free (296 chunks); 1234880528 used
Several indexes are being scanned many thousands of times.
Hmm, this patch inserts one additional palloc() call per
systable_beginscan(). So it won't have much of an impact for isolated
calls, but for thousands of scans you get thousands of small chunks of
memory.Does your test case get better if you insert corresponding pfree() calls?
Yes -- I'd already checked.
--
Justin
Re: Don't overwrite scan key in systable_beginscan()
On 27.11.24 16:35, Justin Pryzby wrote:
On Wed, Nov 27, 2024 at 04:33:25PM +0100, Peter Eisentraut wrote:
On 26.11.24 14:56, Justin Pryzby wrote:
Since 811af9786b, the palloc'd idxkey's seem to be leaking/accumulating
throughout the command.I noticed this on the master branch while running ANALYZE on partitioned
table with 600 attributes, even though only 6 were being analyzed.LOG: level: 3; BuildRelationExtStatistics: 1239963512 total in 278 blocks; 5082984 free (296 chunks); 1234880528 used
Several indexes are being scanned many thousands of times.
Hmm, this patch inserts one additional palloc() call per
systable_beginscan(). So it won't have much of an impact for isolated
calls, but for thousands of scans you get thousands of small chunks of
memory.Does your test case get better if you insert corresponding pfree() calls?
Yes -- I'd already checked.
Ok, I committed a fix that inserts some pfree() calls. Thanks for the
report.