public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber
4+ messages / 4 participants
[nested] [flat]
* [PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber
@ 2024-02-13 00:04 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Melanie Plageman @ 2024-02-13 00:04 UTC (permalink / raw)
BitmapAdjustPrefetchIterator() only used the blockno member of the
passed in TBMIterateResult to ensure that the prefetch iterator and
regular iterator stay in sync. Pass it the BlockNumber only. This will
allow us to move away from using the TBMIterateResult outside of table
AM specific code.
---
src/backend/executor/nodeBitmapHeapscan.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 5df3b5ca46..404de0595e 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -52,7 +52,7 @@
static TupleTableSlot *BitmapHeapNext(BitmapHeapScanState *node);
static inline void BitmapDoneInitializingSharedState(ParallelBitmapHeapState *pstate);
static inline void BitmapAdjustPrefetchIterator(BitmapHeapScanState *node,
- TBMIterateResult *tbmres);
+ BlockNumber blockno);
static inline void BitmapAdjustPrefetchTarget(BitmapHeapScanState *node);
static inline void BitmapPrefetch(BitmapHeapScanState *node,
TableScanDesc scan);
@@ -230,7 +230,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
break;
}
- BitmapAdjustPrefetchIterator(node, tbmres);
+ BitmapAdjustPrefetchIterator(node, tbmres->blockno);
valid = table_scan_bitmap_next_block(scan, tbmres);
@@ -341,7 +341,7 @@ BitmapDoneInitializingSharedState(ParallelBitmapHeapState *pstate)
*/
static inline void
BitmapAdjustPrefetchIterator(BitmapHeapScanState *node,
- TBMIterateResult *tbmres)
+ BlockNumber blockno)
{
#ifdef USE_PREFETCH
ParallelBitmapHeapState *pstate = node->pstate;
@@ -360,7 +360,7 @@ BitmapAdjustPrefetchIterator(BitmapHeapScanState *node,
/* Do not let the prefetch iterator get behind the main one */
TBMIterateResult *tbmpre = tbm_iterate(prefetch_iterator);
- if (tbmpre == NULL || tbmpre->blockno != tbmres->blockno)
+ if (tbmpre == NULL || tbmpre->blockno != blockno)
elog(ERROR, "prefetch and main iterators are out of sync");
}
return;
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0006-table_scan_bitmap_next_block-returns-lossy-or-exa.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation
@ 2026-02-26 13:52 David Rowley <[email protected]>
2026-02-26 15:01 ` Re: More speedups for tuple deformation Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: David Rowley @ 2026-02-26 13:52 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Andres Freund <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]>
Thanks for having a look at this.
On Wed, 25 Feb 2026 at 22:49, Álvaro Herrera <[email protected]> wrote:
> With all these patches applied, I get this from pahole for
> CompactAttribute,
> The 'attnullability' thing stands out. It probably doesn't make much of
> a difference -- considering that it's down to 8 bytes already and I
> doubt anything will change if it's instead 6 -- but given these
> definitions
We could get it to 6 bytes. The alignment is 2, so there's certainly a
memory saving there. I think the primary reason to make it 8 was that
the LEA instruction can be used without any other instructions to
calculate the address of the first CompactAttribute element to use.
LEA can shift the array index to multiply it by 1,2,4 or 8, and
because it can do shift and add concurrently, it can utilise the add
to add the offset of compact_attrs in the TupleDesc struct.
I see gcc doing this when I make attnum a size_t. This is effectively
what I described above:
1c46: 48 8d 74 c2 20 lea rsi,[rdx+rax*8+0x20]
The 0x20 is the offset to the start of compact_attrs in TupleDescData.
rax is attnum. rdx is the tupledesc. Effectively, that's:
rsi = ((char *) tupledesc) + (attnum << 3) + offsetof(TupleDescData,
compact_attrs);
If I make the CompactAttribute 6 bytes, I see gcc producing two LEA
instructions instead of one:
1c80: 48 8d 34 40 lea rsi,[rax+rax*2]
1c89: 49 8d 74 74 20 lea rsi,[r12+rsi*2+0x20]
rax is attnum again. r12 is the tupledesc. Effectively that's:
/* multiply attnum by 3, store in rdi */
rsi = attnum + (attnum << 1);
/* multiply rdi by 2 to make multiply by 6 and at that plus 32 to the
tupledesc's addr. */
rsi = ((char *) tupledesc) + (rsi << 1) + offsetof(TupleDescData,
compact_attrs);
I expect that the extra address calculation costs plus the extra
bitwise-AND for checking attbyval will cost more than it will save.
> Regarding deform_bench, I wonder if we shouldn't start a proper
> benchmarking suite (of which this would the first participant) by
> putting this in src/benchmark/tuple_deform instead (or something like
> that) instead of relegating it to src/test/modules. It doesn't make
> much of a technical difference, but it is a bit of a statement that more
> tools are welcome.
Yeah. Andres was talking about this in [1]. I'd originally not planned
on committing deform_bench at all and only put it in the current
location in the patch as that seemed like the best location for it
that we have today. Andres mentioned just having 1 extension and
adding functions to it. That seems fine to me. I have some other
things that could go in there. If it's just one extension, then
there's likely no need to make a special directory for it. Maybe
src/test/modules is fine. For now, I don't want to focus too much as
I'd rather get the deformation speedups ready and in.
David
[1] https://postgr.es/m/rbxc2qqhsvzxpukgd36caoa4ydgn5r22fxktxanrkn6nobg7j6%4027b4vogohgu2
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation
2026-02-26 13:52 Re: More speedups for tuple deformation David Rowley <[email protected]>
@ 2026-02-26 15:01 ` Álvaro Herrera <[email protected]>
2026-02-26 15:08 ` Re: More speedups for tuple deformation Andres Freund <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Álvaro Herrera @ 2026-02-26 15:01 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Andres Freund <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]>
On 2026-Feb-27, David Rowley wrote:
> > The 'attnullability' thing stands out. It probably doesn't make much of
> > a difference -- considering that it's down to 8 bytes already and I
> > doubt anything will change if it's instead 6 -- but given these
> > definitions
>
> We could get it to 6 bytes. [...]
> If I make the CompactAttribute 6 bytes, I see gcc producing two LEA
> instructions instead of one: [...]
> I expect that the extra address calculation costs plus the extra
> bitwise-AND for checking attbyval will cost more than it will save.
Ah, that makes sense. Interesting, thanks for explaining.
> > Regarding deform_bench, I wonder if we shouldn't start a proper
> > benchmarking suite [...]
>
> Yeah. Andres was talking about this in [1]. I'd originally not planned
> on committing deform_bench at all and only put it in the current
> location in the patch as that seemed like the best location for it
> that we have today. Andres mentioned just having 1 extension and
> adding functions to it. That seems fine to me. I have some other
> things that could go in there. If it's just one extension, then
> there's likely no need to make a special directory for it. Maybe
> src/test/modules is fine. For now, I don't want to focus too much as
> I'd rather get the deformation speedups ready and in.
Hmm, ok. I'd rather not get it in the tree at all for now, and see what
else can we get in there later. Then we might have better ideas of what
to do; if we do get it in src/test/modules now, we might not want to
move it elsewhere later as it might be more painful. As you say, if
it's just one extension then it doesn't matter much where it is. But we
might get other ideas that require more than one extension, or more than
one benchmarking suite, and then src/test/modules doesn't sound so great
a location.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: More speedups for tuple deformation
2026-02-26 13:52 Re: More speedups for tuple deformation David Rowley <[email protected]>
2026-02-26 15:01 ` Re: More speedups for tuple deformation Álvaro Herrera <[email protected]>
@ 2026-02-26 15:08 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Andres Freund @ 2026-02-26 15:08 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: David Rowley <[email protected]>; Zsolt Parragi <[email protected]>; John Naylor <[email protected]>; Chao Li <[email protected]>; PostgreSQL Developers <[email protected]>
Hi,
On 2026-02-26 16:01:26 +0100, Álvaro Herrera wrote:
> On 2026-Feb-27, David Rowley wrote:
> > > Regarding deform_bench, I wonder if we shouldn't start a proper
> > > benchmarking suite [...]
> >
> > Yeah. Andres was talking about this in [1]. I'd originally not planned
> > on committing deform_bench at all and only put it in the current
> > location in the patch as that seemed like the best location for it
> > that we have today. Andres mentioned just having 1 extension and
> > adding functions to it. That seems fine to me. I have some other
> > things that could go in there. If it's just one extension, then
> > there's likely no need to make a special directory for it. Maybe
> > src/test/modules is fine. For now, I don't want to focus too much as
> > I'd rather get the deformation speedups ready and in.
>
> Hmm, ok. I'd rather not get it in the tree at all for now, and see what
> else can we get in there later. Then we might have better ideas of what
> to do; if we do get it in src/test/modules now, we might not want to
> move it elsewhere later as it might be more painful. As you say, if
> it's just one extension then it doesn't matter much where it is. But we
> might get other ideas that require more than one extension, or more than
> one benchmarking suite, and then src/test/modules doesn't sound so great
> a location.
I don't get this. We'll approximately never have a full suite of benchmark
tools that make sense to integrate at once. The reason we're here is because
over and over we've not actually merged quite useful tools, making do with
benchmarking-by-proxy and one off tools. This seems like a recipe for never
getting anywhere better.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-02-26 15:08 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-02-13 00:04 [PATCH v9 05/17] Update BitmapAdjustPrefetchIterator parameter type to BlockNumber Melanie Plageman <[email protected]>
2026-02-26 13:52 Re: More speedups for tuple deformation David Rowley <[email protected]>
2026-02-26 15:01 ` Re: More speedups for tuple deformation Álvaro Herrera <[email protected]>
2026-02-26 15:08 ` Re: More speedups for tuple deformation Andres Freund <[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