Re: BitmapHeapScan streaming read user and prelim refactoring

On Sat, Oct 19, 2024 at 5:48 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> I plan to commit 0002 and 0003 next week. I'm interested if you think
> 0001 is correct.
> I may also commit 0004-0006 as I feel they are ready too.

I noticed an oversight on master, which I think was introduced by the
0005 patch. In ExecReScanBitmapHeapScan:

- if (scan->st.bitmap.rs_shared_iterator)
- {
- tbm_end_shared_iterate(scan->st.bitmap.rs_shared_iterator);
- scan->st.bitmap.rs_shared_iterator = NULL;
- }
-
- if (scan->st.bitmap.rs_iterator)
- {
- tbm_end_private_iterate(scan->st.bitmap.rs_iterator);
- scan->st.bitmap.rs_iterator = NULL;
- }
+ tbm_end_iterate(&scan->st.rs_tbmiterator);

I don't think it's safe to call tbm_end_iterate directly on
scan->st.rs_tbmiterator, as it may have already been cleaned up by
this point.

Here is a query that triggers a SIGSEGV fault on master.

create table t (a int);
insert into t values (1), (2);
create index on t (a);

set enable_seqscan to off;
set enable_indexscan to off;

select * from t t1 left join t t2 on t1.a in (select a from t t3 where
t2.a > 1);

I think we need to check whether rs_tbmiterator is NULL before calling
tbm_end_iterate on it, like below.

--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -572,9 +572,11 @@ ExecReScanBitmapHeapScan(BitmapHeapScanState *node)
if (scan)
{
/*
- * End iteration on iterators saved in scan descriptor.
+ * End iteration on iterators saved in scan descriptor, if they
+ * haven't already been cleaned up.
*/
- tbm_end_iterate(&scan->st.rs_tbmiterator);
+ if (!tbm_exhausted(&scan->st.rs_tbmiterator))
+ tbm_end_iterate(&scan->st.rs_tbmiterator);

/* rescan to release any page pin */
table_rescan(node->ss.ss_currentScanDesc, NULL);

Thanks
Richard