On Wed, 8 Feb 2023 at 15:09, David Rowley wrote:
> Using the tests mentioned in [1], I tested out
> remove_HeapScanDescData_rs_inited_field.patch. It's not looking very
> promising at all.
In light of the performance regression from removing the rs_inited
field, let's just forget doing that for n
On Wed, 8 Feb 2023 at 09:41, Melanie Plageman wrote:
>
> On Tue, Feb 07, 2023 at 05:40:13PM +1300, David Rowley wrote:
> > I ended up adjusting HeapScanDescData more than what is minimally
> > required to remove rs_inited. I wondered if rs_cindex should be closer
> > to rs_cblock in the struct so
On Tue, Feb 07, 2023 at 05:40:13PM +1300, David Rowley wrote:
> On Fri, 3 Feb 2023 at 15:26, David Rowley wrote:
> > I've pushed all but the final 2 patches now.
>
> I just pushed the final patch in the series.
Cool!
> I held back on moving the setting of rs_inited back into the
> heapgettup_in
On Fri, 3 Feb 2023 at 15:26, David Rowley wrote:
> I've pushed all but the final 2 patches now.
I just pushed the final patch in the series. I held back on moving
the setting of rs_inited back into the heapgettup_initial_block()
helper function as I wondered if we should even keep that field.
I
On Fri, 3 Feb 2023 at 06:23, Melanie Plageman wrote:
> Your code also switches to saving the OffsetNumber -- just in a separate
> variable of OffsetNumber type. I am fine with this if it your rationale
> is that it is not a good idea to store a smaller number in a larger
> datatype. However, the b
On Thu, Feb 02, 2023 at 07:00:37PM +1300, David Rowley wrote:
> 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 whichev
On Thu, 2 Feb 2023 at 10:12, Melanie Plageman 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
On Thu, Feb 02, 2023 at 12:21:20AM +1300, David Rowley wrote:
> On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
> wrote:
> > v7 attached
>
> I've been looking over the v7-0002 patch today and I did make a few
> adjustments to heapgettup_initial_block() as I would prefer to see the
> branching of e
On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
wrote:
> v7 attached
I've been looking over the v7-0002 patch today and I did make a few
adjustments to heapgettup_initial_block() as I would prefer to see the
branching of each of all these helper functions follow the pattern of:
if ()
{
if ()
On Tue, 31 Jan 2023 at 12:18, Melanie Plageman
wrote:
>
> On Fri, Jan 27, 2023 at 10:34 PM David Rowley wrote:
> > 4. I think it might be a good idea to use unlikely() in if
> > (!scan->rs_inited). The idea is to help coax the compiler into moving
> > that code off to a cold path. That's likely e
v7 attached
On Fri, Jan 27, 2023 at 10:34 PM David Rowley wrote:
>
> "On Wed, 25 Jan 2023 at 10:17, Melanie Plageman
> wrote:
> > I've added a comment but I didn't include the function name in it -- I
> > find it repetitive when the comments above functions do that -- however,
> > I'm not strong
"On Wed, 25 Jan 2023 at 10:17, Melanie Plageman
wrote:
> I've added a comment but I didn't include the function name in it -- I
> find it repetitive when the comments above functions do that -- however,
> I'm not strongly attached to that.
I think the general format for header comments is:
/*
*
On Tue, Jan 24, 2023 at 04:17:23PM -0500, Melanie Plageman wrote:
> Thanks for taking a look!
>
> On Mon, Jan 23, 2023 at 6:08 AM David Rowley wrote:
> >
> > On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
> > wrote:
> > > In your v2 patch, you remove these assertions:
> > >
> > > - /* chec
Thanks for taking a look!
On Mon, Jan 23, 2023 at 6:08 AM David Rowley wrote:
>
> On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
> wrote:
> > In your v2 patch, you remove these assertions:
> >
> > - /* check that rs_cindex is in sync */
> > - Assert(scan->rs_cindex < scan->rs_ntuples
On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
wrote:
> In your v2 patch, you remove these assertions:
>
> - /* check that rs_cindex is in sync */
> - Assert(scan->rs_cindex < scan->rs_ntuples);
> - Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
>
> Is that intentional?
On 10.01.23 21:31, Melanie Plageman wrote:
On Thu, Jan 5, 2023 at 8:52 AM Peter Eisentraut
wrote:
Ok, let's look through these patches starting from the top then.
v4-0001-Add-no-movement-scan-helper.patch
This makes sense overall; there is clearly some duplicate code that can
be unified.
It
On Thu, Jan 5, 2023 at 8:52 AM Peter Eisentraut
wrote:
>
> Ok, let's look through these patches starting from the top then.
>
> v4-0001-Add-no-movement-scan-helper.patch
>
> This makes sense overall; there is clearly some duplicate code that can
> be unified.
>
> It appears that during your rebasi
On 03.01.23 21:39, Melanie Plageman wrote:
On 30.11.22 23:34, Melanie Plageman wrote:
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.
To keep this moving along a bit, I h
On Mon, Jan 2, 2023 at 5:23 AM Peter Eisentraut
wrote:
>
> On 30.11.22 23:34, Melanie Plageman wrote:
> > 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.
>
> To keep
On 30.11.22 23:34, Melanie Plageman wrote:
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.
To keep this moving along a bit, I have committed your 0002, which I
think is a
On Wed, Nov 16, 2022 at 10:49 AM Peter Eisentraut
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, d
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.
Pl
Thanks for the review!
Attached is v2 with feedback addressed.
On Mon, Oct 31, 2022 at 9:09 PM Andres Freund wrote:
> > From 9d8b01960463dc64ff5b111d523ff80fce3017af Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman
> > Date: Mon, 31 Oct 2022 13:40:06 -0400
> > Subject: [PATCH v1 2/3] Turn Hea
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 d
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
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 tha
26 matches
Mail list logo
