public inbox for [email protected]
help / color / mirror / Atom feedheapgettup refactoring
6+ messages / 4 participants
[nested] [flat]
* heapgettup refactoring
@ 2022-10-31 18:37 Melanie Plageman <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Melanie Plageman @ 2022-10-31 18:37 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Andres Freund <[email protected]>
Hi,
Attached is a patchset to refactor heapgettup(), heapgettup_pagemode(),
and heapgetpage(). heapgettup() and heapgettup_pagemode() have a lot of
duplicated code, confusingly nested if statements, and unnecessary local
variables. While working on a feature for the AIO/DIO patchset, I
noticed that it was difficult to add new code to heapgettup() and
heapgettup_pagemode() because of how the functions are written.
I've taken a stab at refactoring them -- without generating less
efficient code or causing regressions. I'm interested if people find it
more readable and if those with more assembly expertise see issues (new
branches added which are not highly predictable, etc). I took a look at
the assembly for those symbols compiled at O2 but am not experienced
enough at analysis to come to any conclusions.
- Melanie
Attachments:
[application/octet-stream] v1-0002-Turn-HeapKeyTest-macro-into-function.patch (3.6K, ../../CAAKRu_YSOnhKsDyFcqJsKtBSrd32DP-jjXmv7hL0BPD-z0TGXQ@mail.gmail.com/2-v1-0002-Turn-HeapKeyTest-macro-into-function.patch)
download | inline diff:
From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 31 Oct 2022 13:40:06 -0400
Subject: [PATCH v1 2/3] Turn HeapKeyTest macro into function
This should always be inlined appropriately now. It is easier to read as
a function. Also, remove unused include in catcache.c.
---
src/backend/access/heap/heapam.c | 10 ++--
src/backend/utils/cache/catcache.c | 1 -
src/include/access/valid.h | 76 ++++++++++++------------------
3 files changed, 36 insertions(+), 51 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 12be87efed..1c995faa12 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -716,8 +716,10 @@ heapgettup(HeapScanDesc scan,
snapshot);
if (valid && key != NULL)
- HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key, valid);
+ {
+ valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key);
+ }
if (valid)
{
@@ -1032,8 +1034,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
bool valid;
- HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key, valid);
+ valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key);
if (valid)
{
scan->rs_cindex = lineindex;
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 38e943fab2..30ef0ba39c 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -19,7 +19,6 @@
#include "access/relscan.h"
#include "access/sysattr.h"
#include "access/table.h"
-#include "access/valid.h"
#include "access/xact.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_operator.h"
diff --git a/src/include/access/valid.h b/src/include/access/valid.h
index a5525d0d63..26071f723e 100644
--- a/src/include/access/valid.h
+++ b/src/include/access/valid.h
@@ -19,51 +19,35 @@
*
* Test a heap tuple to see if it satisfies a scan key.
*/
-#define HeapKeyTest(tuple, \
- tupdesc, \
- nkeys, \
- keys, \
- result) \
-do \
-{ \
- /* Use underscores to protect the variables passed in as parameters */ \
- int __cur_nkeys = (nkeys); \
- ScanKey __cur_keys = (keys); \
- \
- (result) = true; /* may change */ \
- for (; __cur_nkeys--; __cur_keys++) \
- { \
- Datum __atp; \
- bool __isnull; \
- Datum __test; \
- \
- if (__cur_keys->sk_flags & SK_ISNULL) \
- { \
- (result) = false; \
- break; \
- } \
- \
- __atp = heap_getattr((tuple), \
- __cur_keys->sk_attno, \
- (tupdesc), \
- &__isnull); \
- \
- if (__isnull) \
- { \
- (result) = false; \
- break; \
- } \
- \
- __test = FunctionCall2Coll(&__cur_keys->sk_func, \
- __cur_keys->sk_collation, \
- __atp, __cur_keys->sk_argument); \
- \
- if (!DatumGetBool(__test)) \
- { \
- (result) = false; \
- break; \
- } \
- } \
-} while (0)
+static inline bool
+HeapKeyTest(HeapTuple tuple, TupleDesc tupdesc, int nkeys, ScanKey keys)
+{
+ int cur_nkeys = nkeys;
+ ScanKey cur_key = keys;
+
+ for (; cur_nkeys--; cur_key++)
+ {
+ Datum atp;
+ bool isnull;
+ Datum test;
+
+ if (cur_key->sk_flags & SK_ISNULL)
+ return false;
+
+ atp = heap_getattr(tuple, cur_key->sk_attno, tupdesc, &isnull);
+
+ if (isnull)
+ return false;
+
+ test = FunctionCall2Coll(&cur_key->sk_func,
+ cur_key->sk_collation,
+ atp, cur_key->sk_argument);
+
+ if (!DatumGetBool(test))
+ return false;
+ }
+
+ return true;
+}
#endif /* VALID_H */
--
2.37.0
[application/octet-stream] v1-0001-Remove-breaks-in-HeapTupleSatisfiesVisibility.patch (1.4K, ../../CAAKRu_YSOnhKsDyFcqJsKtBSrd32DP-jjXmv7hL0BPD-z0TGXQ@mail.gmail.com/3-v1-0001-Remove-breaks-in-HeapTupleSatisfiesVisibility.patch)
download | inline diff:
From cde2d6720f4f5ab2531c22ad4a5f0d9e6ec1039d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 26 Oct 2022 20:00:34 -0400
Subject: [PATCH v1 1/3] Remove breaks in HeapTupleSatisfiesVisibility
breaks in HeapTupleSatisfiesVisibility were superfluous
---
src/backend/access/heap/heapam_visibility.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 6e33d1c881..dd5d5da190 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1769,25 +1769,18 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
{
case SNAPSHOT_MVCC:
return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
- break;
case SNAPSHOT_SELF:
return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
- break;
case SNAPSHOT_ANY:
return HeapTupleSatisfiesAny(htup, snapshot, buffer);
- break;
case SNAPSHOT_TOAST:
return HeapTupleSatisfiesToast(htup, snapshot, buffer);
- break;
case SNAPSHOT_DIRTY:
return HeapTupleSatisfiesDirty(htup, snapshot, buffer);
- break;
case SNAPSHOT_HISTORIC_MVCC:
return HeapTupleSatisfiesHistoricMVCC(htup, snapshot, buffer);
- break;
case SNAPSHOT_NON_VACUUMABLE:
return HeapTupleSatisfiesNonVacuumable(htup, snapshot, buffer);
- break;
}
return false; /* keep compiler quiet */
--
2.37.0
[application/octet-stream] v1-0003-Refactor-heapgettup-and-heapgetpage.patch (28.6K, ../../CAAKRu_YSOnhKsDyFcqJsKtBSrd32DP-jjXmv7hL0BPD-z0TGXQ@mail.gmail.com/4-v1-0003-Refactor-heapgettup-and-heapgetpage.patch)
download | inline diff:
From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 31 Oct 2022 13:40:29 -0400
Subject: [PATCH v1 3/3] Refactor heapgettup* and heapgetpage
Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
three contained several unnecessary local variables, duplicate code, and
nested if statements. Streamlining these improves readability and
extensibility.
---
src/backend/access/heap/heapam.c | 874 ++++++++++++-------------------
1 file changed, 342 insertions(+), 532 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 1c995faa12..aee4dd3a41 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -379,7 +379,6 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
int lines;
int ntup;
OffsetNumber lineoff;
- ItemId lpp;
bool all_visible;
Assert(page < scan->rs_nblocks);
@@ -448,31 +447,31 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
*/
all_visible = PageIsAllVisible(dp) && !snapshot->takenDuringRecovery;
- for (lineoff = FirstOffsetNumber, lpp = PageGetItemId(dp, lineoff);
- lineoff <= lines;
- lineoff++, lpp++)
+ for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
{
- if (ItemIdIsNormal(lpp))
- {
- HeapTupleData loctup;
- bool valid;
+ HeapTupleData loctup;
+ bool valid;
- loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
- loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- loctup.t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(loctup.t_self), page, lineoff);
+ ItemId lpp = PageGetItemId(dp, lineoff);
- if (all_visible)
- valid = true;
- else
- valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- &loctup, buffer, snapshot);
+ loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
+ loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ loctup.t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(loctup.t_self), page, lineoff);
- if (valid)
- scan->rs_vistuples[ntup++] = lineoff;
- }
+ if (all_visible)
+ valid = true;
+ else
+ valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+
+ HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+ &loctup, buffer, snapshot);
+
+ if (valid)
+ scan->rs_vistuples[ntup++] = lineoff;
}
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
@@ -481,6 +480,207 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
scan->rs_ntuples = ntup;
}
+
+static inline void heapgettup_no_movement(HeapScanDesc scan)
+{
+ ItemId lpp;
+ OffsetNumber lineoff;
+ BlockNumber page;
+ Page dp;
+ HeapTuple tuple = &(scan->rs_ctup);
+ /*
+ * ``no movement'' scan direction: refetch prior tuple
+ */
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ if (!scan->rs_inited)
+ {
+ Assert(!BufferIsValid(scan->rs_cbuf));
+ tuple->t_data = NULL;
+ return;
+ }
+
+ page = ItemPointerGetBlockNumber(&(tuple->t_self));
+ if (page != scan->rs_cblock)
+ heapgetpage((TableScanDesc) scan, page);
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+ lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
+ lpp = PageGetItemId(dp, lineoff);
+ Assert(ItemIdIsNormal(lpp));
+
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+
+ /* check that rs_cindex is in sync if in pagemode */
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (scan->rs_cindex < scan->rs_ntuples));
+
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (lineoff == scan->rs_vistuples[scan->rs_cindex]));
+
+ return;
+}
+
+static inline Page heapgettup_continue_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir,
+ int *linesleft, OffsetNumber *lineoff)
+{
+ HeapTuple tuple = &(scan->rs_ctup);
+ Page dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+
+ if (ScanDirectionIsForward(dir))
+ {
+ *lineoff = OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
+ *linesleft = PageGetMaxOffsetNumber(dp) - (*lineoff) + 1;
+ }
+ else
+ {
+ /*
+ * The previous returned tuple may have been vacuumed since the
+ * previous scan when we use a non-MVCC snapshot, so we must
+ * re-establish the lineoff <= PageGetMaxOffsetNumber(dp)
+ * invariant
+ */
+ *lineoff = Min(PageGetMaxOffsetNumber(dp),
+ OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));
+ *linesleft = *lineoff;
+ }
+ /* page and lineoff now reference the physically next tid */
+ return dp;
+}
+
+static inline Page heapgettup_start_page(HeapScanDesc scan,
+ BlockNumber page, ScanDirection dir, int *linesleft, OffsetNumber *lineoff)
+{
+ Page dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+
+ *linesleft = PageGetMaxOffsetNumber((Page) dp) - FirstOffsetNumber + 1;
+
+ if (ScanDirectionIsForward(dir))
+ *lineoff = FirstOffsetNumber;
+ else
+ *lineoff = (OffsetNumber) (*linesleft);
+
+ return dp;
+}
+
+static inline BlockNumber heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
+{
+ Assert(!ScanDirectionIsNoMovement(dir));
+ Assert(!scan->rs_inited);
+
+ /* forward and serial */
+ if (ScanDirectionIsForward(dir) && scan->rs_base.rs_parallel == NULL)
+ return scan->rs_startblock;
+
+ /* forward and parallel */
+ if (ScanDirectionIsForward(dir))
+ {
+ table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+ return table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+ }
+
+ /* backward parallel scan not supported */
+ Assert(scan->rs_base.rs_parallel == NULL);
+
+ /*
+ * Disable reporting to syncscan logic in a backwards scan; it's
+ * not very likely anyone else is doing the same thing at the same
+ * time, and much more likely that we'll just bollix things for
+ * forward scanners.
+ */
+ scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
+
+ /*
+ * Start from last page of the scan. Ensure we take into account
+ * rs_numblocks if it's been adjusted by heap_setscanlimits().
+ */
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ return (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
+
+ if (scan->rs_startblock > 0)
+ return scan->rs_startblock - 1;
+
+ return scan->rs_nblocks - 1;
+}
+
+static inline BlockNumber heapgettup_advance_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir)
+{
+ if (ScanDirectionIsBackward(dir))
+ {
+ if (page == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ if (page == 0)
+ page = scan->rs_nblocks;
+
+ page--;
+
+ return page;
+ }
+ else if (scan->rs_base.rs_parallel != NULL)
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+ return page;
+ }
+ else
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ page++;
+
+ if (page >= scan->rs_nblocks)
+ page = 0;
+
+ if (page == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ /*
+ * Report our new scan position for synchronization purposes. We
+ * don't do that when moving backwards, however. That would just
+ * mess up any other forward-moving scanners.
+ *
+ * Note: we do this before checking for end of scan so that the
+ * final state of the position hint is back at the start of the
+ * rel. That's not strictly necessary, but otherwise when you run
+ * the same query multiple times the starting position would shift
+ * a little bit backwards on every invocation, which is confusing.
+ * We don't guarantee any specific ordering in general, though.
+ */
+ if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
+ ss_report_location(scan->rs_base.rs_rd, page);
+
+ return page;
+ }
+}
+
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -511,182 +711,55 @@ heapgettup(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- Snapshot snapshot = scan->rs_base.rs_snapshot;
- bool backward = ScanDirectionIsBackward(dir);
BlockNumber page;
- bool finished;
Page dp;
- int lines;
OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
+
+ if (ScanDirectionIsNoMovement(dir))
+ return heapgettup_no_movement(scan);
/*
- * calculate next starting lineoff, given scan direction
+ * return null immediately if relation is empty
*/
- if (ScanDirectionIsForward(dir))
+ if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
{
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- /* Other processes might have already finished the scan. */
- if (page == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- page = scan->rs_startblock; /* first page */
- heapgetpage((TableScanDesc) scan, page);
- lineoff = FirstOffsetNumber; /* first offnum */
- scan->rs_inited = true;
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- lineoff = /* next offnum */
- OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
- }
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber(dp);
- /* page and lineoff now reference the physically next tid */
-
- linesleft = lines - lineoff + 1;
+ Assert(!BufferIsValid(scan->rs_cbuf));
+ tuple->t_data = NULL;
+ return;
}
- else if (backward)
- {
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- page = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- page = scan->rs_startblock - 1;
- else
- page = scan->rs_nblocks - 1;
- heapgetpage((TableScanDesc) scan, page);
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- }
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber(dp);
-
- if (!scan->rs_inited)
- {
- lineoff = lines; /* final offnum */
- scan->rs_inited = true;
- }
- else
- {
- /*
- * The previous returned tuple may have been vacuumed since the
- * previous scan when we use a non-MVCC snapshot, so we must
- * re-establish the lineoff <= PageGetMaxOffsetNumber(dp)
- * invariant
- */
- lineoff = /* previous offnum */
- Min(lines,
- OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));
- }
- /* page and lineoff now reference the physically previous tid */
+ if (!scan->rs_inited)
+ {
+ page = heapgettup_initial_page(scan, dir);
+ scan->rs_inited = true;
- linesleft = lineoff;
+ /*
+ * If parallel and other processes have already finished the scan, the
+ * returned page is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
+ */
+ Assert(page != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
- /*
- * ``no movement'' scan direction: refetch prior tuple
- */
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- page = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (page != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, page);
-
- /* Since the tuple was previously fetched, needn't lock page here */
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(dp, lineoff);
- Assert(ItemIdIsNormal(lpp));
-
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
-
- return;
+ page = scan->rs_cblock;
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ dp = heapgettup_continue_page(scan, page, dir, &linesleft, &lineoff);
+ goto continue_page;
}
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- lpp = PageGetItemId(dp, lineoff);
- for (;;)
+ while (page != InvalidBlockNumber)
{
+ heapgetpage((TableScanDesc) scan, page);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ dp = heapgettup_start_page(scan, page, dir, &linesleft, &lineoff);
+ continue_page:
+
/*
* Only continue scanning the page while we have lines left.
*
@@ -694,54 +767,39 @@ heapgettup(HeapScanDesc scan,
* PageGetMaxOffsetNumber(); both for forward scans when we resume the
* table scan, and for when we start scanning a new page.
*/
- while (linesleft > 0)
+ for (; linesleft > 0; linesleft--, lineoff += dir)
{
- if (ItemIdIsNormal(lpp))
- {
- bool valid;
-
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(tuple->t_self), page, lineoff);
+ bool visible;
+ ItemId lpp = PageGetItemId(dp, lineoff);
- /*
- * if current tuple qualifies, return it.
- */
- valid = HeapTupleSatisfiesVisibility(tuple,
- snapshot,
- scan->rs_cbuf);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- tuple, scan->rs_cbuf,
- snapshot);
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(tuple->t_self), scan->rs_cblock, lineoff);
- if (valid && key != NULL)
- {
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
- }
+ /*
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the page
+ */
+ visible = HeapTupleSatisfiesVisibility(tuple,
+ scan->rs_base.rs_snapshot,
+ scan->rs_cbuf);
+
+ HeapCheckForSerializableConflictOut(visible, scan->rs_base.rs_rd,
+ tuple, scan->rs_cbuf,
+ scan->rs_base.rs_snapshot);
+
+ if (!visible)
+ continue;
- if (valid)
- {
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- return;
- }
- }
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- {
- --lpp; /* move back in this page's ItemId array */
- --lineoff;
- }
- else
- {
- ++lpp; /* move forward in this page's ItemId array */
- ++lineoff;
- }
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ return;
}
/*
@@ -750,85 +808,17 @@ heapgettup(HeapScanDesc scan,
*/
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- /*
- * advance to next/prior page and detect end of scan
- */
- if (backward)
- {
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (page == 0)
- page = scan->rs_nblocks;
- page--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (page == InvalidBlockNumber);
- }
- else
- {
- page++;
- if (page >= scan->rs_nblocks)
- page = 0;
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, page);
- }
-
- /*
- * return NULL if we've exhausted all the pages
- */
- if (finished)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, page);
+ page = heapgettup_advance_page(scan, page, dir);
+ }
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber((Page) dp);
- linesleft = lines;
- if (backward)
- {
- lineoff = lines;
- lpp = PageGetItemId(dp, lines);
- }
- else
- {
- lineoff = FirstOffsetNumber;
- lpp = PageGetItemId(dp, FirstOffsetNumber);
- }
- }
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
/* ----------------
@@ -851,174 +841,71 @@ heapgettup_pagemode(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- bool backward = ScanDirectionIsBackward(dir);
BlockNumber page;
- bool finished;
Page dp;
- int lines;
int lineindex;
- OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
+
+ if (ScanDirectionIsNoMovement(dir))
+ return heapgettup_no_movement(scan);
/*
- * calculate next starting lineindex, given scan direction
+ * return null immediately if relation is empty
*/
- if (ScanDirectionIsForward(dir))
+ if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
{
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- /* Other processes might have already finished the scan. */
- if (page == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- page = scan->rs_startblock; /* first page */
- heapgetpage((TableScanDesc) scan, page);
- lineindex = 0;
- scan->rs_inited = true;
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- lineindex = scan->rs_cindex + 1;
- }
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
- /* page and lineindex now reference the next visible tid */
-
- linesleft = lines - lineindex;
+ Assert(!BufferIsValid(scan->rs_cbuf));
+ tuple->t_data = NULL;
+ return;
}
- else if (backward)
- {
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- page = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- page = scan->rs_startblock - 1;
- else
- page = scan->rs_nblocks - 1;
- heapgetpage((TableScanDesc) scan, page);
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- }
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
-
- if (!scan->rs_inited)
- {
- lineindex = lines - 1;
- scan->rs_inited = true;
- }
- else
- {
- lineindex = scan->rs_cindex - 1;
- }
- /* page and lineindex now reference the previous visible tid */
-
- linesleft = lineindex + 1;
- }
- else
+ if (!scan->rs_inited)
{
+ page = heapgettup_initial_page(scan, dir);
+ scan->rs_inited = true;
/*
- * ``no movement'' scan direction: refetch prior tuple
+ * If parallel and other processes have already finished the scan, the
+ * returned page is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
*/
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- page = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (page != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, page);
-
- /* Since the tuple was previously fetched, needn't lock page here */
+ Assert(page != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
+ }
+ else
+ {
+ page = scan->rs_cblock;
dp = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(dp, lineoff);
- Assert(ItemIdIsNormal(lpp));
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
-
- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
+ lineindex = scan->rs_cindex + dir;
+ if (ScanDirectionIsForward(dir))
+ linesleft = scan->rs_ntuples - lineindex;
+ else
+ linesleft = scan->rs_cindex;
- return;
+ /* page and lineindex now reference the next visible tid */
+ goto continue_page;
}
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- for (;;)
+ while (page != InvalidBlockNumber)
{
- while (linesleft > 0)
+ heapgetpage((TableScanDesc) scan, page);
+ dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+ linesleft = scan->rs_ntuples;
+ lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+ /* page and lineindex now reference the previous visible tid */
+
+ continue_page:
+
+ for (; linesleft > 0; linesleft--, lineindex += dir)
{
+ ItemId lpp;
+ OffsetNumber lineoff;
+
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(dp, lineoff);
Assert(ItemIdIsNormal(lpp));
@@ -1028,108 +915,31 @@ heapgettup_pagemode(HeapScanDesc scan,
ItemPointerSet(&(tuple->t_self), page, lineoff);
/*
- * if current tuple qualifies, return it.
- */
- if (key != NULL)
- {
- bool valid;
-
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
- if (valid)
- {
- scan->rs_cindex = lineindex;
- return;
- }
- }
- else
- {
- scan->rs_cindex = lineindex;
- return;
- }
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the page
+ */
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- --lineindex;
- else
- ++lineindex;
+ scan->rs_cindex = lineindex;
+ return;
}
/*
* if we get here, it means we've exhausted the items on this page and
* it's time to move to the next.
*/
- if (backward)
- {
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (page == 0)
- page = scan->rs_nblocks;
- page--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (page == InvalidBlockNumber);
- }
- else
- {
- page++;
- if (page >= scan->rs_nblocks)
- page = 0;
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, page);
- }
-
- /*
- * return NULL if we've exhausted all the pages
- */
- if (finished)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, page);
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
- linesleft = lines;
- if (backward)
- lineindex = lines - 1;
- else
- lineindex = 0;
+ page = heapgettup_advance_page(scan, page, dir);
}
+
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
--
2.37.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: heapgettup refactoring
@ 2022-10-31 22:37 Justin Pryzby <[email protected]>
parent: Melanie Plageman <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Justin Pryzby @ 2022-10-31 22:37 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers; Andres Freund <[email protected]>
FYI:
[18:51:54.707] ../src/backend/access/heap/heapam.c(720): warning C4098: 'heapgettup': 'void' function returning a value
[18:51:54.707] ../src/backend/access/heap/heapam.c(850): warning C4098: 'heapgettup_pagemode': 'void' function returning a value
For some reason, MSVC is the only one to complain, and cfbot doesn't
currently tell you about it. I have a patch to show that, which I'll
send $later.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: heapgettup refactoring
@ 2022-11-01 01:09 Andres Freund <[email protected]>
parent: Melanie Plageman <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Andres Freund @ 2022-11-01 01:09 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: pgsql-hackers
Hi,
On 2022-10-31 14:37:39 -0400, Melanie Plageman wrote:
> and heapgetpage(). heapgettup() and heapgettup_pagemode() have a lot of
> duplicated code, confusingly nested if statements, and unnecessary local
> variables. While working on a feature for the AIO/DIO patchset, I
> noticed that it was difficult to add new code to heapgettup() and
> heapgettup_pagemode() because of how the functions are written.
Thanks for working on this - the current state is quite painful.
> From cde2d6720f4f5ab2531c22ad4a5f0d9e6ec1039d Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email protected]>
> Date: Wed, 26 Oct 2022 20:00:34 -0400
> Subject: [PATCH v1 1/3] Remove breaks in HeapTupleSatisfiesVisibility
>
> breaks in HeapTupleSatisfiesVisibility were superfluous
> ---
> src/backend/access/heap/heapam_visibility.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
> index 6e33d1c881..dd5d5da190 100644
> --- a/src/backend/access/heap/heapam_visibility.c
> +++ b/src/backend/access/heap/heapam_visibility.c
> @@ -1769,25 +1769,18 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
> {
> case SNAPSHOT_MVCC:
> return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_SELF:
> return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_ANY:
> return HeapTupleSatisfiesAny(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_TOAST:
> return HeapTupleSatisfiesToast(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_DIRTY:
> return HeapTupleSatisfiesDirty(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_HISTORIC_MVCC:
> return HeapTupleSatisfiesHistoricMVCC(htup, snapshot, buffer);
> - break;
> case SNAPSHOT_NON_VACUUMABLE:
> return HeapTupleSatisfiesNonVacuumable(htup, snapshot, buffer);
> - break;
> }
Not sure what the author of this code, a certain Mr Freund, was thinking when
he added those returns...
> From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email protected]>
> Date: Mon, 31 Oct 2022 13:40:06 -0400
> Subject: [PATCH v1 2/3] Turn HeapKeyTest macro into function
>
> This should always be inlined appropriately now. It is easier to read as
> a function. Also, remove unused include in catcache.c.
> ---
> src/backend/access/heap/heapam.c | 10 ++--
> src/backend/utils/cache/catcache.c | 1 -
> src/include/access/valid.h | 76 ++++++++++++------------------
> 3 files changed, 36 insertions(+), 51 deletions(-)
>
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 12be87efed..1c995faa12 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -716,8 +716,10 @@ heapgettup(HeapScanDesc scan,
> snapshot);
>
> if (valid && key != NULL)
> - HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> - nkeys, key, valid);
> + {
> + valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> + nkeys, key);
> + }
>
> if (valid)
> {
superfluous parens.
> --- a/src/include/access/valid.h
> +++ b/src/include/access/valid.h
> @@ -19,51 +19,35 @@
> *
> * Test a heap tuple to see if it satisfies a scan key.
> */
> -#define HeapKeyTest(tuple, \
> - tupdesc, \
> - nkeys, \
> - keys, \
> - result) \
> -do \
> -{ \
> - /* Use underscores to protect the variables passed in as parameters */ \
> - int __cur_nkeys = (nkeys); \
> - ScanKey __cur_keys = (keys); \
> - \
> - (result) = true; /* may change */ \
> - for (; __cur_nkeys--; __cur_keys++) \
> - { \
> - Datum __atp; \
> - bool __isnull; \
> - Datum __test; \
> - \
> - if (__cur_keys->sk_flags & SK_ISNULL) \
> - { \
> - (result) = false; \
> - break; \
> - } \
> - \
> - __atp = heap_getattr((tuple), \
> - __cur_keys->sk_attno, \
> - (tupdesc), \
> - &__isnull); \
> - \
> - if (__isnull) \
> - { \
> - (result) = false; \
> - break; \
> - } \
> - \
> - __test = FunctionCall2Coll(&__cur_keys->sk_func, \
> - __cur_keys->sk_collation, \
> - __atp, __cur_keys->sk_argument); \
> - \
> - if (!DatumGetBool(__test)) \
> - { \
> - (result) = false; \
> - break; \
> - } \
> - } \
> -} while (0)
> +static inline bool
> +HeapKeyTest(HeapTuple tuple, TupleDesc tupdesc, int nkeys, ScanKey keys)
> +{
> + int cur_nkeys = nkeys;
> + ScanKey cur_key = keys;
> +
> + for (; cur_nkeys--; cur_key++)
> + {
> + Datum atp;
> + bool isnull;
> + Datum test;
> +
> + if (cur_key->sk_flags & SK_ISNULL)
> + return false;
> +
> + atp = heap_getattr(tuple, cur_key->sk_attno, tupdesc, &isnull);
> +
> + if (isnull)
> + return false;
> +
> + test = FunctionCall2Coll(&cur_key->sk_func,
> + cur_key->sk_collation,
> + atp, cur_key->sk_argument);
> +
> + if (!DatumGetBool(test))
> + return false;
> + }
> +
> + return true;
> +}
Seems like a simple and nice win in readability.
I recall looking at this in the past and thinking that there was some
additional subtlety here, but I can't see what that'd be.
> From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <[email protected]>
> Date: Mon, 31 Oct 2022 13:40:29 -0400
> Subject: [PATCH v1 3/3] Refactor heapgettup* and heapgetpage
>
> Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
> three contained several unnecessary local variables, duplicate code, and
> nested if statements. Streamlining these improves readability and
> extensibility.
It'd be nice to break this into slightly smaller chunks.
> +
> +static inline void heapgettup_no_movement(HeapScanDesc scan)
> +{
FWIW, for function definitions we keep the return type (and with that also the
the "static inline") on a separate line.
> + ItemId lpp;
> + OffsetNumber lineoff;
> + BlockNumber page;
> + Page dp;
> + HeapTuple tuple = &(scan->rs_ctup);
> + /*
> + * ``no movement'' scan direction: refetch prior tuple
> + */
> +
> + /* Since the tuple was previously fetched, needn't lock page here */
> + if (!scan->rs_inited)
> + {
> + Assert(!BufferIsValid(scan->rs_cbuf));
> + tuple->t_data = NULL;
> + return;
Is it possible to have a no-movement scan with an uninitialized scan? That
doesn't really seem to make sense. At least that's how I understand the
explanation for NoMovement nearby:
* dir == NoMovementScanDirection means "re-fetch the tuple indicated
* by scan->rs_ctup".
We can't have a rs_ctup without an already started scan, no?
Looks like this is pre-existing code that you just moved, but it still seems
wrong.
> + }
> + page = ItemPointerGetBlockNumber(&(tuple->t_self));
> + if (page != scan->rs_cblock)
> + heapgetpage((TableScanDesc) scan, page);
We have a
BlockNumber page;
and
Page dp;
in this code which seems almost intentionally confusing. This again is a
pre-existing issue but perhaps we could clean it up first?
> +static inline Page heapgettup_continue_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir,
> + int *linesleft, OffsetNumber *lineoff)
> +{
> + HeapTuple tuple = &(scan->rs_ctup);
Hm. Finding the next offset via rs_ctup doesn't seem quite right. For one,
it's not actually that cheap to extract the offset from an ItemPointer because
of the the way we pack it into ItemPointerData.
> + Page dp = BufferGetPage(scan->rs_cbuf);
> + TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
Newlines between definitions and code :)
Perhaps worth asserting that the scan is initialized and that rs_cbuf is valid?
> + if (ScanDirectionIsForward(dir))
> + {
> + *lineoff = OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
> + *linesleft = PageGetMaxOffsetNumber(dp) - (*lineoff) + 1;
We can't access PageGetMaxOffsetNumber etc without holding a lock on the
page. It's not immediately obvious that that is held in all paths.
> +static inline BlockNumber heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
> +{
> + Assert(!ScanDirectionIsNoMovement(dir));
> + Assert(!scan->rs_inited);
Is there a reason we couldn't set rs_inited in here, rather than reapeating
that in all callers?
ISTM that this function should deal with the
/*
* return null immediately if relation is empty
*/
logic, I think you now are repeating that check on every call to heapgettup().
> @@ -511,182 +711,55 @@ heapgettup(HeapScanDesc scan,
> ScanKey key)
> {
> HeapTuple tuple = &(scan->rs_ctup);
> - Snapshot snapshot = scan->rs_base.rs_snapshot;
> - bool backward = ScanDirectionIsBackward(dir);
> BlockNumber page;
> - bool finished;
> Page dp;
> - int lines;
> OffsetNumber lineoff;
> int linesleft;
> - ItemId lpp;
> +
> + if (ScanDirectionIsNoMovement(dir))
> + return heapgettup_no_movement(scan);
Maybe add an unlikely() - this path is barely ever used...
> /*
> - * calculate next starting lineoff, given scan direction
> + * return null immediately if relation is empty
> */
> - if (ScanDirectionIsForward(dir))
> + if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
> {
As mentioned above, I don't think we should repeat the nblocks check on every
call.
> + page = scan->rs_cblock;
> + LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> + dp = heapgettup_continue_page(scan, page, dir, &linesleft, &lineoff);
> + goto continue_page;
> }
>
> /*
> * advance the scan until we find a qualifying tuple or run out of stuff
> * to scan
> */
> - lpp = PageGetItemId(dp, lineoff);
> - for (;;)
> + while (page != InvalidBlockNumber)
> {
> + heapgetpage((TableScanDesc) scan, page);
> + LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> + dp = heapgettup_start_page(scan, page, dir, &linesleft, &lineoff);
> + continue_page:
I don't like the goto continue_page at all. Seems that the paths leading here
should call LockBuffer(), heapgettup_start_page() etc? Possibly a do {} while
() loop could do the trick as well.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: heapgettup refactoring
@ 2022-11-04 15:51 Melanie Plageman <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Melanie Plageman @ 2022-11-04 15:51 UTC (permalink / raw)
To: Andres Freund <[email protected]>; pgsql-hackers
Thanks for the review!
Attached is v2 with feedback addressed.
On Mon, Oct 31, 2022 at 9:09 PM Andres Freund <[email protected]> wrote:
> > From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman <[email protected]>
> > Date: Mon, 31 Oct 2022 13:40:06 -0400
> > Subject: [PATCH v1 2/3] Turn HeapKeyTest macro into function
> >
> > This should always be inlined appropriately now. It is easier to read as
> > a function. Also, remove unused include in catcache.c.
> > ---
> > src/backend/access/heap/heapam.c | 10 ++--
> > src/backend/utils/cache/catcache.c | 1 -
> > src/include/access/valid.h | 76 ++++++++++++------------------
> > 3 files changed, 36 insertions(+), 51 deletions(-)
> >
> > diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> > index 12be87efed..1c995faa12 100644
> > --- a/src/backend/access/heap/heapam.c
> > +++ b/src/backend/access/heap/heapam.c
> > @@ -716,8 +716,10 @@ heapgettup(HeapScanDesc scan,
> > snapshot);
> >
> > if (valid && key != NULL)
> > - HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> > - nkeys, key, valid);
> > + {
> > + valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
> > + nkeys, key);
> > + }
> >
> > if (valid)
> > {
>
> superfluous parens.
fixed.
> > From a894ce38c488df6546392b9f3bd894b67edf951e Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman <[email protected]>
> > Date: Mon, 31 Oct 2022 13:40:29 -0400
> > Subject: [PATCH v1 3/3] Refactor heapgettup* and heapgetpage
> >
> > Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
> > three contained several unnecessary local variables, duplicate code, and
> > nested if statements. Streamlining these improves readability and
> > extensibility.
>
> It'd be nice to break this into slightly smaller chunks.
I can do that. Since incorporating feedback will be harder once I break
it up into smaller chunks, I'm inclined to wait to do so until I know
that the structure I have now is the one we will go with. (I know smaller
chunks will make it more reviewable.)
> > +
> > +static inline void heapgettup_no_movement(HeapScanDesc scan)
> > +{
>
> FWIW, for function definitions we keep the return type (and with that also the
> the "static inline") on a separate line.
Fixed
>
> > + ItemId lpp;
> > + OffsetNumber lineoff;
> > + BlockNumber page;
> > + Page dp;
> > + HeapTuple tuple = &(scan->rs_ctup);
> > + /*
> > + * ``no movement'' scan direction: refetch prior tuple
> > + */
> > +
> > + /* Since the tuple was previously fetched, needn't lock page here */
> > + if (!scan->rs_inited)
> > + {
> > + Assert(!BufferIsValid(scan->rs_cbuf));
> > + tuple->t_data = NULL;
> > + return;
>
> Is it possible to have a no-movement scan with an uninitialized scan? That
> doesn't really seem to make sense. At least that's how I understand the
> explanation for NoMovement nearby:
> * dir == NoMovementScanDirection means "re-fetch the tuple indicated
> * by scan->rs_ctup".
>
> We can't have a rs_ctup without an already started scan, no?
>
> Looks like this is pre-existing code that you just moved, but it still seems
> wrong.
Changed to an assert
>
> > + }
> > + page = ItemPointerGetBlockNumber(&(tuple->t_self));
> > + if (page != scan->rs_cblock)
> > + heapgetpage((TableScanDesc) scan, page);
>
>
> We have a
> BlockNumber page;
> and
> Page dp;
> in this code which seems almost intentionally confusing. This again is a
> pre-existing issue but perhaps we could clean it up first?
in attached
page -> block
dp -> page
in basically all locations in heapam.c (should that be its own commit?)
> > +static inline Page heapgettup_continue_page(HeapScanDesc scan, BlockNumber page, ScanDirection dir,
> > + int *linesleft, OffsetNumber *lineoff)
> > +{
> > + HeapTuple tuple = &(scan->rs_ctup);
>
> Hm. Finding the next offset via rs_ctup doesn't seem quite right. For one,
> it's not actually that cheap to extract the offset from an ItemPointer because
> of the the way we pack it into ItemPointerData.
So, it was like this before [1].
What about saving the lineoff in rs_cindex.
It is smaller, but that seems okay, right?
> > + Page dp = BufferGetPage(scan->rs_cbuf);
> > + TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
>
> Newlines between definitions and code :)
k
> Perhaps worth asserting that the scan is initialized and that rs_cbuf is valid?
indeed.
>
> > + if (ScanDirectionIsForward(dir))
> > + {
> > + *lineoff = OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
> > + *linesleft = PageGetMaxOffsetNumber(dp) - (*lineoff) + 1;
>
> We can't access PageGetMaxOffsetNumber etc without holding a lock on the
> page. It's not immediately obvious that that is held in all paths.
In heapgettup() I call LockBuffer() before invoking
heapgettup_continue_page() and heapgettup_start_page() which are the
ones doing this.
I did have big plans for using the continue_page and start_page
functions in heapgettup_pagemode() as well, but since I'm not doing that
now, I can add in an expectation that the lock is held.
I added a comment saying the caller is responsible for acquiring the
lock if needed. I thought of adding an assert, but I don't see that
being done outside of bufmgr.c
BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1);
Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
> > +static inline BlockNumber heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
> > +{
> > + Assert(!ScanDirectionIsNoMovement(dir));
> > + Assert(!scan->rs_inited);
>
> Is there a reason we couldn't set rs_inited in here, rather than reapeating
> that in all callers?
I wasn't sure if future callers or existing callers in the future may
need to do steps other than what is in heapgettup_initial_page() before
setting rs_inited. I thought of the responsibility of
heapgettup_initial_page() as returning the initial page to start the
scan. If it is going to do all initialization steps, perhaps the name
should change? I thought having a function that says it does
initialization of the scan might be confusing since initscan() also
exists.
> ISTM that this function should deal with the
> /*
> * return null immediately if relation is empty
> */
>
> logic, I think you now are repeating that check on every call to heapgettup().
So, that's a good point. If I move setting rs_inited inside of
heapgettup_initial_page(), then I can also easily move the empty table
check inside there too.
I don't want to set rs_inited before every return in
heapgettup_initial_page(). Do you think there are any issues with
setting it at the top of the function?
I thought about setting it at the very top (even before checking if the
relation is empty) Is it okay to set it before the empty table check?
rs_inited will be set to false at the bottom before returning. But,
maybe this will be an issue in other callers of
heapgettup_initial_page()?
Anyway, I have changed it in attached v2.
> > @@ -511,182 +711,55 @@ heapgettup(HeapScanDesc scan,
> > ScanKey key)
> > {
> > HeapTuple tuple = &(scan->rs_ctup);
> > - Snapshot snapshot = scan->rs_base.rs_snapshot;
> > - bool backward = ScanDirectionIsBackward(dir);
> > BlockNumber page;
> > - bool finished;
> > Page dp;
> > - int lines;
> > OffsetNumber lineoff;
> > int linesleft;
> > - ItemId lpp;
> > +
> > + if (ScanDirectionIsNoMovement(dir))
> > + return heapgettup_no_movement(scan);
>
> Maybe add an unlikely() - this path is barely ever used...
done.
> > + page = scan->rs_cblock;
> > + LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> > + dp = heapgettup_continue_page(scan, page, dir, &linesleft, &lineoff);
> > + goto continue_page;
> > }
> >
> > /*
> > * advance the scan until we find a qualifying tuple or run out of stuff
> > * to scan
> > */
> > - lpp = PageGetItemId(dp, lineoff);
> > - for (;;)
> > + while (page != InvalidBlockNumber)
> > {
> > + heapgetpage((TableScanDesc) scan, page);
> > + LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
> > + dp = heapgettup_start_page(scan, page, dir, &linesleft, &lineoff);
> > + continue_page:
>
>
> I don't like the goto continue_page at all. Seems that the paths leading here
> should call LockBuffer(), heapgettup_start_page() etc? Possibly a do {} while
> () loop could do the trick as well.
I don't see how a do while loop would solve help with the problem.
We need to check if the block number is valid after getting a block
assignment before doing heapgetpage() (e.g. after
heapgettup_initial_page() and after heapgettup_advance_page()).
Removing the goto continue_page means adding the heapgettpage(),
heapgettup_start_page(), etc code block in two places now (both after
heapgettup_initial_page() and after heapgettup_advance_page()) and, in
both locations we have to add an if statement to check if the block is
valid. I feel like this makes the function longer and harder to
understand. Keeping the loop as short as possible makes it clear what it
is doing. I think that with an appropriate warning comment, the goto
continue_page is clearer and easier to understand. To me, starting a
page at the top of the outer loop, then looping through the lines in the
page and is the structure that makes the most sense.
- Melanie
[1] https://github.com/postgres/postgres/blob/master/src/backend/access/heap/heapam.c#L572
Attachments:
[application/octet-stream] v2-0001-Remove-breaks-in-HeapTupleSatisfiesVisibility.patch (1.4K, ../../CAAKRu_ZJg_N7zHtWP+JoSY_hrce4+GKioL137Y2c2En-kuXQ7g@mail.gmail.com/2-v2-0001-Remove-breaks-in-HeapTupleSatisfiesVisibility.patch)
download | inline diff:
From b36ef3d8f812e8500580a695faa0a9229f28176b Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 26 Oct 2022 20:00:34 -0400
Subject: [PATCH v2 1/3] Remove breaks in HeapTupleSatisfiesVisibility
breaks in HeapTupleSatisfiesVisibility were superfluous
---
src/backend/access/heap/heapam_visibility.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 6e33d1c881..dd5d5da190 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -1769,25 +1769,18 @@ HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot, Buffer buffer)
{
case SNAPSHOT_MVCC:
return HeapTupleSatisfiesMVCC(htup, snapshot, buffer);
- break;
case SNAPSHOT_SELF:
return HeapTupleSatisfiesSelf(htup, snapshot, buffer);
- break;
case SNAPSHOT_ANY:
return HeapTupleSatisfiesAny(htup, snapshot, buffer);
- break;
case SNAPSHOT_TOAST:
return HeapTupleSatisfiesToast(htup, snapshot, buffer);
- break;
case SNAPSHOT_DIRTY:
return HeapTupleSatisfiesDirty(htup, snapshot, buffer);
- break;
case SNAPSHOT_HISTORIC_MVCC:
return HeapTupleSatisfiesHistoricMVCC(htup, snapshot, buffer);
- break;
case SNAPSHOT_NON_VACUUMABLE:
return HeapTupleSatisfiesNonVacuumable(htup, snapshot, buffer);
- break;
}
return false; /* keep compiler quiet */
--
2.38.1
[application/octet-stream] v2-0002-Turn-HeapKeyTest-macro-into-function.patch (3.7K, ../../CAAKRu_ZJg_N7zHtWP+JoSY_hrce4+GKioL137Y2c2En-kuXQ7g@mail.gmail.com/3-v2-0002-Turn-HeapKeyTest-macro-into-function.patch)
download | inline diff:
From 062534beeedc1aa66309ed200c34c5c1cf27bdb2 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 31 Oct 2022 13:40:06 -0400
Subject: [PATCH v2 2/3] Turn HeapKeyTest macro into function
This should always be inlined appropriately now. It is easier to read as
a function. Also, remove unused include in catcache.c.
---
src/backend/access/heap/heapam.c | 8 +--
src/backend/utils/cache/catcache.c | 1 -
src/include/access/valid.h | 81 +++++++++++++-----------------
3 files changed, 39 insertions(+), 51 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 12be87efed..fdf3b08fcc 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -716,8 +716,8 @@ heapgettup(HeapScanDesc scan,
snapshot);
if (valid && key != NULL)
- HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key, valid);
+ valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key);
if (valid)
{
@@ -1032,8 +1032,8 @@ heapgettup_pagemode(HeapScanDesc scan,
{
bool valid;
- HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key, valid);
+ valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key);
if (valid)
{
scan->rs_cindex = lineindex;
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 38e943fab2..30ef0ba39c 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -19,7 +19,6 @@
#include "access/relscan.h"
#include "access/sysattr.h"
#include "access/table.h"
-#include "access/valid.h"
#include "access/xact.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_operator.h"
diff --git a/src/include/access/valid.h b/src/include/access/valid.h
index a5525d0d63..f0f563c2b0 100644
--- a/src/include/access/valid.h
+++ b/src/include/access/valid.h
@@ -14,56 +14,45 @@
#ifndef VALID_H
#define VALID_H
+#include "access/htup.h"
+#include "access/htup_details.h"
+#include "access/tupdesc.h"
+#include "access/skey.h"
+
/*
* HeapKeyTest
*
* Test a heap tuple to see if it satisfies a scan key.
*/
-#define HeapKeyTest(tuple, \
- tupdesc, \
- nkeys, \
- keys, \
- result) \
-do \
-{ \
- /* Use underscores to protect the variables passed in as parameters */ \
- int __cur_nkeys = (nkeys); \
- ScanKey __cur_keys = (keys); \
- \
- (result) = true; /* may change */ \
- for (; __cur_nkeys--; __cur_keys++) \
- { \
- Datum __atp; \
- bool __isnull; \
- Datum __test; \
- \
- if (__cur_keys->sk_flags & SK_ISNULL) \
- { \
- (result) = false; \
- break; \
- } \
- \
- __atp = heap_getattr((tuple), \
- __cur_keys->sk_attno, \
- (tupdesc), \
- &__isnull); \
- \
- if (__isnull) \
- { \
- (result) = false; \
- break; \
- } \
- \
- __test = FunctionCall2Coll(&__cur_keys->sk_func, \
- __cur_keys->sk_collation, \
- __atp, __cur_keys->sk_argument); \
- \
- if (!DatumGetBool(__test)) \
- { \
- (result) = false; \
- break; \
- } \
- } \
-} while (0)
+static inline bool
+HeapKeyTest(HeapTuple tuple, TupleDesc tupdesc, int nkeys, ScanKey keys)
+{
+ int cur_nkeys = nkeys;
+ ScanKey cur_key = keys;
+
+ for (; cur_nkeys--; cur_key++)
+ {
+ Datum atp;
+ bool isnull;
+ Datum test;
+
+ if (cur_key->sk_flags & SK_ISNULL)
+ return false;
+
+ atp = heap_getattr(tuple, cur_key->sk_attno, tupdesc, &isnull);
+
+ if (isnull)
+ return false;
+
+ test = FunctionCall2Coll(&cur_key->sk_func,
+ cur_key->sk_collation,
+ atp, cur_key->sk_argument);
+
+ if (!DatumGetBool(test))
+ return false;
+ }
+
+ return true;
+}
#endif /* VALID_H */
--
2.38.1
[application/octet-stream] v2-0003-Refactor-heapgettup-and-heapgetpage.patch (32.2K, ../../CAAKRu_ZJg_N7zHtWP+JoSY_hrce4+GKioL137Y2c2En-kuXQ7g@mail.gmail.com/4-v2-0003-Refactor-heapgettup-and-heapgetpage.patch)
download | inline diff:
From 2304f808998b94fb8a7457cb788c8fe709d1d639 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 31 Oct 2022 13:40:29 -0400
Subject: [PATCH v2 3/3] Refactor heapgettup* and heapgetpage
Simplify heapgettup(), heapgettup_pagemode(), and heapgetpage(). All
three contained several unnecessary local variables, duplicate code, and
nested if statements. Streamlining these improves readability and
extensibility.
---
src/backend/access/heap/heapam.c | 939 +++++++++++++------------------
src/include/access/heapam.h | 8 +-
2 files changed, 384 insertions(+), 563 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fdf3b08fcc..f235b28dcf 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -370,19 +370,18 @@ heap_setscanlimits(TableScanDesc sscan, BlockNumber startBlk, BlockNumber numBlk
* which tuples on the page are visible.
*/
void
-heapgetpage(TableScanDesc sscan, BlockNumber page)
+heapgetpage(TableScanDesc sscan, BlockNumber block)
{
HeapScanDesc scan = (HeapScanDesc) sscan;
Buffer buffer;
- Snapshot snapshot;
- Page dp;
- int lines;
+ Snapshot snapshot;
+ Page page;
+ int lines;
int ntup;
OffsetNumber lineoff;
- ItemId lpp;
bool all_visible;
- Assert(page < scan->rs_nblocks);
+ Assert(block < scan->rs_nblocks);
/* release previous scan buffer, if any */
if (BufferIsValid(scan->rs_cbuf))
@@ -399,9 +398,9 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
CHECK_FOR_INTERRUPTS();
/* read page using selected strategy */
- scan->rs_cbuf = ReadBufferExtended(scan->rs_base.rs_rd, MAIN_FORKNUM, page,
- RBM_NORMAL, scan->rs_strategy);
- scan->rs_cblock = page;
+ scan->rs_cbuf = ReadBufferExtended(scan->rs_base.rs_rd, MAIN_FORKNUM,
+ block, RBM_NORMAL, scan->rs_strategy);
+ scan->rs_cblock = block;
if (!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE))
return;
@@ -421,9 +420,9 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
*/
LockBuffer(buffer, BUFFER_LOCK_SHARE);
- dp = BufferGetPage(buffer);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber(dp);
+ page = BufferGetPage(buffer);
+ TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
+ lines = PageGetMaxOffsetNumber(page);
ntup = 0;
/*
@@ -446,33 +445,33 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
* full page write. Until we can prove that beyond doubt, let's check each
* tuple for visibility the hard way.
*/
- all_visible = PageIsAllVisible(dp) && !snapshot->takenDuringRecovery;
+ all_visible = PageIsAllVisible(page) && !snapshot->takenDuringRecovery;
- for (lineoff = FirstOffsetNumber, lpp = PageGetItemId(dp, lineoff);
- lineoff <= lines;
- lineoff++, lpp++)
+ for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
{
- if (ItemIdIsNormal(lpp))
- {
- HeapTupleData loctup;
- bool valid;
+ HeapTupleData loctup;
+ bool valid;
- loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
- loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- loctup.t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(loctup.t_self), page, lineoff);
+ ItemId lpp = PageGetItemId(page, lineoff);
- if (all_visible)
- valid = true;
- else
- valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- &loctup, buffer, snapshot);
+ loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
+ loctup.t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
+ loctup.t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(loctup.t_self), block, lineoff);
- if (valid)
- scan->rs_vistuples[ntup++] = lineoff;
- }
+ if (all_visible)
+ valid = true;
+ else
+ valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+
+ HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+ &loctup, buffer, snapshot);
+
+ if (valid)
+ scan->rs_vistuples[ntup++] = lineoff;
}
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
@@ -481,6 +480,226 @@ heapgetpage(TableScanDesc sscan, BlockNumber page)
scan->rs_ntuples = ntup;
}
+
+/*
+ * ``no movement'' scan direction: refetch prior tuple
+ */
+static inline void
+heapgettup_no_movement(HeapScanDesc scan)
+{
+ ItemId lpp;
+ OffsetNumber lineoff;
+ BlockNumber page;
+ Page dp;
+ HeapTuple tuple = &(scan->rs_ctup);
+
+ /* The scan must be init'd for there to be a current tuple (rs_ctup) */
+ Assert(scan->rs_inited);
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ page = ItemPointerGetBlockNumber(&(tuple->t_self));
+ if (page != scan->rs_cblock)
+ heapgetpage((TableScanDesc) scan, page);
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+ lineoff = scan->rs_cindex;
+ lpp = PageGetItemId(dp, lineoff);
+ Assert(ItemIdIsNormal(lpp));
+
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+
+ /* check that rs_cindex is in sync if in pagemode */
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (scan->rs_cindex < scan->rs_ntuples));
+
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (lineoff == scan->rs_vistuples[scan->rs_cindex]));
+
+ return;
+}
+
+static inline Page
+heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
+ dir, int *linesleft, OffsetNumber *lineoff)
+{
+ Page page;
+
+ Assert(scan->rs_inited);
+ Assert(BufferIsValid(scan->rs_cbuf));
+
+ /* Caller is responsible for ensuring buffer is locked if needed */
+ page = BufferGetPage(scan->rs_cbuf);
+
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+ if (ScanDirectionIsForward(dir))
+ {
+ *lineoff = OffsetNumberNext(scan->rs_cindex);
+ *linesleft = PageGetMaxOffsetNumber(page) - (*lineoff) + 1;
+ }
+ else
+ {
+ /*
+ * The previous returned tuple may have been vacuumed since the
+ * previous scan when we use a non-MVCC snapshot, so we must
+ * re-establish the lineoff <= PageGetMaxOffsetNumber(page)
+ * invariant
+ */
+ *lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
+ *linesleft = *lineoff;
+ }
+ /* block and lineoff now reference the physically next tid */
+ return page;
+}
+
+static inline Page
+heapgettup_start_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir,
+ int *linesleft, OffsetNumber *lineoff)
+{
+ Page page;
+
+ Assert(scan->rs_inited);
+ Assert(BufferIsValid(scan->rs_cbuf));
+
+ /* Caller is responsible for ensuring buffer is locked if needed */
+ page = BufferGetPage(scan->rs_cbuf);
+
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+ *linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
+
+ if (ScanDirectionIsForward(dir))
+ *lineoff = FirstOffsetNumber;
+ else
+ *lineoff = (OffsetNumber) (*linesleft);
+
+ return page;
+}
+
+static inline BlockNumber
+heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
+{
+ Assert(!ScanDirectionIsNoMovement(dir));
+ Assert(!scan->rs_inited);
+
+ /* return null immediately if relation is empty */
+ if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+
+ scan->rs_inited = true;
+
+ /* forward and serial */
+ if (ScanDirectionIsForward(dir) && scan->rs_base.rs_parallel == NULL)
+ return scan->rs_startblock;
+
+ /* forward and parallel */
+ if (ScanDirectionIsForward(dir))
+ {
+ table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+ return table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+ }
+
+ /* backward parallel scan not supported */
+ Assert(scan->rs_base.rs_parallel == NULL);
+
+ /*
+ * Disable reporting to syncscan logic in a backwards scan; it's
+ * not very likely anyone else is doing the same thing at the same
+ * time, and much more likely that we'll just bollix things for
+ * forward scanners.
+ */
+ scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
+
+ /*
+ * Start from last page of the scan. Ensure we take into account
+ * rs_numblocks if it's been adjusted by heap_setscanlimits().
+ */
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ return (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
+
+ if (scan->rs_startblock > 0)
+ return scan->rs_startblock - 1;
+
+ return scan->rs_nblocks - 1;
+}
+
+static inline BlockNumber
+heapgettup_advance_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir)
+{
+ if (ScanDirectionIsBackward(dir))
+ {
+ if (block == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ if (block == 0)
+ block = scan->rs_nblocks;
+
+ block--;
+
+ return block;
+ }
+ else if (scan->rs_base.rs_parallel != NULL)
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata, (ParallelBlockTableScanDesc)
+ scan->rs_base.rs_parallel);
+
+ return block;
+ }
+ else
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ block++;
+
+ if (block >= scan->rs_nblocks)
+ block = 0;
+
+ if (block == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ /*
+ * Report our new scan position for synchronization purposes. We
+ * don't do that when moving backwards, however. That would just
+ * mess up any other forward-moving scanners.
+ *
+ * Note: we do this before checking for end of scan so that the
+ * final state of the position hint is back at the start of the
+ * rel. That's not strictly necessary, but otherwise when you run
+ * the same query multiple times the starting position would shift
+ * a little bit backwards on every invocation, which is confusing.
+ * We don't guarantee any specific ordering in general, though.
+ */
+ if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
+ ss_report_location(scan->rs_base.rs_rd, block);
+
+ return block;
+ }
+}
+
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -510,323 +729,108 @@ heapgettup(HeapScanDesc scan,
int nkeys,
ScanKey key)
{
- HeapTuple tuple = &(scan->rs_ctup);
- Snapshot snapshot = scan->rs_base.rs_snapshot;
- bool backward = ScanDirectionIsBackward(dir);
- BlockNumber page;
- bool finished;
- Page dp;
- int lines;
+ HeapTuple tuple = &(scan->rs_ctup);
+ BlockNumber block;
+ Page page;
OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
- /*
- * calculate next starting lineoff, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (unlikely(ScanDirectionIsNoMovement(dir)))
{
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- /* Other processes might have already finished the scan. */
- if (page == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- page = scan->rs_startblock; /* first page */
- heapgetpage((TableScanDesc) scan, page);
- lineoff = FirstOffsetNumber; /* first offnum */
- scan->rs_inited = true;
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- lineoff = /* next offnum */
- OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
- }
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber(dp);
- /* page and lineoff now reference the physically next tid */
-
- linesleft = lines - lineoff + 1;
+ heapgettup_no_movement(scan);
+ return;
}
- else if (backward)
- {
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- page = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- page = scan->rs_startblock - 1;
- else
- page = scan->rs_nblocks - 1;
- heapgetpage((TableScanDesc) scan, page);
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- }
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber(dp);
-
- if (!scan->rs_inited)
- {
- lineoff = lines; /* final offnum */
- scan->rs_inited = true;
- }
- else
- {
- /*
- * The previous returned tuple may have been vacuumed since the
- * previous scan when we use a non-MVCC snapshot, so we must
- * re-establish the lineoff <= PageGetMaxOffsetNumber(dp)
- * invariant
- */
- lineoff = /* previous offnum */
- Min(lines,
- OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));
- }
- /* page and lineoff now reference the physically previous tid */
+ if (!scan->rs_inited)
+ {
+ block = heapgettup_initial_page(scan, dir);
- linesleft = lineoff;
+ /*
+ * If parallel and other processes have already finished the scan, the
+ * returned block is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
+ */
+ Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
- /*
- * ``no movement'' scan direction: refetch prior tuple
- */
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- page = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (page != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, page);
-
- /* Since the tuple was previously fetched, needn't lock page here */
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(dp, lineoff);
- Assert(ItemIdIsNormal(lpp));
-
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
-
- return;
+ block = scan->rs_cblock;
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
+ goto continue_page;
}
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- lpp = PageGetItemId(dp, lineoff);
- for (;;)
+ while (block != InvalidBlockNumber)
{
+ heapgetpage((TableScanDesc) scan, block);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
+ continue_page:
+
/*
- * Only continue scanning the page while we have lines left.
+ * Only continue scanning the block while we have lines left.
*
* Note that this protects us from accessing line pointers past
* PageGetMaxOffsetNumber(); both for forward scans when we resume the
- * table scan, and for when we start scanning a new page.
+ * table scan, and for when we start scanning a new block.
*/
- while (linesleft > 0)
+ for (; linesleft > 0; linesleft--, lineoff += dir)
{
- if (ItemIdIsNormal(lpp))
- {
- bool valid;
-
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(tuple->t_self), page, lineoff);
+ bool visible;
+ ItemId lpp = PageGetItemId(page, lineoff);
- /*
- * if current tuple qualifies, return it.
- */
- valid = HeapTupleSatisfiesVisibility(tuple,
- snapshot,
- scan->rs_cbuf);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- tuple, scan->rs_cbuf,
- snapshot);
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(tuple->t_self), scan->rs_cblock, lineoff);
- if (valid && key != NULL)
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
+ /*
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the block
+ */
+ visible = HeapTupleSatisfiesVisibility(tuple,
+ scan->rs_base.rs_snapshot,
+ scan->rs_cbuf);
+
+ HeapCheckForSerializableConflictOut(visible, scan->rs_base.rs_rd,
+ tuple, scan->rs_cbuf,
+ scan->rs_base.rs_snapshot);
+
+ if (!visible)
+ continue;
- if (valid)
- {
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- return;
- }
- }
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- {
- --lpp; /* move back in this page's ItemId array */
- --lineoff;
- }
- else
- {
- ++lpp; /* move forward in this page's ItemId array */
- ++lineoff;
- }
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
+ return;
}
/*
- * if we get here, it means we've exhausted the items on this page and
+ * if we get here, it means we've exhausted the items on this block and
* it's time to move to the next.
*/
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- /*
- * advance to next/prior page and detect end of scan
- */
- if (backward)
- {
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (page == 0)
- page = scan->rs_nblocks;
- page--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (page == InvalidBlockNumber);
- }
- else
- {
- page++;
- if (page >= scan->rs_nblocks)
- page = 0;
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, page);
- }
-
- /*
- * return NULL if we've exhausted all the pages
- */
- if (finished)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, page);
+ block = heapgettup_advance_page(scan, block, dir);
+ }
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, dp);
- lines = PageGetMaxOffsetNumber((Page) dp);
- linesleft = lines;
- if (backward)
- {
- lineoff = lines;
- lpp = PageGetItemId(dp, lines);
- }
- else
- {
- lineoff = FirstOffsetNumber;
- lpp = PageGetItemId(dp, FirstOffsetNumber);
- }
- }
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
/* ----------------
@@ -848,286 +852,99 @@ heapgettup_pagemode(HeapScanDesc scan,
int nkeys,
ScanKey key)
{
- HeapTuple tuple = &(scan->rs_ctup);
- bool backward = ScanDirectionIsBackward(dir);
- BlockNumber page;
- bool finished;
- Page dp;
- int lines;
- int lineindex;
- OffsetNumber lineoff;
+ HeapTuple tuple = &(scan->rs_ctup);
+ BlockNumber block;
+ Page page;
+ int lineindex;
int linesleft;
- ItemId lpp;
- /*
- * calculate next starting lineindex, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (unlikely(ScanDirectionIsNoMovement(dir)))
{
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- /* Other processes might have already finished the scan. */
- if (page == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- page = scan->rs_startblock; /* first page */
- heapgetpage((TableScanDesc) scan, page);
- lineindex = 0;
- scan->rs_inited = true;
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- lineindex = scan->rs_cindex + 1;
- }
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
- /* page and lineindex now reference the next visible tid */
-
- linesleft = lines - lineindex;
+ heapgettup_no_movement(scan);
+ return;
}
- else if (backward)
- {
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
-
- if (!scan->rs_inited)
- {
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- page = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- page = scan->rs_startblock - 1;
- else
- page = scan->rs_nblocks - 1;
- heapgetpage((TableScanDesc) scan, page);
- }
- else
- {
- /* continue from previously returned page/tuple */
- page = scan->rs_cblock; /* current page */
- }
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
-
- if (!scan->rs_inited)
- {
- lineindex = lines - 1;
- scan->rs_inited = true;
- }
- else
- {
- lineindex = scan->rs_cindex - 1;
- }
- /* page and lineindex now reference the previous visible tid */
+ if (!scan->rs_inited)
+ {
+ block = heapgettup_initial_page(scan, dir);
- linesleft = lineindex + 1;
+ /*
+ * If parallel and other processes have already finished the scan, the
+ * returned block is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
+ */
+ Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
- /*
- * ``no movement'' scan direction: refetch prior tuple
- */
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- page = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (page != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, page);
-
- /* Since the tuple was previously fetched, needn't lock page here */
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(dp, lineoff);
- Assert(ItemIdIsNormal(lpp));
-
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
+ block = scan->rs_cblock;
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
+ lineindex = scan->rs_cindex + dir;
+ if (ScanDirectionIsForward(dir))
+ linesleft = scan->rs_ntuples - lineindex;
+ else
+ linesleft = scan->rs_cindex;
- return;
+ /* block and lineindex now reference the next visible tid */
+ goto continue_page;
}
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- for (;;)
+ while (block != InvalidBlockNumber)
{
- while (linesleft > 0)
+ heapgetpage((TableScanDesc) scan, block);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+ linesleft = scan->rs_ntuples;
+ lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+ /* block and lineindex now reference the previous visible tid */
+
+ continue_page:
+
+ for (; linesleft > 0; linesleft--, lineindex += dir)
{
+ ItemId lpp;
+ OffsetNumber lineoff;
+
lineoff = scan->rs_vistuples[lineindex];
- lpp = PageGetItemId(dp, lineoff);
+ lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
- tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
tuple->t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(tuple->t_self), page, lineoff);
+ ItemPointerSet(&(tuple->t_self), block, lineoff);
/*
- * if current tuple qualifies, return it.
- */
- if (key != NULL)
- {
- bool valid;
-
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
- if (valid)
- {
- scan->rs_cindex = lineindex;
- return;
- }
- }
- else
- {
- scan->rs_cindex = lineindex;
- return;
- }
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the block
+ */
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- --lineindex;
- else
- ++lineindex;
+ scan->rs_cindex = lineindex;
+ return;
}
/*
- * if we get here, it means we've exhausted the items on this page and
+ * if we get here, it means we've exhausted the items on this block and
* it's time to move to the next.
*/
- if (backward)
- {
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (page == 0)
- page = scan->rs_nblocks;
- page--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- page = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (page == InvalidBlockNumber);
- }
- else
- {
- page++;
- if (page >= scan->rs_nblocks)
- page = 0;
- finished = (page == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, page);
- }
-
- /*
- * return NULL if we've exhausted all the pages
- */
- if (finished)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, page);
-
- dp = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
- lines = scan->rs_ntuples;
- linesleft = lines;
- if (backward)
- lineindex = lines - 1;
- else
- lineindex = 0;
+ block = heapgettup_advance_page(scan, block, dir);
}
+
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 9dab35551e..83884685a7 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -71,8 +71,12 @@ typedef struct HeapScanDescData
*/
ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
+ /*
+ * current tuple's index in vistuples or current lineoff in page
+ */
+ int rs_cindex;
+
/* 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;
@@ -120,7 +124,7 @@ extern TableScanDesc heap_beginscan(Relation relation, Snapshot snapshot,
uint32 flags);
extern void heap_setscanlimits(TableScanDesc sscan, BlockNumber startBlk,
BlockNumber numBlks);
-extern void heapgetpage(TableScanDesc sscan, BlockNumber page);
+extern void heapgetpage(TableScanDesc sscan, BlockNumber block);
extern void heap_rescan(TableScanDesc sscan, ScanKey key, bool set_params,
bool allow_strat, bool allow_sync, bool allow_pagemode);
extern void heap_endscan(TableScanDesc sscan);
--
2.38.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: heapgettup refactoring
@ 2022-11-16 15:49 Peter Eisentraut <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2022-11-16 15:49 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On 04.11.22 16:51, Melanie Plageman wrote:
> Thanks for the review!
> Attached is v2 with feedback addressed.
Your 0001 had already been pushed.
I have pushed your 0002.
I have also pushed the renaming of page -> block, dp -> page separately.
This should reduce the size of your 0003 a bit.
Please produce an updated version of the 0003 page for further review.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: heapgettup refactoring
@ 2022-11-30 22:34 Melanie Plageman <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Melanie Plageman @ 2022-11-30 22:34 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Wed, Nov 16, 2022 at 10:49 AM Peter Eisentraut
<[email protected]> wrote:
>
> On 04.11.22 16:51, Melanie Plageman wrote:
> > Thanks for the review!
> > Attached is v2 with feedback addressed.
>
> Your 0001 had already been pushed.
>
> I have pushed your 0002.
>
> I have also pushed the renaming of page -> block, dp -> page separately.
> This should reduce the size of your 0003 a bit.
>
> Please produce an updated version of the 0003 page for further review.
Thanks for looking at this!
I have attached a patchset with only the code changes contained in the
previous patch 0003. I have broken the refactoring down into many
smaller pieces for ease of review.
- Melanie
Attachments:
[text/x-patch] v3-0005-Add-heapgettup-helpers.patch (3.9K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/2-v3-0005-Add-heapgettup-helpers.patch)
download | inline diff:
From 2e63656e75b00b24d286533ae8c8ef291876d4a8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 14:18:08 -0500
Subject: [PATCH v3 5/7] Add heapgettup() helpers
Add heapgettup_start_page() and heapgettup_continue_page(), helper
functions for heapgettup().
---
src/backend/access/heap/heapam.c | 93 +++++++++++++++++++++-----------
1 file changed, 61 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a4365beee1..f9b2d5cadb 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -575,6 +575,65 @@ heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
return scan->rs_nblocks - 1;
}
+static inline Page
+heapgettup_start_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir,
+ int *linesleft, OffsetNumber *lineoff)
+{
+ Page page;
+
+ Assert(scan->rs_inited);
+ Assert(BufferIsValid(scan->rs_cbuf));
+
+ /* Caller is responsible for ensuring buffer is locked if needed */
+ page = BufferGetPage(scan->rs_cbuf);
+
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+ *linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
+
+ if (ScanDirectionIsForward(dir))
+ *lineoff = FirstOffsetNumber;
+ else
+ *lineoff = (OffsetNumber) (*linesleft);
+
+ /* lineoff now references the physically previous or next tid */
+ return page;
+}
+
+static inline Page
+heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
+ dir, int *linesleft, OffsetNumber *lineoff)
+{
+ Page page;
+
+ Assert(scan->rs_inited);
+ Assert(BufferIsValid(scan->rs_cbuf));
+
+ /* Caller is responsible for ensuring buffer is locked if needed */
+ page = BufferGetPage(scan->rs_cbuf);
+
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+
+ if (ScanDirectionIsForward(dir))
+ {
+ *lineoff = OffsetNumberNext(scan->rs_cindex);
+ *linesleft = PageGetMaxOffsetNumber(page) - (*lineoff) + 1;
+ }
+ else
+ {
+ /*
+ * The previous returned tuple may have been vacuumed since the
+ * previous scan when we use a non-MVCC snapshot, so we must
+ * re-establish the lineoff <= PageGetMaxOffsetNumber(page)
+ * invariant
+ */
+ *lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
+ *linesleft = *lineoff;
+ }
+ /* block and lineoff now reference the physically next tid */
+ return page;
+}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -632,45 +691,15 @@ heapgettup(HeapScanDesc scan,
}
heapgetpage((TableScanDesc) scan, block);
-
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-
- linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
-
- if (ScanDirectionIsForward(dir))
- lineoff = FirstOffsetNumber;
- else
- lineoff = (OffsetNumber) linesleft;
+ page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
}
else
{
block = scan->rs_cblock; /* current page */
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
-
- if (ScanDirectionIsForward(dir))
- {
- lineoff = OffsetNumberNext(scan->rs_cindex);
- /* block and lineoff now reference the physically next tid */
- linesleft = PageGetMaxOffsetNumber(page) - lineoff + 1;
- }
- else
- {
- /*
- * The previous returned tuple may have been vacuumed since the
- * previous scan when we use a non-MVCC snapshot, so we must
- * re-establish the lineoff <= PageGetMaxOffsetNumber(page)
- * invariant
- */
- lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
- /* block and lineoff now reference the physically previous tid */
- linesleft = lineoff;
- }
+ page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
}
/*
--
2.34.1
[text/x-patch] v3-0002-Push-lpp-variable-closer-to-usage-in-heapgetpage.patch (2.3K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/3-v3-0002-Push-lpp-variable-closer-to-usage-in-heapgetpage.patch)
download | inline diff:
From 8d4106f9f045999c5b3432fc19ee8f216b85df2d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 10:42:12 -0500
Subject: [PATCH v3 2/7] Push lpp variable closer to usage in heapgetpage()
---
src/backend/access/heap/heapam.c | 41 ++++++++++++++++----------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f15296a67a..3da9c81c5b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -382,7 +382,6 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
int lines;
int ntup;
OffsetNumber lineoff;
- ItemId lpp;
bool all_visible;
Assert(block < scan->rs_nblocks);
@@ -451,31 +450,31 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
*/
all_visible = PageIsAllVisible(page) && !snapshot->takenDuringRecovery;
- for (lineoff = FirstOffsetNumber, lpp = PageGetItemId(page, lineoff);
- lineoff <= lines;
- lineoff++, lpp++)
+ for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
{
- if (ItemIdIsNormal(lpp))
- {
- HeapTupleData loctup;
- bool valid;
+ HeapTupleData loctup;
+ bool valid;
- loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
- loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
- loctup.t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(loctup.t_self), block, lineoff);
+ ItemId lpp = PageGetItemId(page, lineoff);
- if (all_visible)
- valid = true;
- else
- valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- &loctup, buffer, snapshot);
+ loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
+ loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+ loctup.t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(loctup.t_self), block, lineoff);
- if (valid)
- scan->rs_vistuples[ntup++] = lineoff;
- }
+ if (all_visible)
+ valid = true;
+ else
+ valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+
+ HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+ &loctup, buffer, snapshot);
+
+ if (valid)
+ scan->rs_vistuples[ntup++] = lineoff;
}
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
--
2.34.1
[text/x-patch] v3-0003-Add-heapgettup_initial_page-helper.patch (9.1K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/4-v3-0003-Add-heapgettup_initial_page-helper.patch)
download | inline diff:
From 2ddb8123f1a8d1d6938c15608a57d09958cfecfd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 17:06:43 -0500
Subject: [PATCH v3 3/7] Add heapgettup_initial_page() helper
---
src/backend/access/heap/heapam.c | 215 ++++++++++++-------------------
1 file changed, 82 insertions(+), 133 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3da9c81c5b..142bcc0a1c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -523,6 +523,58 @@ heapgettup_no_movement(HeapScanDesc scan)
return;
}
+static inline BlockNumber
+heapgettup_initial_page(HeapScanDesc scan, ScanDirection dir)
+{
+ Assert(!ScanDirectionIsNoMovement(dir));
+ Assert(!scan->rs_inited);
+
+ /* return null immediately if relation is empty */
+ if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+
+ scan->rs_inited = true;
+
+ /* forward and serial */
+ if (ScanDirectionIsForward(dir) && scan->rs_base.rs_parallel == NULL)
+ return scan->rs_startblock;
+
+ /* forward and parallel */
+ if (ScanDirectionIsForward(dir))
+ {
+ table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+
+ return table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata,
+ (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel);
+ }
+
+ /* backward parallel scan not supported */
+ Assert(scan->rs_base.rs_parallel == NULL);
+
+ /*
+ * Disable reporting to syncscan logic in a backwards scan; it's
+ * not very likely anyone else is doing the same thing at the same
+ * time, and much more likely that we'll just bollix things for
+ * forward scanners.
+ */
+ scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
+
+ /*
+ * Start from last page of the scan. Ensure we take into account
+ * rs_numblocks if it's been adjusted by heap_setscanlimits().
+ */
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ return (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
+
+ if (scan->rs_startblock > 0)
+ return scan->rs_startblock - 1;
+
+ return scan->rs_nblocks - 1;
+}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -569,48 +621,20 @@ heapgettup(HeapScanDesc scan,
return;
}
- /*
- * calculate next starting lineoff, given scan direction
- */
if (ScanDirectionIsForward(dir))
{
if (!scan->rs_inited)
{
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ block = heapgettup_initial_page(scan, dir);
+
+ if (block == InvalidBlockNumber)
{
Assert(!BufferIsValid(scan->rs_cbuf));
tuple->t_data = NULL;
return;
}
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- /* Other processes might have already finished the scan. */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- block = scan->rs_startblock; /* first page */
heapgetpage((TableScanDesc) scan, block);
- lineoff = FirstOffsetNumber; /* first offnum */
- scan->rs_inited = true;
+ lineoff = FirstOffsetNumber;
}
else
{
@@ -632,60 +656,36 @@ heapgettup(HeapScanDesc scan,
else
{
Assert(backward);
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
if (!scan->rs_inited)
{
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ block = heapgettup_initial_page(scan, dir);
+
+ if (block == InvalidBlockNumber)
{
Assert(!BufferIsValid(scan->rs_cbuf));
tuple->t_data = NULL;
return;
}
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- block = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- block = scan->rs_startblock - 1;
- else
- block = scan->rs_nblocks - 1;
heapgetpage((TableScanDesc) scan, block);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
+ lines = PageGetMaxOffsetNumber(page);
+ lineoff = lines; /* final offnum */
}
else
{
/* continue from previously returned page/tuple */
block = scan->rs_cblock; /* current page */
- }
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
+ lines = PageGetMaxOffsetNumber(page);
- if (!scan->rs_inited)
- {
- lineoff = lines; /* final offnum */
- scan->rs_inited = true;
- }
- else
- {
/*
* The previous returned tuple may have been vacuumed since the
* previous scan when we use a non-MVCC snapshot, so we must
@@ -893,41 +893,17 @@ heapgettup_pagemode(HeapScanDesc scan,
{
if (!scan->rs_inited)
{
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ block = heapgettup_initial_page(scan, dir);
+
+ if (block == InvalidBlockNumber)
{
Assert(!BufferIsValid(scan->rs_cbuf));
tuple->t_data = NULL;
return;
}
- if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- table_block_parallelscan_startblock_init(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
-
- block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- /* Other processes might have already finished the scan. */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- }
- else
- block = scan->rs_startblock; /* first page */
heapgetpage((TableScanDesc) scan, block);
lineindex = 0;
- scan->rs_inited = true;
}
else
{
@@ -946,58 +922,31 @@ heapgettup_pagemode(HeapScanDesc scan,
else
{
Assert(backward);
- /* backward parallel scan not supported */
- Assert(scan->rs_base.rs_parallel == NULL);
if (!scan->rs_inited)
{
- /*
- * return null immediately if relation is empty
- */
- if (scan->rs_nblocks == 0 || scan->rs_numblocks == 0)
+ block = heapgettup_initial_page(scan, dir);
+
+ if (block == InvalidBlockNumber)
{
Assert(!BufferIsValid(scan->rs_cbuf));
tuple->t_data = NULL;
return;
}
- /*
- * Disable reporting to syncscan logic in a backwards scan; it's
- * not very likely anyone else is doing the same thing at the same
- * time, and much more likely that we'll just bollix things for
- * forward scanners.
- */
- scan->rs_base.rs_flags &= ~SO_ALLOW_SYNC;
-
- /*
- * Start from last page of the scan. Ensure we take into account
- * rs_numblocks if it's been adjusted by heap_setscanlimits().
- */
- if (scan->rs_numblocks != InvalidBlockNumber)
- block = (scan->rs_startblock + scan->rs_numblocks - 1) % scan->rs_nblocks;
- else if (scan->rs_startblock > 0)
- block = scan->rs_startblock - 1;
- else
- block = scan->rs_nblocks - 1;
heapgetpage((TableScanDesc) scan, block);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+ lines = scan->rs_ntuples;
+ lineindex = lines - 1;
}
else
{
/* continue from previously returned page/tuple */
block = scan->rs_cblock; /* current page */
- }
-
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lines = scan->rs_ntuples;
-
- if (!scan->rs_inited)
- {
- lineindex = lines - 1;
- scan->rs_inited = true;
- }
- else
- {
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+ lines = scan->rs_ntuples;
lineindex = scan->rs_cindex - 1;
}
/* block and lineindex now reference the previous visible tid */
--
2.34.1
[text/x-patch] v3-0001-Add-no-movement-scan-helper.patch (5.0K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/5-v3-0001-Add-no-movement-scan-helper.patch)
download | inline diff:
From 38d83796ff3f3060a8dcb244c85864279f080450 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 10:20:30 -0500
Subject: [PATCH v3 1/7] Add no movement scan helper
No movement scan can be handled first in heapgettup() and
heapgettup_pagemode(). Refactor this case into its own helper function
to improve readability.
---
src/backend/access/heap/heapam.c | 118 +++++++++++++++----------------
1 file changed, 56 insertions(+), 62 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 747db50376..f15296a67a 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -484,6 +484,46 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
scan->rs_ntuples = ntup;
}
+/*
+ * ``no movement'' scan direction: refetch prior tuple
+ */
+static inline void
+heapgettup_no_movement(HeapScanDesc scan)
+{
+ ItemId lpp;
+ OffsetNumber lineoff;
+ BlockNumber page;
+ Page dp;
+ HeapTuple tuple = &(scan->rs_ctup);
+
+ /* The scan must be init'd for there to be a current tuple (rs_ctup) */
+ Assert(scan->rs_inited);
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ page = ItemPointerGetBlockNumber(&(tuple->t_self));
+ if (page != scan->rs_cblock)
+ heapgetpage((TableScanDesc) scan, page);
+
+ /* Since the tuple was previously fetched, needn't lock page here */
+ dp = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, dp);
+ lineoff = scan->rs_cindex;
+ lpp = PageGetItemId(dp, lineoff);
+ Assert(ItemIdIsNormal(lpp));
+
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) dp, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+
+ /* check that rs_cindex is in sync if in pagemode */
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (scan->rs_cindex < scan->rs_ntuples));
+
+ Assert(!(scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) ||
+ (lineoff == scan->rs_vistuples[scan->rs_cindex]));
+
+ return;
+}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -524,6 +564,12 @@ heapgettup(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ if (unlikely(ScanDirectionIsNoMovement(dir)))
+ {
+ heapgettup_no_movement(scan);
+ return;
+ }
+
/*
* calculate next starting lineoff, given scan direction
*/
@@ -584,8 +630,9 @@ heapgettup(HeapScanDesc scan,
linesleft = lines - lineoff + 1;
}
- else if (backward)
+ else
{
+ Assert(backward);
/* backward parallel scan not supported */
Assert(scan->rs_base.rs_parallel == NULL);
@@ -654,34 +701,6 @@ heapgettup(HeapScanDesc scan,
linesleft = lineoff;
}
- else
- {
- /*
- * ``no movement'' scan direction: refetch prior tuple
- */
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- block = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (block != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, block);
-
- /* Since the tuple was previously fetched, needn't lock page here */
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(page, lineoff);
- Assert(ItemIdIsNormal(lpp));
-
- tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
-
- return;
- }
/*
* advance the scan until we find a qualifying tuple or run out of stuff
@@ -862,6 +881,12 @@ heapgettup_pagemode(HeapScanDesc scan,
int linesleft;
ItemId lpp;
+ if (unlikely(ScanDirectionIsNoMovement(dir)))
+ {
+ heapgettup_no_movement(scan);
+ return;
+ }
+
/*
* calculate next starting lineindex, given scan direction
*/
@@ -919,8 +944,9 @@ heapgettup_pagemode(HeapScanDesc scan,
linesleft = lines - lineindex;
}
- else if (backward)
+ else
{
+ Assert(backward);
/* backward parallel scan not supported */
Assert(scan->rs_base.rs_parallel == NULL);
@@ -979,38 +1005,6 @@ heapgettup_pagemode(HeapScanDesc scan,
linesleft = lineindex + 1;
}
- else
- {
- /*
- * ``no movement'' scan direction: refetch prior tuple
- */
- if (!scan->rs_inited)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- block = ItemPointerGetBlockNumber(&(tuple->t_self));
- if (block != scan->rs_cblock)
- heapgetpage((TableScanDesc) scan, block);
-
- /* Since the tuple was previously fetched, needn't lock page here */
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self));
- lpp = PageGetItemId(page, lineoff);
- Assert(ItemIdIsNormal(lpp));
-
- tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
-
- /* check that rs_cindex is in sync */
- Assert(scan->rs_cindex < scan->rs_ntuples);
- Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
-
- return;
- }
/*
* advance the scan until we find a qualifying tuple or run out of stuff
--
2.34.1
[text/x-patch] v3-0004-Streamline-heapgettup-for-refactor.patch (8.5K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/6-v3-0004-Streamline-heapgettup-for-refactor.patch)
download | inline diff:
From 4d63c4942028d96adb431d4cd6c35330ed6e76f0 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 12:08:05 -0500
Subject: [PATCH v3 4/7] Streamline heapgettup*() for refactor
Flip initial logic to set local variables in order to make it easier to
add helper functions. Also, remove unneeded local variables.
---
src/backend/access/heap/heapam.c | 168 ++++++++++---------------------
src/include/access/heapam.h | 6 +-
2 files changed, 58 insertions(+), 116 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 142bcc0a1c..a4365beee1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -610,7 +610,6 @@ heapgettup(HeapScanDesc scan,
BlockNumber block;
bool finished;
Page page;
- int lines;
OffsetNumber lineoff;
int linesleft;
ItemId lpp;
@@ -621,84 +620,57 @@ heapgettup(HeapScanDesc scan,
return;
}
- if (ScanDirectionIsForward(dir))
+ if (!scan->rs_inited)
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_page(scan, dir);
+ block = heapgettup_initial_page(scan, dir);
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
- heapgetpage((TableScanDesc) scan, block);
- lineoff = FirstOffsetNumber;
- }
- else
+ if (block == InvalidBlockNumber)
{
- /* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
- lineoff = /* next offnum */
- OffsetNumberNext(ItemPointerGetOffsetNumber(&(tuple->t_self)));
+ Assert(!BufferIsValid(scan->rs_cbuf));
+ tuple->t_data = NULL;
+ return;
}
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ heapgetpage((TableScanDesc) scan, block);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
- /* block and lineoff now reference the physically next tid */
- linesleft = lines - lineoff + 1;
+ linesleft = PageGetMaxOffsetNumber((Page) page) - FirstOffsetNumber + 1;
+
+ if (ScanDirectionIsForward(dir))
+ lineoff = FirstOffsetNumber;
+ else
+ lineoff = (OffsetNumber) linesleft;
}
else
{
- Assert(backward);
-
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_page(scan, dir);
+ block = scan->rs_cblock; /* current page */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- heapgetpage((TableScanDesc) scan, block);
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
- lineoff = lines; /* final offnum */
+ if (ScanDirectionIsForward(dir))
+ {
+ lineoff = OffsetNumberNext(scan->rs_cindex);
+ /* block and lineoff now reference the physically next tid */
+ linesleft = PageGetMaxOffsetNumber(page) - lineoff + 1;
}
else
{
- /* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
-
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
-
/*
* The previous returned tuple may have been vacuumed since the
* previous scan when we use a non-MVCC snapshot, so we must
* re-establish the lineoff <= PageGetMaxOffsetNumber(page)
* invariant
*/
- lineoff = /* previous offnum */
- Min(lines,
- OffsetNumberPrev(ItemPointerGetOffsetNumber(&(tuple->t_self))));
+ lineoff = Min(PageGetMaxOffsetNumber(page), OffsetNumberPrev(scan->rs_cindex));
+ /* block and lineoff now reference the physically previous tid */
+ linesleft = lineoff;
}
- /* block and lineoff now reference the physically previous tid */
-
- linesleft = lineoff;
}
/*
@@ -743,6 +715,7 @@ heapgettup(HeapScanDesc scan,
if (valid)
{
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
return;
}
}
@@ -835,12 +808,11 @@ heapgettup(HeapScanDesc scan,
page = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- lines = PageGetMaxOffsetNumber(page);
- linesleft = lines;
+ linesleft = PageGetMaxOffsetNumber(page);
if (backward)
{
- lineoff = lines;
- lpp = PageGetItemId(page, lines);
+ lineoff = linesleft;
+ lpp = PageGetItemId(page, linesleft);
}
else
{
@@ -874,7 +846,6 @@ heapgettup_pagemode(HeapScanDesc scan,
BlockNumber block;
bool finished;
Page page;
- int lines;
int lineindex;
OffsetNumber lineoff;
int linesleft;
@@ -886,73 +857,41 @@ heapgettup_pagemode(HeapScanDesc scan,
return;
}
- /*
- * calculate next starting lineindex, given scan direction
- */
- if (ScanDirectionIsForward(dir))
+ if (!scan->rs_inited)
{
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_page(scan, dir);
+ block = heapgettup_initial_page(scan, dir);
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
- lineindex = 0;
- }
- else
+ if (block == InvalidBlockNumber)
{
- /* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
- lineindex = scan->rs_cindex + 1;
+ Assert(!BufferIsValid(scan->rs_cbuf));
+ tuple->t_data = NULL;
+ return;
}
+ heapgetpage((TableScanDesc) scan, block);
page = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lines = scan->rs_ntuples;
- /* block and lineindex now reference the next visible tid */
-
- linesleft = lines - lineindex;
+ linesleft = scan->rs_ntuples;
+ lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
}
else
{
- Assert(backward);
-
- if (!scan->rs_inited)
- {
- block = heapgettup_initial_page(scan, dir);
+ /* continue from previously returned page/tuple */
+ block = scan->rs_cblock; /* current page */
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- heapgetpage((TableScanDesc) scan, block);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lines = scan->rs_ntuples;
- lineindex = lines - 1;
- }
+ lineindex = scan->rs_cindex + dir;
+ if (ScanDirectionIsForward(dir))
+ linesleft = scan->rs_ntuples - lineindex;
else
- {
- /* continue from previously returned page/tuple */
- block = scan->rs_cblock; /* current page */
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lines = scan->rs_ntuples;
- lineindex = scan->rs_cindex - 1;
- }
- /* block and lineindex now reference the previous visible tid */
-
- linesleft = lineindex + 1;
+ linesleft = scan->rs_cindex;
}
+ /*
+ * block and lineindex now reference the next or previous visible tid
+ */
+
/*
* advance the scan until we find a qualifying tuple or run out of stuff
@@ -1066,10 +1005,9 @@ heapgettup_pagemode(HeapScanDesc scan,
page = BufferGetPage(scan->rs_cbuf);
TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- lines = scan->rs_ntuples;
- linesleft = lines;
+ linesleft = scan->rs_ntuples;
if (backward)
- lineindex = lines - 1;
+ lineindex = linesleft - 1;
else
lineindex = 0;
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 810baaf9d0..8db8ae7624 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -71,8 +71,12 @@ typedef struct HeapScanDescData
*/
ParallelBlockTableScanWorkerData *rs_parallelworkerdata;
+ /*
+ * current tuple's index in vistuples or current lineoff in page
+ */
+ int rs_cindex;
+
/* 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;
--
2.34.1
[text/x-patch] v3-0007-Refactor-heapgettup-and-heapgettup_pagemode.patch (9.8K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/7-v3-0007-Refactor-heapgettup-and-heapgettup_pagemode.patch)
download | inline diff:
From 5ed8a30fcb49779bf874d5b56e4bd5bf9189a00d Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 15:38:44 -0500
Subject: [PATCH v3 7/7] Refactor heapgettup() and heapgettup_pagemode()
---
src/backend/access/heap/heapam.c | 252 ++++++++++++-------------------
1 file changed, 93 insertions(+), 159 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f1081ab529..8900490f2c 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -732,13 +732,10 @@ heapgettup(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- Snapshot snapshot = scan->rs_base.rs_snapshot;
- bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
Page page;
OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
if (unlikely(ScanDirectionIsNoMovement(dir)))
{
@@ -750,32 +747,32 @@ heapgettup(HeapScanDesc scan,
{
block = heapgettup_initial_page(scan, dir);
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
- page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
+ /*
+ * If parallel and other processes have already finished the scan, the
+ * returned block is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
+ */
+ Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
block = scan->rs_cblock; /* current page */
-
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
page = heapgettup_continue_page(scan, block, dir, &linesleft, &lineoff);
+ goto continue_page;
}
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- lpp = PageGetItemId(page, lineoff);
- for (;;)
+ while (block != InvalidBlockNumber)
{
+ heapgetpage((TableScanDesc) scan, block);
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ page = heapgettup_start_page(scan, block, dir, &linesleft, &lineoff);
+ continue_page:
+
/*
* Only continue scanning the page while we have lines left.
*
@@ -783,53 +780,40 @@ heapgettup(HeapScanDesc scan,
* PageGetMaxOffsetNumber(); both for forward scans when we resume the
* table scan, and for when we start scanning a new page.
*/
- while (linesleft > 0)
+ for (; linesleft > 0; linesleft--, lineoff += dir)
{
- if (ItemIdIsNormal(lpp))
- {
- bool valid;
+ bool visible;
+ ItemId lpp = PageGetItemId(page, lineoff);
- tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
- tuple->t_len = ItemIdGetLength(lpp);
- ItemPointerSet(&(tuple->t_self), block, lineoff);
-
- /*
- * if current tuple qualifies, return it.
- */
- valid = HeapTupleSatisfiesVisibility(tuple,
- snapshot,
- scan->rs_cbuf);
+ if (!ItemIdIsNormal(lpp))
+ continue;
- HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
- tuple, scan->rs_cbuf,
- snapshot);
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
+ tuple->t_len = ItemIdGetLength(lpp);
+ ItemPointerSet(&(tuple->t_self), scan->rs_cblock, lineoff);
- if (valid && key != NULL)
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
+ /*
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the block
+ */
+ visible = HeapTupleSatisfiesVisibility(tuple,
+ scan->rs_base.rs_snapshot,
+ scan->rs_cbuf);
+
+ HeapCheckForSerializableConflictOut(visible, scan->rs_base.rs_rd,
+ tuple, scan->rs_cbuf,
+ scan->rs_base.rs_snapshot);
+
+ if (!visible)
+ continue;
- if (valid)
- {
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- scan->rs_cindex = lineoff;
- return;
- }
- }
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- {
- --lpp; /* move back in this page's ItemId array */
- --lineoff;
- }
- else
- {
- ++lpp; /* move forward in this page's ItemId array */
- ++lineoff;
- }
+ LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
+ scan->rs_cindex = lineoff;
+ return;
}
/*
@@ -839,39 +823,16 @@ heapgettup(HeapScanDesc scan,
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
block = heapgettup_advance_page(scan, block, dir);
+ }
- /*
- * return NULL if we've exhausted all the pages
- */
- if (block == InvalidBlockNumber)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
-
- LockBuffer(scan->rs_cbuf, BUFFER_LOCK_SHARE);
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page);
- linesleft = PageGetMaxOffsetNumber(page);
- if (backward)
- {
- lineoff = linesleft;
- lpp = PageGetItemId(page, linesleft);
- }
- else
- {
- lineoff = FirstOffsetNumber;
- lpp = PageGetItemId(page, FirstOffsetNumber);
- }
- }
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
/* ----------------
@@ -894,13 +855,10 @@ heapgettup_pagemode(HeapScanDesc scan,
ScanKey key)
{
HeapTuple tuple = &(scan->rs_ctup);
- bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
Page page;
int lineindex;
- OffsetNumber lineoff;
int linesleft;
- ItemId lpp;
if (unlikely(ScanDirectionIsNoMovement(dir)))
{
@@ -912,18 +870,12 @@ heapgettup_pagemode(HeapScanDesc scan,
{
block = heapgettup_initial_page(scan, dir);
- if (block == InvalidBlockNumber)
- {
- Assert(!BufferIsValid(scan->rs_cbuf));
- tuple->t_data = NULL;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- linesleft = scan->rs_ntuples;
- lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+ /*
+ * If parallel and other processes have already finished the scan, the
+ * returned block is expected to be InvalidBlockNumber. In this case,
+ * ensure that the backend is not sitting on a valid buffer.
+ */
+ Assert(block != InvalidBlockNumber || !BufferIsValid(scan->rs_cbuf));
}
else
{
@@ -938,84 +890,66 @@ heapgettup_pagemode(HeapScanDesc scan,
linesleft = scan->rs_ntuples - lineindex;
else
linesleft = scan->rs_cindex;
- }
- /*
- * block and lineindex now reference the next or previous visible tid
- */
+ /* block and lineindex now reference the next or previous visible tid */
+ goto continue_page;
+ }
/*
* advance the scan until we find a qualifying tuple or run out of stuff
* to scan
*/
- for (;;)
+ while (block != InvalidBlockNumber)
{
- while (linesleft > 0)
+ heapgetpage((TableScanDesc) scan, block);
+ page = BufferGetPage(scan->rs_cbuf);
+ TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
+ linesleft = scan->rs_ntuples;
+ lineindex = ScanDirectionIsForward(dir) ? 0 : linesleft - 1;
+
+ /* block and lineindex now reference the next or previous visible tid */
+
+ continue_page:
+
+ for (; linesleft > 0; linesleft--, lineindex += dir)
{
+ ItemId lpp;
+ OffsetNumber lineoff;
+
lineoff = scan->rs_vistuples[lineindex];
lpp = PageGetItemId(page, lineoff);
Assert(ItemIdIsNormal(lpp));
- tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+ tuple->t_data = (HeapTupleHeader) PageGetItem((Page) page, lpp);
tuple->t_len = ItemIdGetLength(lpp);
ItemPointerSet(&(tuple->t_self), block, lineoff);
/*
- * if current tuple qualifies, return it.
- */
- if (key != NULL)
- {
- bool valid;
-
- valid = HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
- nkeys, key);
- if (valid)
- {
- scan->rs_cindex = lineindex;
- return;
- }
- }
- else
- {
- scan->rs_cindex = lineindex;
- return;
- }
+ * if current tuple qualifies, return it.
+ * otherwise move to the next item on the block
+ */
+ if (key && !HeapKeyTest(tuple, RelationGetDescr(scan->rs_base.rs_rd),
+ nkeys, key))
+ continue;
- /*
- * otherwise move to the next item on the page
- */
- --linesleft;
- if (backward)
- --lineindex;
- else
- ++lineindex;
+ scan->rs_cindex = lineindex;
+ return;
}
- block = heapgettup_advance_page(scan, block, dir);
/*
- * return NULL if we've exhausted all the pages
+ * if we get here, it means we've exhausted the items on this page and
+ * it's time to move to the next.
*/
- if (block == InvalidBlockNumber)
- {
- if (BufferIsValid(scan->rs_cbuf))
- ReleaseBuffer(scan->rs_cbuf);
- scan->rs_cbuf = InvalidBuffer;
- scan->rs_cblock = InvalidBlockNumber;
- tuple->t_data = NULL;
- scan->rs_inited = false;
- return;
- }
-
- heapgetpage((TableScanDesc) scan, block);
-
- page = BufferGetPage(scan->rs_cbuf);
- TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page);
- linesleft = scan->rs_ntuples;
- if (backward)
- lineindex = linesleft - 1;
- else
- lineindex = 0;
+ block = heapgettup_advance_page(scan, block, dir);
}
+
+ /* end of scan */
+ if (BufferIsValid(scan->rs_cbuf))
+ ReleaseBuffer(scan->rs_cbuf);
+ scan->rs_cbuf = InvalidBuffer;
+ scan->rs_cblock = InvalidBlockNumber;
+ tuple->t_data = NULL;
+ scan->rs_inited = false;
}
--
2.34.1
[text/x-patch] v3-0006-Add-heapgettup_advance_page.patch (6.8K, ../../CAAKRu_bWPFT8ZX5pTw0nm_rpBrivSjB3wMntqnHNDwJAV1LdsA@mail.gmail.com/8-v3-0006-Add-heapgettup_advance_page.patch)
download | inline diff:
From bc7332551da42cc1798b23f422c8d06f9057713a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 30 Nov 2022 15:06:34 -0500
Subject: [PATCH v3 6/7] Add heapgettup_advance_page()
---
src/backend/access/heap/heapam.c | 168 +++++++++++++------------------
1 file changed, 72 insertions(+), 96 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f9b2d5cadb..f1081ab529 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -634,6 +634,74 @@ heapgettup_continue_page(HeapScanDesc scan, BlockNumber block, ScanDirection
return page;
}
+static inline BlockNumber
+heapgettup_advance_page(HeapScanDesc scan, BlockNumber block, ScanDirection dir)
+{
+ if (ScanDirectionIsBackward(dir))
+ {
+ if (block == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ if (block == 0)
+ block = scan->rs_nblocks;
+
+ block--;
+
+ return block;
+ }
+ else if (scan->rs_base.rs_parallel != NULL)
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
+ scan->rs_parallelworkerdata, (ParallelBlockTableScanDesc)
+ scan->rs_base.rs_parallel);
+
+ return block;
+ }
+ else
+ {
+ Assert(ScanDirectionIsForward(dir));
+
+ block++;
+
+ if (block >= scan->rs_nblocks)
+ block = 0;
+
+ if (block == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
+ /*
+ * Report our new scan position for synchronization purposes. We
+ * don't do that when moving backwards, however. That would just
+ * mess up any other forward-moving scanners.
+ *
+ * Note: we do this before checking for end of scan so that the
+ * final state of the position hint is back at the start of the
+ * rel. That's not strictly necessary, but otherwise when you run
+ * the same query multiple times the starting position would shift
+ * a little bit backwards on every invocation, which is confusing.
+ * We don't guarantee any specific ordering in general, though.
+ */
+ if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
+ ss_report_location(scan->rs_base.rs_rd, block);
+
+ return block;
+ }
+}
+
/* ----------------
* heapgettup - fetch next heap tuple
*
@@ -667,7 +735,6 @@ heapgettup(HeapScanDesc scan,
Snapshot snapshot = scan->rs_base.rs_snapshot;
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
OffsetNumber lineoff;
int linesleft;
@@ -771,56 +838,12 @@ heapgettup(HeapScanDesc scan,
*/
LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
- /*
- * advance to next/prior page and detect end of scan
- */
- if (backward)
- {
- finished = (block == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (block == 0)
- block = scan->rs_nblocks;
- block--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (block == InvalidBlockNumber);
- }
- else
- {
- block++;
- if (block >= scan->rs_nblocks)
- block = 0;
- finished = (block == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, block);
- }
+ block = heapgettup_advance_page(scan, block, dir);
/*
* return NULL if we've exhausted all the pages
*/
- if (finished)
+ if (block == InvalidBlockNumber)
{
if (BufferIsValid(scan->rs_cbuf))
ReleaseBuffer(scan->rs_cbuf);
@@ -873,7 +896,6 @@ heapgettup_pagemode(HeapScanDesc scan,
HeapTuple tuple = &(scan->rs_ctup);
bool backward = ScanDirectionIsBackward(dir);
BlockNumber block;
- bool finished;
Page page;
int lineindex;
OffsetNumber lineoff;
@@ -969,57 +991,11 @@ heapgettup_pagemode(HeapScanDesc scan,
++lineindex;
}
- /*
- * if we get here, it means we've exhausted the items on this page and
- * it's time to move to the next.
- */
- if (backward)
- {
- finished = (block == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
- if (block == 0)
- block = scan->rs_nblocks;
- block--;
- }
- else if (scan->rs_base.rs_parallel != NULL)
- {
- ParallelBlockTableScanDesc pbscan =
- (ParallelBlockTableScanDesc) scan->rs_base.rs_parallel;
- ParallelBlockTableScanWorker pbscanwork =
- scan->rs_parallelworkerdata;
-
- block = table_block_parallelscan_nextpage(scan->rs_base.rs_rd,
- pbscanwork, pbscan);
- finished = (block == InvalidBlockNumber);
- }
- else
- {
- block++;
- if (block >= scan->rs_nblocks)
- block = 0;
- finished = (block == scan->rs_startblock) ||
- (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);
-
- /*
- * Report our new scan position for synchronization purposes. We
- * don't do that when moving backwards, however. That would just
- * mess up any other forward-moving scanners.
- *
- * Note: we do this before checking for end of scan so that the
- * final state of the position hint is back at the start of the
- * rel. That's not strictly necessary, but otherwise when you run
- * the same query multiple times the starting position would shift
- * a little bit backwards on every invocation, which is confusing.
- * We don't guarantee any specific ordering in general, though.
- */
- if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
- ss_report_location(scan->rs_base.rs_rd, block);
- }
-
+ block = heapgettup_advance_page(scan, block, dir);
/*
* return NULL if we've exhausted all the pages
*/
- if (finished)
+ if (block == InvalidBlockNumber)
{
if (BufferIsValid(scan->rs_cbuf))
ReleaseBuffer(scan->rs_cbuf);
--
2.34.1
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-11-30 22:34 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-10-31 18:37 heapgettup refactoring Melanie Plageman <[email protected]>
2022-10-31 22:37 ` Justin Pryzby <[email protected]>
2022-11-01 01:09 ` Andres Freund <[email protected]>
2022-11-04 15:51 ` Melanie Plageman <[email protected]>
2022-11-16 15:49 ` Peter Eisentraut <[email protected]>
2022-11-30 22:34 ` Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox