On Wed, Jun 19, 2024 at 2:13 PM Melanie Plageman
<melanieplage...@gmail.com> wrote:
>
> On Wed, Jun 19, 2024 at 12:38 PM Tomas Vondra
> <tomas.von...@enterprisedb.com> wrote:
>
> > > + * XXX I don't understand why we should have this special node if we
> > > + * don't have special nodes for other scan types.
> > >
> > > In this case, up until the final commit (using the read stream
> > > interface), there are six fields required by bitmap heap scan that are
> > > not needed by any other user of HeapScanDescData. There are also
> > > several members of HeapScanDescData that are not needed by bitmap heap
> > > scans and all of the setup in initscan() for those fields is not
> > > required for bitmap heap scans.
> > >
> > > Also, because the BitmapHeapScanDesc needs information like the
> > > ParallelBitmapHeapState and prefetch_maximum (for use in prefetching),
> > > the scan_begin() callback would have to take those as parameters. I
> > > thought adding so much bitmap table scan-specific information to the
> > > generic table scan callbacks was a bad idea.
> > >
> > > Once we add the read stream API code, the number of fields required
> > > for bitmap heap scan that are in the scan descriptor goes down to
> > > three. So, perhaps we could justify having that many bitmap heap
> > > scan-specific fields in the HeapScanDescData.
> > >
> > > Though, I actually think we could start moving toward having
> > > specialized scan descriptors if the requirements for that scan type
> > > are appreciably different. I can't think of new code that would be
> > > added to the HeapScanDescData that would have to be duplicated over to
> > > specialized scan descriptors.
> > >
> > > With the final read stream state, I can see the argument for bloating
> > > the HeapScanDescData with three extra members and avoiding making new
> > > scan descriptors. But, for the intermediate patches which have all of
> > > the bitmap prefetch members pushed down into the HeapScanDescData, I
> > > think it is really not okay. Six members only for bitmap heap scans
> > > and two bitmap-specific members to begin_scan() seems bad.
> > >
> > > What I thought we plan to do is commit the refactoring patches
> > > sometime after the branch for 18 is cut and leave the final read
> > > stream patch uncommitted so we can do performance testing on it. If
> > > you think it is okay to have the six member bloated HeapScanDescData
> > > in master until we push the read stream code, I am okay with removing
> > > the BitmapTableScanDesc and BitmapHeapScanDesc.
> > >
> >
> > I admit I don't have a very good idea what the ideal / desired state
> > look like. My comment is motivated solely by the feeling that it seems
> > strange to have one struct serving most scan types, and then a special
> > struct for one particular scan type ...
>
> I see what you are saying. We could make BitmapTableScanDesc inherit
> from TableScanDescData which would be similar to what we do with other
> things like the executor scan nodes themselves. We would waste space
> and LOC with initializing the unneeded members, but it might seem less
> weird.
>
> Whether we want the specialized scan descriptors at all is probably
> the bigger question, though.
>
> The top level BitmapTableScanDesc is motivated by wanting fewer bitmap
> table scan specific members passed to scan_begin(). And the
> BitmapHeapScanDesc is motivated by this plus wanting to avoid bloating
> the HeapScanDescData.
>
> If you look at at HEAD~1 (with my patches applied) and think you would
> be okay with
> 1) the contents of the BitmapHeapScanDesc being in the HeapScanDescData and
> 2) the extra bitmap table scan-specific parameters in scan_begin_bm()
> being passed to scan_begin()
>
> then I will remove the specialized scan descriptors.
>
> The final state (with the read stream) will still have three bitmap
> heap scan-specific members in the HeapScanDescData.
>
> Would it help if I do a version like this so you can see what it is like?
I revisited this issue (how to keep from bloating the Heap and Table
scan descriptors and adding many parameters to the scan_begin() table
AM callback) and am trying to find a less noisy way to address it than
my previous proposal.
I've attached a prototype of what I think might work applied on top of
master instead of on top of my patchset.
For the top-level TableScanDescData, I suggest we use a union with the
members of each scan type in it in anonymous structs (see 0001). This
will avoid too much bloat because there are other scan types (like TID
Range scans) whose members we can move into the union. It isn't great,
but it avoids a new top-level scan descriptor and changes to the
generic scanstate node.
We will still have to pass the parameters needed to set up the
parallel bitmap iterators to scan_begin() in the intermediate patches,
but if we think that we can actually get the streaming read version of
bitmapheapscan in in the same release, then I think it should be okay
because the final version of these table AM callbacks do not need any
bitmap-specific members.
To address the bloat in the HeapScanDescData, I've kept the
BitmapHeapScanDesc but made it inherit from the HeapScanDescData with
a "suffix" of bitmap scan-specific members which were moved out of the
HeapScanDescData and into the BitmapHeapScanDesc (in 0002).
It's probably better to temporarily increase the parameters to
scan_begin() than to introduce new table AM callbacks and then rip
them out in a later commit.
- Melanie
From 880a70639504cabf00d7d60d665f4025df3d64ef Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 25 Sep 2024 18:39:47 -0400
Subject: [PATCH 2/2] Move BitmapHeapScan-specific members into suffix
Make a new scan descriptor, BitmapHeapScanDesc, which inherits from
HeapScanDescData but has a few extra members that are specific to
BitmapHeapScans. These were in the generic HeapScanDescData but they
don't need to be. This saves space and sets the precedent that future
scan type-specific members shouldn't be put in the generic
HeapScanDescData.
---
src/backend/access/heap/heapam.c | 35 ++++++++++++++++--------
src/backend/access/heap/heapam_handler.c | 12 ++++----
src/include/access/heapam.h | 19 ++++++++-----
3 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f167107257..61cb3350bd 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1036,7 +1036,15 @@ heap_beginscan(Relation relation, Snapshot snapshot,
/*
* allocate and initialize scan descriptor
*/
- scan = (HeapScanDesc) palloc(sizeof(HeapScanDescData));
+ if (flags & SO_TYPE_BITMAPSCAN)
+ {
+ BitmapHeapScanDesc *bscan = palloc(sizeof(BitmapHeapScanDesc));
+ bscan->rs_vmbuffer = InvalidBuffer;
+ bscan->rs_empty_tuples_pending = 0;
+ scan = &bscan->heap;
+ }
+ else
+ scan = palloc(sizeof(HeapScanDescData));
scan->rs_base.rs_rd = relation;
scan->rs_base.rs_snapshot = snapshot;
@@ -1044,8 +1052,6 @@ heap_beginscan(Relation relation, Snapshot snapshot,
scan->rs_base.rs_flags = flags;
scan->rs_base.rs_parallel = parallel_scan;
scan->rs_strategy = NULL; /* set in initscan */
- scan->rs_vmbuffer = InvalidBuffer;
- scan->rs_empty_tuples_pending = 0;
/*
* Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
@@ -1161,18 +1167,20 @@ heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
if (BufferIsValid(scan->rs_cbuf))
ReleaseBuffer(scan->rs_cbuf);
- if (BufferIsValid(scan->rs_vmbuffer))
- {
- ReleaseBuffer(scan->rs_vmbuffer);
- scan->rs_vmbuffer = InvalidBuffer;
- }
-
/*
* Reset rs_empty_tuples_pending, a field only used by bitmap heap scan,
* to avoid incorrectly emitting NULL-filled tuples from a previous scan
* on rescan.
*/
- scan->rs_empty_tuples_pending = 0;
+ if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
+ {
+ ((BitmapHeapScanDesc *) scan)->rs_empty_tuples_pending = 0;
+ if (BufferIsValid(((BitmapHeapScanDesc *) scan)->rs_vmbuffer))
+ {
+ ReleaseBuffer(((BitmapHeapScanDesc *) scan)->rs_vmbuffer);
+ ((BitmapHeapScanDesc *) scan)->rs_vmbuffer = InvalidBuffer;
+ }
+ }
/*
* The read stream is reset on rescan. This must be done before
@@ -1201,8 +1209,11 @@ heap_endscan(TableScanDesc sscan)
if (BufferIsValid(scan->rs_cbuf))
ReleaseBuffer(scan->rs_cbuf);
- if (BufferIsValid(scan->rs_vmbuffer))
- ReleaseBuffer(scan->rs_vmbuffer);
+ if (scan->rs_base.rs_flags & SO_TYPE_BITMAPSCAN)
+ {
+ if (BufferIsValid(((BitmapHeapScanDesc *) scan)->rs_vmbuffer))
+ ReleaseBuffer(((BitmapHeapScanDesc *) scan)->rs_vmbuffer);
+ }
/*
* Must free the read stream before freeing the BufferAccessStrategy.
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 1c6da286d4..6fa1fec8f3 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2119,6 +2119,7 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
TBMIterateResult *tbmres)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
+ BitmapHeapScanDesc *bscan = (BitmapHeapScanDesc *) hscan;
BlockNumber block = tbmres->blockno;
Buffer buffer;
Snapshot snapshot;
@@ -2134,13 +2135,13 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
*/
if (!(scan->rs_flags & SO_NEED_TUPLES) &&
!tbmres->recheck &&
- VM_ALL_VISIBLE(scan->rs_rd, tbmres->blockno, &hscan->rs_vmbuffer))
+ VM_ALL_VISIBLE(scan->rs_rd, tbmres->blockno, &bscan->rs_vmbuffer))
{
/* can't be lossy in the skip_fetch case */
Assert(tbmres->ntuples >= 0);
- Assert(hscan->rs_empty_tuples_pending >= 0);
+ Assert(bscan->rs_empty_tuples_pending >= 0);
- hscan->rs_empty_tuples_pending += tbmres->ntuples;
+ ((BitmapHeapScanDesc *) hscan)->rs_empty_tuples_pending += tbmres->ntuples;
return true;
}
@@ -2253,17 +2254,18 @@ heapam_scan_bitmap_next_tuple(TableScanDesc scan,
TupleTableSlot *slot)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
+ BitmapHeapScanDesc *bscan = (BitmapHeapScanDesc *) hscan;
OffsetNumber targoffset;
Page page;
ItemId lp;
- if (hscan->rs_empty_tuples_pending > 0)
+ if (bscan->rs_empty_tuples_pending > 0)
{
/*
* If we don't have to fetch the tuple, just return nulls.
*/
ExecStoreAllNullTuple(slot);
- hscan->rs_empty_tuples_pending--;
+ bscan->rs_empty_tuples_pending--;
return true;
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b92eb506ec..752bf40c5e 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -92,6 +92,17 @@ typedef struct HeapScanDescData
*/
ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
+
+ /* these fields only used in page-at-a-time mode and for bitmap scans */
+ int rs_cindex; /* current tuple's index in vistuples */
+ int rs_ntuples; /* number of visible tuples on page */
+ OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
+} HeapScanDescData;
+typedef struct HeapScanDescData *HeapScanDesc;
+
+typedef struct BitmapHeapScanDesc {
+ HeapScanDescData heap;
+
/*
* These fields are only used for bitmap scans for the "skip fetch"
* optimization. Bitmap scans needing no fields from the heap may skip
@@ -101,13 +112,7 @@ typedef struct HeapScanDescData
*/
Buffer rs_vmbuffer;
int rs_empty_tuples_pending;
-
- /* these fields only used in page-at-a-time mode and for bitmap scans */
- int rs_cindex; /* current tuple's index in vistuples */
- int rs_ntuples; /* number of visible tuples on page */
- OffsetNumber rs_vistuples[MaxHeapTuplesPerPage]; /* their offsets */
-} HeapScanDescData;
-typedef struct HeapScanDescData *HeapScanDesc;
+} BitmapHeapScanDesc;
/*
* Descriptor for fetches from heap via an index.
--
2.45.2
From ae02bbecfcdae29b80eda106b350d97048bc9dca Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 27 Sep 2024 15:51:07 -0400
Subject: [PATCH 1/2] Example use of union in TableScanDescData
For scan type-specific members of the TableScanDescData, we could store
them like this to reduce overhead.
This is just an example since bitmap table scan on master doesn't yet
need the TBMIterator
---
src/include/access/relscan.h | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 114a85dc47..eacdf44d80 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -16,6 +16,7 @@
#include "access/htup_details.h"
#include "access/itup.h"
+#include "nodes/tidbitmap.h"
#include "port/atomics.h"
#include "storage/buf.h"
#include "storage/relfilelocator.h"
@@ -38,8 +39,14 @@ typedef struct TableScanDescData
struct ScanKeyData *rs_key; /* array of scan key descriptors */
/* Range of ItemPointers for table_scan_getnextslot_tidrange() to scan. */
- ItemPointerData rs_mintid;
- ItemPointerData rs_maxtid;
+ union {
+ struct {
+ ItemPointerData rs_mintid;
+ ItemPointerData rs_maxtid;
+ };
+
+ TBMIterator *iterator;
+ };
/*
* Information about type and behaviour of the scan, a bitmask of members
--
2.45.2
