Обсуждение: Pass heaprel to GlobalVisTestFor() in vacuumRedirectAndPlaceholder()

hi hackers,

now that the heap relation is passed down to vacuumRedirectAndPlaceholder()
thanks to 61b313e47e, we can also pass it down to GlobalVisTestFor() too (to
allow more pruning).

Please find attached a tiny patch to do so.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Sun, Apr 2, 2023 at 1:25 AM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
> now that the heap relation is passed down to vacuumRedirectAndPlaceholder()
> thanks to 61b313e47e, we can also pass it down to GlobalVisTestFor() too (to
> allow more pruning).

What about BTPageIsRecyclable() and _bt_pendingfsm_finalize()?

Making nbtree page deletion more efficient when logical decoding is in
use seems well worthwhile. There is an "XXX" comment about this issue,
similar to the SP-GiST one. It looks like you already have everything
you need to make this work from yesterday's commit 61b313e47e.

--
Peter Geoghegan

On Sun, Apr 2, 2023 at 10:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
> Making nbtree page deletion more efficient when logical decoding is in
> use seems well worthwhile. There is an "XXX" comment about this issue,
> similar to the SP-GiST one. It looks like you already have everything
> you need to make this work from yesterday's commit 61b313e47e.

Actually, I suppose that isn't quite true, since you'd still need to
find a way to pass the heap relation down to nbtree VACUUM. Say by
adding it to IndexVacuumInfo.

That doesn't seem hard at all. The hard part was passing the heap rel
down to _bt_getbuf(), which you've already taken care of.

--
Peter Geoghegan

Hi,

On 2023-04-02 10:22:18 -0700, Peter Geoghegan wrote:
> On Sun, Apr 2, 2023 at 10:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > Making nbtree page deletion more efficient when logical decoding is in
> > use seems well worthwhile. There is an "XXX" comment about this issue,
> > similar to the SP-GiST one. It looks like you already have everything
> > you need to make this work from yesterday's commit 61b313e47e.

+1


> Actually, I suppose that isn't quite true, since you'd still need to
> find a way to pass the heap relation down to nbtree VACUUM. Say by
> adding it to IndexVacuumInfo.

It has been added to that already, so it should really be as trivial as you
suggested earlier...

Greetings,

Andres Freund

On Sun, Apr 2, 2023 at 3:30 PM Andres Freund <andres@anarazel.de> wrote:
> > Actually, I suppose that isn't quite true, since you'd still need to
> > find a way to pass the heap relation down to nbtree VACUUM. Say by
> > adding it to IndexVacuumInfo.
>
> It has been added to that already, so it should really be as trivial as you
> suggested earlier...

Oh yeah, I missed it because you put it at the end of the struct,
rather than at the start, next to the existing Relation.

This page deletion issue matters a lot more after the Postgres 14
optimization added by commit e5d8a99903, which came after your
GlobalVisCheckRemovableFullXid() snapshot scalability work (well, a
few months after, at least). I really don't like the idea of something
like that being much less effective due to logical decoding. Granted,
the optimization in commit e5d8a99903 was itself kind of a hack, which
should be replaced by a scheme that explicitly makes recycle safety
the responsibility of the FSM itself, not the responsibility of
VACUUM.

--
Peter Geoghegan

Hi,

On 2023-04-02 15:52:14 -0700, Peter Geoghegan wrote:
> On Sun, Apr 2, 2023 at 3:30 PM Andres Freund <andres@anarazel.de> wrote:
> > > Actually, I suppose that isn't quite true, since you'd still need to
> > > find a way to pass the heap relation down to nbtree VACUUM. Say by
> > > adding it to IndexVacuumInfo.
> >
> > It has been added to that already, so it should really be as trivial as you
> > suggested earlier...
> 
> Oh yeah, I missed it because you put it at the end of the struct,
> rather than at the start, next to the existing Relation.

Well, Bertrand. But I didn't change it, so you're not wrong...


> This page deletion issue matters a lot more after the Postgres 14
> optimization added by commit e5d8a99903, which came after your
> GlobalVisCheckRemovableFullXid() snapshot scalability work (well, a
> few months after, at least). I really don't like the idea of something
> like that being much less effective due to logical decoding.

Yea, it's definitely good to use the relation there.

Greetings,

Andres Freund

Hi,

On 4/3/23 12:30 AM, Andres Freund wrote:
> Hi,
> 
> On 2023-04-02 10:22:18 -0700, Peter Geoghegan wrote:
>> On Sun, Apr 2, 2023 at 10:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
>>> Making nbtree page deletion more efficient when logical decoding is in
>>> use seems well worthwhile. There is an "XXX" comment about this issue,
>>> similar to the SP-GiST one. It looks like you already have everything
>>> you need to make this work from yesterday's commit 61b313e47e.
> 
> +1
> 

Thanks Peter for the suggestion!

> 
>> Actually, I suppose that isn't quite true, since you'd still need to
>> find a way to pass the heap relation down to nbtree VACUUM. Say by
>> adding it to IndexVacuumInfo.
> 
> It has been added to that already, so it should really be as trivial as you
> suggested earlier...
> 

Right. Please find enclosed V2 also taking care of BTPageIsRecyclable()
and _bt_pendingfsm_finalize().

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Apr 3, 2023 at 12:09 AM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
> Right. Please find enclosed V2 also taking care of BTPageIsRecyclable()
> and _bt_pendingfsm_finalize().

Pushed that as too separate patches just now. Thanks.

BTW, I'm not overly happy about the extent of the changes to nbtree
from commit 61b313e4. I understand that it was necessary to pass down
a heaprel in a lot of places, which is bound to create a lot of churn.
However, a lot of the churn from the commit seems completely
avoidable. There is no reason why the BT_READ path in _bt_getbuf()
could possibly require a valid heaprel. In fact, most individual
BT_WRITE calls don't need heaprel, either -- only those that pass
P_NEW. The changes affecting places like _bt_mkscankey() and
_bt_metaversion() seem particularly bad.

Anyway, I'll take care of this myself at some point after feature freeze.

--
Peter Geoghegan