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
Attachments:
v1-0001-make-use-of-heaprel-in-vacuumRedirectAndPlaceholder.patchtext/plain; charset=UTF-8; name=v1-0001-make-use-of-heaprel-in-vacuumRedirectAndPlaceholder.patchDownload
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 3cff71e720..5a7e55441b 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -506,8 +506,7 @@ vacuumRedirectAndPlaceholder(Relation index, Relation heaprel, Buffer buffer)
xlrec.nToPlaceholder = 0;
xlrec.snapshotConflictHorizon = InvalidTransactionId;
- /* XXX: providing heap relation would allow more pruning */
- vistest = GlobalVisTestFor(NULL);
+ vistest = GlobalVisTestFor(heaprel);
START_CRIT_SECTION();
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
Attachments:
v2-0001-make-use-of-heaprel-in-vacuumRedirectAndPlaceholder.patchtext/plain; charset=UTF-8; name=v2-0001-make-use-of-heaprel-in-vacuumRedirectAndPlaceholder.patchDownload
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index ee996b5660..9f6659ae10 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -934,7 +934,7 @@ _bt_getbuf(Relation rel, Relation heaprel, BlockNumber blkno, int access)
return buf;
}
- if (BTPageIsRecyclable(page))
+ if (BTPageIsRecyclable(page, heaprel))
{
/*
* If we are generating WAL for Hot Standby then create a
@@ -2961,6 +2961,8 @@ void
_bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
{
IndexBulkDeleteResult *stats = vstate->stats;
+ IndexVacuumInfo *info = vstate->info;
+ Relation heaprel = info->heaprel;
Assert(stats->pages_newly_deleted >= vstate->npendingpages);
@@ -2993,7 +2995,7 @@ _bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
* essential; GlobalVisCheckRemovableFullXid() will not reliably recognize
* that it is now safe to recycle newly deleted pages without this step.
*/
- GetOldestNonRemovableTransactionId(NULL);
+ GetOldestNonRemovableTransactionId(heaprel);
for (int i = 0; i < vstate->npendingpages; i++)
{
@@ -3008,7 +3010,7 @@ _bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
* must be non-recyclable too, since _bt_pendingfsm_add() adds pages
* to the array in safexid order.
*/
- if (!GlobalVisCheckRemovableFullXid(NULL, safexid))
+ if (!GlobalVisCheckRemovableFullXid(heaprel, safexid))
break;
RecordFreeIndexPage(rel, target);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 97a39b0f65..409a2c1210 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -1039,6 +1039,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno)
IndexBulkDeleteCallback callback = vstate->callback;
void *callback_state = vstate->callback_state;
Relation rel = info->index;
+ Relation heaprel = info->heaprel;
bool attempt_pagedel;
BlockNumber blkno,
backtrack_to;
@@ -1124,7 +1125,7 @@ backtrack:
}
}
- if (!opaque || BTPageIsRecyclable(page))
+ if (!opaque || BTPageIsRecyclable(page, heaprel))
{
/* Okay to recycle this page (which could be leaf or internal) */
RecordFreeIndexPage(rel, blkno);
diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c
index 3cff71e720..5a7e55441b 100644
--- a/src/backend/access/spgist/spgvacuum.c
+++ b/src/backend/access/spgist/spgvacuum.c
@@ -506,8 +506,7 @@ vacuumRedirectAndPlaceholder(Relation index, Relation heaprel, Buffer buffer)
xlrec.nToPlaceholder = 0;
xlrec.snapshotConflictHorizon = InvalidTransactionId;
- /* XXX: providing heap relation would allow more pruning */
- vistest = GlobalVisTestFor(NULL);
+ vistest = GlobalVisTestFor(heaprel);
START_CRIT_SECTION();
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 6dee307042..953bf6586b 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -288,7 +288,7 @@ BTPageGetDeleteXid(Page page)
* well need special handling for new pages anyway.
*/
static inline bool
-BTPageIsRecyclable(Page page)
+BTPageIsRecyclable(Page page, Relation heaprel)
{
BTPageOpaque opaque;
@@ -307,12 +307,8 @@ BTPageIsRecyclable(Page page)
* For that check if the deletion XID could still be visible to
* anyone. If not, then no scan that's still in progress could have
* seen its downlink, and we can recycle it.
- *
- * XXX: If we had the heap relation we could be more aggressive about
- * recycling deleted pages in non-catalog relations. For now we just
- * pass NULL. That is at least simple and consistent.
*/
- return GlobalVisCheckRemovableFullXid(NULL, BTPageGetDeleteXid(page));
+ return GlobalVisCheckRemovableFullXid(heaprel, BTPageGetDeleteXid(page));
}
return false;
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