Re: checking snapshot argument for index_beginscan
Show quoted text
Hi,
I was looking at commit 941aa6a6268a6a66f6895401aad6b5329111d412 .I think it would be better to move the assertion into
`index_beginscan_internal` because it is called by index_beginscan
and index_beginscan_bitmapIn the future, when a new variant of `index_beginscan_XX` is added, the
assertion would be effective for the new variant.Please let me know what you think.
Cheers
Attachments:
index-begin-scan-check-snapshot.patchapplication/octet-stream; name=index-begin-scan-check-snapshot.patchDownload
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index dc303995e5..fc419fd8e6 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -209,8 +209,6 @@ index_beginscan(Relation heapRelation,
{
IndexScanDesc scan;
- Assert(snapshot != InvalidSnapshot);
-
scan = index_beginscan_internal(indexRelation, nkeys, norderbys, snapshot, NULL, false);
/*
@@ -239,8 +237,6 @@ index_beginscan_bitmap(Relation indexRelation,
{
IndexScanDesc scan;
- Assert(snapshot != InvalidSnapshot);
-
scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot, NULL, false);
/*
@@ -265,6 +261,8 @@ index_beginscan_internal(Relation indexRelation,
RELATION_CHECKS;
CHECK_REL_PROCEDURE(ambeginscan);
+ Assert(snapshot != InvalidSnapshot);
+
if (!(indexRelation->rd_indam->ampredlocks))
PredicateLockRelation(indexRelation, snapshot);
Import Notes
Reply to msg id not found: CALte62xUkqbf0ZKSJTA306nrLJwrJniU48+DgnjXATzZDhdoqg@mail.gmail.comReference msg id not found: CALte62xUkqbf0ZKSJTA306nrLJwrJniU48+DgnjXATzZDhdoqg@mail.gmail.com
On Fri, 23 Dec 2022 at 00:36, Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
I was looking at commit 941aa6a6268a6a66f6895401aad6b5329111d412 .I think it would be better to move the assertion into `index_beginscan_internal` because it is called by index_beginscan and index_beginscan_bitmap
In the future, when a new variant of `index_beginscan_XX` is added, the assertion would be effective for the new variant.
Please let me know what you think.
Hi, Ted!
The two asserts out of four could be combined as you proposed in the patch.
Assert added by 941aa6a6268a6a66f6 to index_parallelscan_initialize
should remain anyway as otherwise, we can have Segfault in
SerializeSnapshot called from this function.
Assert in index_parallelscan_estimate can be omitted as there is the
same assert inside EstimateSnapshotSpace called from this function.
I've included it into version 2 of a patch.
Not sure it's worth the effort but IMO the patch is right and can be committed.
Kind regards,
Pavel Borisov,
Supabase.
Attachments:
v2-0001-Optimize-asserts-introduced-in-941aa6a6268a6a66f6.patchapplication/x-patch; name=v2-0001-Optimize-asserts-introduced-in-941aa6a6268a6a66f6.patchDownload
From ea3f6ca585156d0b90eb56516d3f62102b939932 Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Fri, 23 Dec 2022 01:04:40 +0400
Subject: [PATCH v2] Optimize asserts introduced in 941aa6a6268a6a66f68
Two asserts combined, one unnecessary removed.
Authors: Ted Yu <yuzhihong@gmail.com>, Pavel Borsiov
<pashkin.elfe@gmail.com>
Reviewed-by: Pavel Borisov <pashkin.elfe@gmail.com>
---
src/backend/access/index/indexam.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index dc303995e5b..673395893ba 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -209,8 +209,6 @@ index_beginscan(Relation heapRelation,
{
IndexScanDesc scan;
- Assert(snapshot != InvalidSnapshot);
-
scan = index_beginscan_internal(indexRelation, nkeys, norderbys, snapshot, NULL, false);
/*
@@ -239,8 +237,6 @@ index_beginscan_bitmap(Relation indexRelation,
{
IndexScanDesc scan;
- Assert(snapshot != InvalidSnapshot);
-
scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot, NULL, false);
/*
@@ -265,6 +261,8 @@ index_beginscan_internal(Relation indexRelation,
RELATION_CHECKS;
CHECK_REL_PROCEDURE(ambeginscan);
+ Assert(snapshot != InvalidSnapshot);
+
if (!(indexRelation->rd_indam->ampredlocks))
PredicateLockRelation(indexRelation, snapshot);
@@ -407,8 +405,6 @@ index_parallelscan_estimate(Relation indexRelation, Snapshot snapshot)
{
Size nbytes;
- Assert(snapshot != InvalidSnapshot);
-
RELATION_CHECKS;
nbytes = offsetof(ParallelIndexScanDescData, ps_snapshot_data);
--
2.24.3 (Apple Git-128)