On 09.08.24 06:55, Tom Lane wrote:
Noah Misch <n...@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.
From 2f84e4d21ce611a4e6f428f72a18518e22776400 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From b69eb077340879600c0c4c03f7bde6996b82c1fc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From 9d091ad6b5a109e0973d32541c42ccc2b86dcbd1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From c14d4860eea754573eb28e86ee108e1a7e176c52 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
From e131e182698986839c5190389bd75983c2a54e35 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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
