From: | Melanie Plageman <melanieplageman(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, David Geier <geidav(dot)pg(at)gmail(dot)com> |
Subject: | Re: Eliminate redundant tuple visibility check in vacuum |
Date: | 2023-09-06 21:21:22 |
Message-ID: | CAAKRu_a0kgBKgkJzDM0z=BGa34JDKiNC9WjoYcwXorKRXTnJMA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Sep 6, 2023 at 1:04 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Aug 31, 2023 at 6:29 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> > I have changed this.
>
> I spent a bunch of time today looking at this, thinking maybe I could
> commit it. But then I got cold feet.
>
> With these patches applied, PruneResult ends up being declared in
> heapam.h, with a comment that says /* State returned from pruning */.
> But that comment isn't accurate. The two new members that get added to
> the structure by 0002, namely nnewlpdead and htsv, are in fact state
> that is returned from pruning. But the other 5 members aren't. They're
> just initialized to constant values by pruning and then filled in for
> real by the vacuum logic. That's extremely weird. It would be fine if
> heap_page_prune() just grew a new output argument that only returned
> the HTSV results, or perhaps it could make sense to bundle any
> existing out parameters together into a struct and then add new things
> to that struct instead of adding even more parameters to the function
> itself. But there doesn't seem to be any good reason to muddle
> together the new output parameters for heap_page_prune() with a bunch
> of state that is currently internal to vacuumlazy.c.
>
> I realize that the shape of the patches probably stems from the fact
> that they started out life as part of a bigger patch set. But to be
> committed independently, they need to be shaped in a way that makes
> sense independently, and I don't think this qualifies. On the plus
> side, it seems to me that it's probably not that much work to fix this
> issue and that the result would likely be a smaller patch than what
> you have now, which is something.
Yeah, I think this is a fair concern. I have addressed it in the
attached patches.
I thought a lot about whether or not adding a PruneResult which
contains only the output parameters and result of heap_page_prune() is
annoying since we have so many other *Prune* data structures. I
decided it's not annoying. In some cases, four outputs don't merit a
new structure. In this case, I think it declutters the code a bit --
independent of any other patches I may be writing :)
- Melanie
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Move-heap_page_prune-output-parameters-into-struc.patch | text/x-patch | 8.7 KB |
v3-0002-Reuse-heap_page_prune-tuple-visibility-statuses.patch | text/x-patch | 10.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2023-09-06 22:01:53 | Re: Performance degradation on concurrent COPY into a single relation in PG16. |
Previous Message | Daniel Gustafsson | 2023-09-06 20:23:55 | Re: GUC for temporarily disabling event triggers |