On Thu, 2 Feb 2023 at 10:12, Melanie Plageman <melanieplage...@gmail.com> wrote: > FWIW, I like setting rs_inited in heapgettup_initial_block() better in > the final refactor, but I agree with you that in this patch on its own > it is better in the body of heapgettup() and heapgettup_pagemode().
We can reconsider that when we get to that patch. It just felt a bit ugly to add an InvalidBlockNumber check after calling table_block_parallelscan_nextpage() > I believe this else is superfluous since we returned above. TBH, that's on purpose. I felt that it just looked better that way as the code all fitted onto my screen. I think if the function was longer and people had to scroll down to read it, it can often be better to return and reduce the nesting. This allows you to mentally not that a certain case is handled above. However, since all these helper functions seem to fit onto a screen without too much trouble, it just seems better (to me) if they all follow the format that I mentioned earlier. I might live to regret that as we often see get-rid-of-useless-else-clause patches coming up. I'm starting to wonder if someone's got some alarm that goes off every time one gets committed, but we'll see. I'd much rather have consistency between the helper functions than save a few bytes on tab characters. It would be different if the indentation were shifting things too far right, but that's not going to happen in a function that all fits onto a screen at once. I've attached a version of the next patch in the series. I admit to making a couple of adjustments. I couldn't bring myself to remove the snapshot local variable in this commit. We can deal with that when we come to it in whichever patch that needs to be changed in. The only other thing I really did was question your use of rs_cindex to store the last OffsetNumber. I ended up adding a new field which slots into the padding between a bool and BlockNumber field named rs_coffset for this purpose. I noticed what Andres wrote [1] earlier in this thread about that, so thought we should move away from looking at the last tuple to get this number I've attached the rebased and updated patch. David [1] https://postgr.es/m/20221101010948.hsf33emgnwzvi...@awork3.anarazel.de
From 32652b312d0c802e3105c064c24ff5b99694399c Mon Sep 17 00:00:00 2001 From: David Rowley <dgrow...@gmail.com> Date: Thu, 2 Feb 2023 15:07:27 +1300 Subject: [PATCH v9] Further refactor of heapgettup and heapgettup_pagemode Backward and forward scans share much of the same page acquisition code. Here we consolidate that code to reduce some duplication. Additionally, add a new rs_coffset field to HeapScanDescData to track the offset of the current tuple. The new field fits nicely into the padding between a bool and BlockNumber field and saves having to look at the last returned tuple to figure out which offset we should be looking at for the current tuple. Author: Melanie Plageman Reviewed-by: David Rowley Discussion: https://postgr.es/m/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY=_t8qeqzqol02rpi...@mail.gmail.com --- src/backend/access/heap/heapam.c | 200 ++++++++++--------------------- src/include/access/heapam.h | 1 + 2 files changed, 63 insertions(+), 138 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index b34e20a71f..ec6b7962c5 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -576,101 +576,67 @@ heapgettup(HeapScanDesc scan, BlockNumber block; bool finished; Page page; - int lines; OffsetNumber lineoff; int linesleft; ItemId lpp; - /* - * calculate next starting lineoff, given scan direction - */ - if (ScanDirectionIsForward(dir)) + if (unlikely(!scan->rs_inited)) { - if (!scan->rs_inited) - { - block = heapgettup_initial_block(scan, dir); + block = heapgettup_initial_block(scan, dir); - /* - * Check if we have reached the end of the scan already. This - * could happen if the table is empty or if the parallel workers - * have already finished the scan before we did anything ourselves - */ - if (block == InvalidBlockNumber) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } - heapgetpage((TableScanDesc) scan, block); - lineoff = FirstOffsetNumber; /* first offnum */ - scan->rs_inited = true; - } - else + /* + * Check if we have reached the end of the scan already. This could + * happen if the table is empty or if the parallel workers have + * already finished the scan before we did anything ourselves + */ + 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) - FirstOffsetNumber + 1; + + if (ScanDirectionIsForward(dir)) + lineoff = FirstOffsetNumber; /* first offnum */ + else + lineoff = (OffsetNumber) linesleft; + + scan->rs_inited = true; } else { - if (!scan->rs_inited) - { - block = heapgettup_initial_block(scan, dir); - - /* - * Check if we have reached the end of the scan already. This - * could happen if the table is empty. - */ - if (block == InvalidBlockNumber) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } + /* continue from previously returned page/tuple */ + block = scan->rs_cblock; - heapgetpage((TableScanDesc) scan, block); - 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); - page = BufferGetPage(scan->rs_cbuf); - TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page); - lines = PageGetMaxOffsetNumber(page); - lineoff = lines; /* final offnum */ - scan->rs_inited = true; + if (ScanDirectionIsForward(dir)) + { + lineoff = OffsetNumberNext(scan->rs_coffset); + 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_coffset)); + linesleft = lineoff; } - /* block and lineoff now reference the physically previous tid */ - - linesleft = lineoff; } /* @@ -715,6 +681,7 @@ heapgettup(HeapScanDesc scan, if (valid) { LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK); + scan->rs_coffset = lineoff; return; } } @@ -807,12 +774,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 { @@ -846,87 +812,46 @@ heapgettup_pagemode(HeapScanDesc scan, BlockNumber block; bool finished; Page page; - int lines; int lineindex; OffsetNumber lineoff; int linesleft; ItemId lpp; - /* - * calculate next starting lineindex, given scan direction - */ - if (ScanDirectionIsForward(dir)) + if (unlikely(!scan->rs_inited)) { - if (!scan->rs_inited) - { - block = heapgettup_initial_block(scan, dir); + block = heapgettup_initial_block(scan, dir); - /* - * Check if we have reached the end of the scan already. This - * could happen if the table is empty or if the parallel workers - * have already finished the scan before we did anything ourselves - */ - if (block == InvalidBlockNumber) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } - heapgetpage((TableScanDesc) scan, block); - lineindex = 0; - scan->rs_inited = true; - } - else + /* + * Check if we have reached the end of the scan already. This could + * happen if the table is empty or if the other workers in a parallel + * scan have already finished the scan. + */ + 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; + scan->rs_inited = true; } else { - if (!scan->rs_inited) - { - block = heapgettup_initial_block(scan, dir); - - /* - * Check if we have reached the end of the scan already. This - * could happen if the table is empty. - */ - if (block == InvalidBlockNumber) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } + /* 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); - 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; - scan->rs_inited = true; - } + 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; } /* @@ -1041,10 +966,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 417108f1e0..8d28bc93ef 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -57,6 +57,7 @@ typedef struct HeapScanDescData /* scan current state */ bool rs_inited; /* false = scan not init'd yet */ + OffsetNumber rs_coffset; /* current offset # in non-page-at-a-time mode */ BlockNumber rs_cblock; /* current block # in scan, if any */ Buffer rs_cbuf; /* current buffer in scan, if any */ /* NB: if rs_cbuf is not InvalidBuffer, we hold a pin on that buffer */ -- 2.39.0.windows.1
