On Fri, 23 Dec 2022 at 00:36, Ted Yu <yuzhih...@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.
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)