Tomas Vondra writes:
> On 8/30/22 02:45, David Rowley wrote:
>> I think the existing sentinel check looks wrong:
>> if (!sentinel_ok(chunk, slab->chunkSize))
>> shouldn't that be passing the pointer rather than the chunk?
> I agree the check in SlabCheck() looks wrong, as it's ignoring the chunk
David Rowley writes:
> Here's v2 of the slab-fix patch.
> I've included the sentinel check fix. This passes make check-world
> for me when do a 32-bit build on my x86_64 machine and adjust
> pg_config.h to set MAXIMUM_ALIGNOF to 8.
> Any chance you could run make check-world on your 32-bit
On 8/30/22 02:45, David Rowley wrote:
> On Tue, 30 Aug 2022 at 03:39, Tomas Vondra
> wrote:
>> The attached patch seems to fix the issue for me - at least it seems
>> like that. This probably will need to get backpatched, I guess. Maybe we
>> should add an assert to MemoryChunkGetPointer to
On Tue, 30 Aug 2022 at 12:45, David Rowley wrote:
> I think the existing sentinel check looks wrong:
>
> if (!sentinel_ok(chunk, slab->chunkSize))
>
> shouldn't that be passing the pointer rather than the chunk?
Here's v2 of the slab-fix patch.
I've included the sentinel check fix. This passes
On Tue, 30 Aug 2022 at 12:22, Tomas Vondra
wrote:
> I also suggested doing a similar check in MemoryChunkGetPointer, so that
> we catch the issue earlier - right after we allocate the chunk. Any
> opinion on that?
I think it's probably a good idea. However, I'm not yet sure if we can
keep it as
On Tue, 30 Aug 2022 at 03:39, Tomas Vondra
wrote:
> The attached patch seems to fix the issue for me - at least it seems
> like that. This probably will need to get backpatched, I guess. Maybe we
> should add an assert to MemoryChunkGetPointer to check alignment?
Hi Tomas,
I just wanted to
On 8/29/22 23:57, Tom Lane wrote:
> David Rowley writes:
>> I've read over the emails and glanced at Tomas' patch. I think that
>> seems good. I think I'd rather see us do that than pad the struct out
>> further as Tomas' method is more aligned to what we do in aset.c
>> (ALLOC_BLOCKHDRSZ) and
David Rowley writes:
> I've read over the emails and glanced at Tomas' patch. I think that
> seems good. I think I'd rather see us do that than pad the struct out
> further as Tomas' method is more aligned to what we do in aset.c
> (ALLOC_BLOCKHDRSZ) and generation.c (Generation_BLOCKHDRSZ).
> I
On Tue, 30 Aug 2022 at 03:43, Tom Lane wrote:
> I think adding a padding field to SlabBlock would be a less messy
> solution than your patch.
Thank you both of you for looking at this while I was sleeping.
I've read over the emails and glanced at Tomas' patch. I think that
seems good. I think
On 8/29/22 17:43, Tom Lane wrote:
> Tomas Vondra writes:
>> I suspect it's a pre-existing bug in Slab allocator, because it does this:
>
>> #define SlabBlockGetChunk(slab, block, idx) \
>> ((MemoryChunk *) ((char *) (block) + sizeof(SlabBlock) \
>> +
Andres Freund writes:
> On 2022-08-29 11:43:14 -0400, Tom Lane wrote:
>> I think adding a padding field to SlabBlock would be a less messy
>> solution than your patch.
> That just seems to invite the same problem happening again later and it's
> harder to ensure that the padding is correct
Hi,
On 2022-08-29 11:43:14 -0400, Tom Lane wrote:
> Tomas Vondra writes:
> > I suspect it's a pre-existing bug in Slab allocator, because it does this:
>
> > #define SlabBlockGetChunk(slab, block, idx) \
> > ((MemoryChunk *) ((char *) (block) + sizeof(SlabBlock) \
> >
Tomas Vondra writes:
> I suspect it's a pre-existing bug in Slab allocator, because it does this:
> #define SlabBlockGetChunk(slab, block, idx) \
> ((MemoryChunk *) ((char *) (block) + sizeof(SlabBlock) \
> + (idx * slab->fullChunkSize)))
> and
On 8/29/22 17:27, Tomas Vondra wrote:
> ...
>
> I suspect it's a pre-existing bug in Slab allocator, because it does this:
>
> #define SlabBlockGetChunk(slab, block, idx) \
> ((MemoryChunk *) ((char *) (block) + sizeof(SlabBlock) \
> + (idx *
Got it: sizeof(SlabBlock) isn't a multiple of MAXALIGN,
(gdb) p sizeof(SlabBlock)
$4 = 20
but this coding requires it to be:
#define SlabBlockGetChunk(slab, block, idx) \
((MemoryChunk *) ((char *) (block) + sizeof(SlabBlock) \
+ (idx *
Robert Haas writes:
> I've got another problem with this patch here on macOS:
> In file included from aset.c:52:
> ../../../../src/include/utils/memutils_memorychunk.h:170:18: error:
> comparison of constant 7 with expression of type
> 'MemoryContextMethodID' (aka 'enum MemoryContextMethodID')
On 8/29/22 17:20, Tom Lane wrote:
> Tomas Vondra writes:
>> I can reproduce it on my system (rpi4 running 32-bit raspbian).
>
> Yeah, more or less same as what I'm testing on.
>
> Seeing that the complaint is about pfree'ing a non-maxaligned
> ReorderBufferChange pointer, I tried adding
>
>
I wrote:
> So the bug is in fact in David's changes, and it consists in palloc
> sometimes handing back non-maxaligned pointers. I find it mildly
> astonishing that we managed to get through core regression tests
> without such a fault surfacing, but there you have it.
Oh! I just noticed that
Tomas Vondra writes:
> I can reproduce it on my system (rpi4 running 32-bit raspbian).
Yeah, more or less same as what I'm testing on.
Seeing that the complaint is about pfree'ing a non-maxaligned
ReorderBufferChange pointer, I tried adding
diff --git
Hi,
I've got another problem with this patch here on macOS:
ccache clang -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla
-Werror=unguarded-availability-new -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv
Amit Kapila writes:
> Yeah, I also thought that way but couldn't find a reason. I think if
> David is able to reproduce it on one of his systems then he can try
> locally reverting both the commits one by one.
It seems to repro easily on any 32-bit platform. Aside from the
buildfarm results,
On 8/29/22 16:02, Amit Kapila wrote:
> On Mon, Aug 29, 2022 at 7:17 PM Tom Lane wrote:
>>
>> David Rowley writes:
>>> I suspect, going by all 3 failing animals being 32-bit which have a
>>> MAXIMUM_ALIGNOF 8 and SIZEOF_SIZE_T of 4 that this is due to the lack
>>> of padding in the MemoryChunk
On Mon, Aug 29, 2022 at 7:17 PM Tom Lane wrote:
>
> David Rowley writes:
> > I suspect, going by all 3 failing animals being 32-bit which have a
> > MAXIMUM_ALIGNOF 8 and SIZEOF_SIZE_T of 4 that this is due to the lack
> > of padding in the MemoryChunk struct.
> > AllocChunkData and
David Rowley writes:
> I suspect, going by all 3 failing animals being 32-bit which have a
> MAXIMUM_ALIGNOF 8 and SIZEOF_SIZE_T of 4 that this is due to the lack
> of padding in the MemoryChunk struct.
> AllocChunkData and GenerationChunk had padding to account for
> sizeof(Size) being 4 and
On Mon, 29 Aug 2022 at 21:37, Amit Kapila wrote:
> 2022-08-29 03:29:56.911 EDT [1056:67] pg_regress/ddl STATEMENT:
> SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> TRAP: FailedAssertion("pointer == (void *)
On Mon, 29 Aug 2022 at 21:37, Amit Kapila wrote:
> I am not completely sure if the failure is due to your commit but it
> was showing the line added by this commit. Note that I had also
> committed (commit id: d2169c9985) one patch today but couldn't
> correlate the failure with that so thought
On Mon, Aug 29, 2022 at 10:57 AM David Rowley wrote:
>
> After a bit more revision, mostly updating outdated comments and
> naming adjustments, I've pushed this.
>
> Per the benchmark results I showed in [1], due to the performance of
> having the AllocSet free list pointers stored at the end of
On Mon, 29 Aug 2022 at 10:39, David Rowley wrote:
> One more try to make CFbot happy.
After a bit more revision, mostly updating outdated comments and
naming adjustments, I've pushed this.
Per the benchmark results I showed in [1], due to the performance of
having the AllocSet free list
On Sun, 28 Aug 2022 at 23:04, David Rowley wrote:
> I'll try that one again...
One more try to make CFbot happy.
David
From 118022b089279ea7a6b1d43ca9a105a3203905e3 Mon Sep 17 00:00:00 2001
From: David Rowley
Date: Thu, 9 Jun 2022 20:09:22 +1200
Subject: [PATCH v8] Improve performance of and
On Fri, 26 Aug 2022 at 17:16, David Rowley wrote:
> The CFbot just alerted me to the cplusplus check was failing with the
> v5 patch, so here's v6.
I'll try that one again...
David
From 90e48eef5709e8cdfb474f8798fa0aef1d0158b1 Mon Sep 17 00:00:00 2001
From: David Rowley
Date: Thu, 9 Jun 2022
On Tue, 23 Aug 2022 at 21:03, David Rowley wrote:
> Finally, the v5 patch with the fixes mentioned above.
The CFbot just alerted me to the cplusplus check was failing with the
v5 patch, so here's v6.
> I'm pretty keen to get this patch committed early next week. This is
> quite core
On Wed, 10 Aug 2022 at 06:44, Andres Freund wrote:
> I think it's fine, given that we can change this at any time, but it's
> probably worth to explicitly agree that this will for now restrict us to 8
> context methods?
I know there was some discussion about this elsewhere in this thread
about
Thanks for giving this a look.
On Wed, 10 Aug 2022 at 02:37, Robert Haas wrote:
> # We also add a restriction that block sizes for all 3 of the memory
> # allocators cannot be 1GB or larger. We would be unable to store the
> # number of bytes that the block is offset from the chunk stored
David Rowley writes:
> On Wed, 10 Aug 2022 at 09:23, Tom Lane wrote:
>> Hmm, I suppose you mean we could reduce 4) if we needed to. Yeah, that
>> seems like a reasonable place to buy more bits later if we run out of
>> MemoryContextMethodIDs. Should be fine then.
> I think he means 3). If 4)
On Wed, 10 Aug 2022 at 09:23, Tom Lane wrote:
>
> Andres Freund writes:
> > On 2022-08-09 15:21:57 -0400, Tom Lane wrote:
> >> Do we really need it to be that tight? I know we only have 3 methods
> >> today,
> >> but 8 doesn't seem that far away. If there were six bits reserved for
> >> this
Andres Freund writes:
> On 2022-08-09 15:21:57 -0400, Tom Lane wrote:
>> Do we really need it to be that tight? I know we only have 3 methods today,
>> but 8 doesn't seem that far away. If there were six bits reserved for
>> this I'd be happier.
> We only have so many bits available, so that'd
Hi,
On 2022-08-09 15:21:57 -0400, Tom Lane wrote:
> Andres Freund writes:
> > I think it's fine, given that we can change this at any time, but it's
> > probably worth to explicitly agree that this will for now restrict us to 8
> > context methods?
>
> Do we really need it to be that tight? I
On 2022-Aug-09, Andres Freund wrote:
> Mildly wondering whether we ought to use designated initializers instead,
> given we're whacking it around already. Too easy to get the order wrong when
> adding new members, and we might want to have optional callbacks too.
Strong +1. It makes code much
Andres Freund writes:
> I think it's fine, given that we can change this at any time, but it's
> probably worth to explicitly agree that this will for now restrict us to 8
> context methods?
Do we really need it to be that tight? I know we only have 3 methods today,
but 8 doesn't seem that far
Hi,
On 2022-08-09 10:36:57 -0400, Robert Haas wrote:
> On Tue, Aug 9, 2022 at 8:53 AM David Rowley wrote:
> > I think the patch is now starting to take shape. I've added it to the
> > September commitfest [1].
>
> This is extremely cool. The memory savings are really nice.
+1
> And I also
On Tue, Aug 9, 2022 at 8:53 AM David Rowley wrote:
> I think the patch is now starting to take shape. I've added it to the
> September commitfest [1].
This is extremely cool. The memory savings are really nice. And I also
like this:
# Additionally, this commit completely changes the rule that
On Wed, 13 Jul 2022 at 17:20, David Rowley wrote:
> I did consider that in all cases where the allocations are above
> allocChunkLimit that the chunk is put on a dedicated block and in
> fact, the blockoffset is always the same for those. I wondered if we
> could use the full 60 bits for the
В Вт, 12/07/2022 в 22:41 -0700, Andres Freund пишет:
> Hi,
>
> On 2022-07-12 10:42:07 -0700, Andres Freund wrote:
> > On 2022-07-12 17:01:18 +1200, David Rowley wrote:
> > > There is at least one. It might be major; to reduce the AllocSet chunk
> > > header from 16 bytes down to 8 bytes I had to
Hi,
On 2022-07-12 10:42:07 -0700, Andres Freund wrote:
> On 2022-07-12 17:01:18 +1200, David Rowley wrote:
> > There is at least one. It might be major; to reduce the AllocSet chunk
> > header from 16 bytes down to 8 bytes I had to get rid of the freelist
> > pointer that was reusing the "aset"
On Wed, 13 Jul 2022 at 05:42, Andres Freund wrote:
> > There is at least one. It might be major; to reduce the AllocSet chunk
> > header from 16 bytes down to 8 bytes I had to get rid of the freelist
> > pointer that was reusing the "aset" field in the chunk header struct.
> > This works now by
On Wed, 13 Jul 2022 at 05:44, Andres Freund wrote:
> On 2022-07-12 20:22:57 +0300, Yura Sokolov wrote:
> > I don't get, why "large chunk" needs additional fields for size and
> > offset.
> > Large allocation sizes are certainly rounded to page size.
> > And allocations which doesn't fit 1GB we
Hi,
On 2022-07-12 20:22:57 +0300, Yura Sokolov wrote:
> I don't get, why "large chunk" needs additional fields for size and
> offset.
> Large allocation sizes are certainly rounded to page size.
> And allocations which doesn't fit 1GB we could easily round to 1MB.
> Then we could simply store
Hi,
On 2022-07-12 17:01:18 +1200, David Rowley wrote:
> I've taken Andres' patch and made some quite significant changes to
> it. In the patch's current state, the sort performance regression in
> PG15 vs PG14 is fixed. The generation.c context chunk header has been
> reduced to 8 bytes from the
Good day, David.
В Вт, 12/07/2022 в 17:01 +1200, David Rowley пишет:
> Over on [1], I highlighted that 40af10b57 (Use Generation memory
> contexts to store tuples in sorts) could cause some performance
> regressions for sorts when the size of the tuple is exactly a power of
> 2. The reason for
101 - 149 of 149 matches
Mail list logo
