Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM
 on-access)
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAAKRu_Y1MuANdm1p47Ev13Y9EQz8z+pw-vHOh=3DVdahUTjgXg@mail.gmail.com>
Date: Tue, 3 Mar 2026 15:32:29 +0800
Cc: Andres Freund <andres@anarazel.de>,
 Andrey Borodin <x4mmm@yandex-team.ru>,
 Kirill Reshke <reshkekirill@gmail.com>,
 Xuneng Zhou <xunengzhou@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Heikki Linnakangas <hlinnaka@iki.fi>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BD8B69E7-26D8-4706-9164-597C6AE57812@gmail.com>
References: <6BC5DBAB-6084-4BB8-8450-52E9648AB021@gmail.com>
 <CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
 <7F5BCD7A-764D-4D8D-8E27-6F2CAAEA1CEE@gmail.com>
 <CAAKRu_a04jbDACwzRYwzDND31aPyf7Yvz9TAZrTr=+F5bK1aVA@mail.gmail.com>
 <CALdSSPjcv25jmXm29X-MRWZBae6+HwcWfVH1PE8NfD=EMTnkAg@mail.gmail.com>
 <CAAKRu_bwtBEzDwemyim1r6yYonw7FTyFr1HXG8vywCe-MdbPBQ@mail.gmail.com>
 <4379FDA3-9446-4E2C-9C15-32EFE8D4F31B@yandex-team.ru>
 <CAAKRu_YQd=2KvomM+RHcpeDKj0bq+peJ=3W-fip+pkvzA-Jq9w@mail.gmail.com>
 <7ib3sa55sapwjlaz4sijbiq7iezna27kjvvvar4dpgkmadml6t@gfpkkwmdnepx>
 <CAAKRu_bs+gZ83QDacmBxunPvCGnXJ05hxP2BDPJ3BGwdbGRXzg@mail.gmail.com>
 <bqc4kh5midfn44gnjiqez3bjqv4zogydguvdn446riw45jcf3y@4ez66il7ebvk>
 <CAAKRu_Y1MuANdm1p47Ev13Y9EQz8z+pw-vHOh=3DVdahUTjgXg@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/BD8B69E7-26D8-4706-9164-597C6AE57812%40gmail.com>
Precedence: bulk


> On Mar 3, 2026, at 07:38, Melanie Plageman <melanieplageman@gmail.com> =
wrote:
>=20
> On Fri, Feb 20, 2026 at 12:59=E2=80=AFPM Andres Freund =
<andres@anarazel.de> wrote:
>>=20
>> On 2026-01-28 18:16:10 -0500, Melanie Plageman wrote:
>>=20
>>> I could see an argument for moving identify_and_fix_vm_corruption()
>>> out of the helper and into heap_page_prune_and_freeze() but then =
we'd
>>> have to move visibilitymap_get_status() out too. And that takes away =
a
>>> lot of the benefit of encapsulating all that logic.
>>=20
>> I was wondering about that option. Relatedly, I also was wondering if =
we ought
>> to do identify_and_fix_vm_corruption() regardless of =
->attempt_update_vm.
>=20
> Attached v35 does this. I always pin the vmbuffer if we are going to
> prune in heap_page_prune_opt(). In many cases, because it's saved in
> the scan descriptor, it won't actually need to take a new pin. During
> pruning, I check for VM corruption even if I am not considering
> setting the VM.
>=20
>>> Well, after this patch set, clearing the VM does happen before we =
emit
>>> WAL for pruning.
>>=20
>> That I think is a substantial improvement, the current (i.e. before =
your
>> series) placement really is pretty insane due to the guaranteed =
divergence it
>> causes.
>>=20
>> I wonder if we actually should just force an FPI whenever we detect =
such
>> corruption, that way it would reliably fixed on the standby as well.
>=20
> Only problem is we would have to do an FPI of the VM page as well if
> we wanted the corruption to be reliably fixed on the standby.
>=20
>>> It wouldn't be hard to move the corruption fixups to the beginning =
of
>>> heap_page_prune_and_freeze() in the new code structure.
>>=20
>> As identify_and_fix_vm_corruption() needs lpdead_items, I'm not sure =
that's
>> true?
>>=20
>> I wonder if at least the warning for the =
"(PageIsAllVisible(heap_page) &&
>> nlpdead_items > 0)" test should be moved to
>> heap_prune_record_dead_or_unused(). That way the WARNING could =
include the
>> offset number and it'd also work in the mark_unused_now case.
>>=20
>> Perhaps it also should trigger for RECENTLY_DEAD, INSERT_IN_PROGRESS,
>> DELETE_IN_PROGRESS?
>>=20
>> At that point the !page_all_visible && vm_all_visible part could =
indeed be
>> moved to the start of heap_page_prune_and_freeze()
>=20
> I've done all this. There is heap page/VM corruption check at the
> beginning of heap_page_prune_and_freeze() and then checking for
> corruption during pruning in the previously covered case (lpdead
> items) as well as the mark_unused_now case, and
> RECENTLY_DEAD/INSERT_IN_PROGRESS/DELETE_IN_PROGRESS.
>=20
>>> Would it be worth it? What benefit would we get? Do you just feel =
that it
>>> should logically come first?
>>=20
>> One insanity is that right now we will process all frozen pages over =
and over
>> due to he skip pages threshold, wasting a *lot* of CPU and memory =
bandwidth.
>> It'd be quite defensible to just skip processing the page once we =
determined
>> it's already all frozen.  But for that we'd probably want to do the
>> "page_all_visible && vm_all_visible" check before returning...
>=20
> I've added a fast path to bypass pruning/freezing when the page is
> already all-visible. And I check for pg_all_visible && vm_all_visible
> beforehand. The one downside this has is if there is a page marked
> all-frozen but has dead tuples on it, we'll never get to fix that
> corruption nor clean up the dead tuples. But the fast path kind of
> seems worth it to me.
>=20
>>>> Do we actually forsee a case where only one of =
HEAP_PAGE_PRUNE_FREEZE |
>>>> HEAP_PAGE_PRUNE_UPDATE_VM would be set?
>>>=20
>>> Yes, when setting the VM on-access, it is too expensive to call
>>> heap_prepare_freeze_tuple() on each tuple. I could work on trying to
>>> optimize it, but it isn't currently viable.
>>=20
>> Is it too expensive to do so even when we already decided to do some =
pruning?
>> I am not surprised it's too expensive when there's not even a dead =
tuple on
>> the page.  But I am mildly surprised if it's too expensive to do when =
we'd WAL
>> log anyway?
>=20
> It's not really possible in the current code structure to only call
> heap_prepare_freeze_tuple() when there are at least some prunable
> tuples. We go through the line pointers and record them as prunable at
> the same time we call heap_prepare_freeze_tuple(), so we won't know
> until we've examined all line pointers that there are no prunable
> tuples, at which point we will have called heap_prepare_freeze_tuple()
> for every tuple.
>=20
>>> I think using all_frozen_except_dead while maintaining
>>> visibility_cutoff_xid (in heap_prune_record_unchanged_lp_normal()) =
has
>>> the potential to be confusing, though. We'd need to keep updating
>>> visibility_cutoff_xid when all_visible is false but
>>> all_frozen_except_dead is true as well as when all_visible is true.
>>> And because we don't care about all_visible_except_dead, it gets =
even
>>> more confusing to make sure we are maintaining the right variables =
in
>>> the right situations.
>>=20
>> I suspect we should just track all of the horizons/cutoffs all the =
time. This
>> whole stuff about optimizing out a few conditional assignments =
complicates the
>> code substantially and feels extremely error prone to me.
>=20
> I've done this in v35. I posted the freeze horizon tracking patch
> separately in [1] but it is in v35 as 0004. Tracking the newest live
> xid is in 0009. This also always tracks all_visible for all callers
> since I unconditionally pass the vmbuffer now. I still don't set the
> VM if the query is modifying the relation, though.
>=20
>> I probably complained about this before, and it's not this patch's =
fault, but
>> PruneState->{all_visible,all_frozen} are imo confusingly named, due =
to
>> sounding like they describe the current state, rather than the =
possible state
>> after pruning.  It's not helped by this comment:
>>=20
>>         * NOTE: all_visible and all_frozen initially don't include =
LP_DEAD items.
>>         * That's convenient for heap_page_prune_and_freeze() to use =
them to
>>         * decide whether to opportunistically freeze the page or not. =
 The
>>         * all_visible and all_frozen values ultimately used to set =
the VM are
>>         * adjusted to include LP_DEAD items after we determine =
whether or not to
>>         * opportunistically freeze.
>>=20
>> "all-visible ... are adjusted to include LP_DEAD" ... - just reading =
that it's
>> hard to know what it means.
>=20
> 0003 does the rename.
>=20
>> The first thing to improve pruning performance that I would do is to =
introduce
>> a fastpath for pages that a) area already frozen b) do not have dead =
items (if
>> we're not freezing). Iterating through HOT chains is far from cheap, =
and if
>> all rows are live, there's not really a point in doing so.  This is
>> particulary important for VACUUMs where we end up freezing a ton of =
pages that
>> are already frozen, due to the silly skip_pages_threshold thing.
>=20
> 0007 adds a fast path.
>=20
>>> +static TransactionId
>>> +get_conflict_xid(bool do_prune, bool do_freeze, bool do_set_vm,
>>> +                              uint8 old_vmbits, uint8 new_vmbits,
>>> +                              TransactionId latest_xid_removed, =
TransactionId frz_conflict_horizon,
>>> +                              TransactionId visibility_cutoff_xid)
>>> +{
>>> +     TransactionId conflict_xid;
>>> +
>>> +     /*
>>> +      * We can omit the snapshot conflict horizon if we are not =
pruning or
>>> +      * freezing any tuples and are setting an already all-visible =
page
>>> +      * all-frozen in the VM.
>>=20
>> Maybe mention when this can happen, because it's not immediately =
obvious.
>=20
> I've added this to my TODO. I honestly can't think of a scenario where
> it can happen. But I remember spending quite a bit of time thinking
> about it on another occasion. The current code (in master) does
> specifically account for this scenario, which is why I kept the logic,
> but I'm not sure how it can happen.
>=20
> I made all the other changes to specific comments you mentioned in
> your mail but I won't bore you with itemization.
>=20
>>>      if (do_set_vm)
>>>              conflict_xid =3D visibility_cutoff_xid;
>>>      else if (do_freeze)
>>>              conflict_xid =3D frz_conflict_horizon;
>>>      else
>>>              conflict_xid =3D InvalidTransactionId;
>>=20
>> Could it be worth checking that if (do_set_vm && do_freeze) the
>> frz_conflict_horizon won't "violated" by using visibility_cutoff_xid =
instead?
>=20
> Yes, as you mentioned off-list, this wasn't right. New code is like =
this
>=20
> TransactionId conflict_xid =3D InvalidTransactionId;
> ...
>    if (do_set_vm)
>        conflict_xid =3D newest_live_xid;
>    if (do_freeze && TransactionIdFollows(newest_frozen_xid, =
conflict_xid))
>        conflict_xid =3D newest_frozen_xid;
>=20
>>> =46rom 8d350868206456f631883a40a955dff480e408d3 Mon Sep 17 00:00:00 =
2001
>>> From: Melanie Plageman <melanieplageman@gmail.com>
>>> Date: Wed, 17 Dec 2025 16:51:05 -0500
>>> Subject: [PATCH v34 09/14] Use GlobalVisState in vacuum to determine =
page
>>> level visibility
>>>=20
>>> [...]
>>>=20
>>> Because comparing a transaction ID against GlobalVisState is more
>>> expensive than comparing against a single XID, we defer this check =
until
>>> after scanning all tuples on the page.
>>=20
>> Curious, is this a precaution or was this a measurable bottleneck?
>=20
> I did see GlobalVisTestXidMaybeRunning() in a profile I did when it
> was still called for every HEAPTUPLE_LIVE tuple in
> heap_prune_record_unchanged_lp_normal(), but I don't have the profile
> or test case around anymore.
>=20
> However, since I now unconditionally maintain the newest_live_xid,
> moving GlobalVisTestXidMaybeRunning() back into
> heap_prune_record_unchanged_lp_normal() wouldn't help us avoid any
> work. It would just make the values of prstate.set_all_visible and
> prstate.set_all_frozen more accurate sooner. But I don't think it's
> worth the extra function call since set_all_frozen and set_all_visible
> won't be totally "done" until after we decide whether or not to
> opportunistically freeze anyway.
>=20
>>> @@ -1077,6 +1078,24 @@ heap_page_prune_and_freeze(PruneFreezeParams =
*params,
>>>      prune_freeze_plan(RelationGetRelid(params->relation),
>>>                                        buffer, &prstate, off_loc);
>>>=20
>>> +     /*
>>> +      * After processing all the live tuples on the page, if the =
newest xmin
>>> +      * amongst them may be considered running by any snapshot, the =
page cannot
>>> +      * be all-visible.
>>> +      */
>>> +     if (prstate.all_visible &&
>>> +             TransactionIdIsNormal(prstate.visibility_cutoff_xid) =
&&
>>=20
>> Any reason to test IsNormal rather than just IsValid()?  There should =
never be
>> a reason it's a valid but not "normal" xid, right?
>=20
> Well the reason I did this was that the existing code in master
> tracking visibility_cutoff_xid only advances it if
> TransactionIdIsNormal(). I'm a bit confused about it too because it
> seems like we would still want to do it for bootstrap mode xids. But I
> see PageSetPrunable() only allows normal xids.
>=20
>>> @@ -1794,28 +1812,15 @@ heap_prune_record_unchanged_lp_normal(Page =
page, PruneState *prstate, OffsetNumb
>>>                              }
>>>=20
>>>                              /*
>>> -                              * The inserter definitely committed.  =
But is it old enough
>>> -                              * that everyone sees it as committed? =
 A FrozenTransactionId
>>> -                              * is seen as committed to everyone.  =
Otherwise, we check if
>>> -                              * there is a snapshot that considers =
this xid to still be
>>> -                              * running, and if so, we don't =
consider the page all-visible.
>>> +                              * The inserter definitely committed. =
But we don't know if it
>>> +                              * is old enough that everyone sees it =
as committed. Later,
>>> +                              * after processing all the tuples on =
the page, we'll check if
>>> +                              * there is any snapshot that still =
considers the newest xid
>>> +                              * on the page to be running. If so, =
we don't consider the
>>> +                              * page all-visible.
>>>                               */
>>>                              xmin =3D HeapTupleHeaderGetXmin(htup);
>>>=20
>>> -                             /*
>>> -                              * For now always use prstate->cutoffs =
for this test, because
>>> -                              * we only update 'all_visible' and =
'all_frozen' when freezing
>>> -                              * is requested. We could use =
GlobalVisTestIsRemovableXid
>>> -                              * instead, if a non-freezing caller =
wanted to set the VM bit.
>>> -                              */
>>> -                             Assert(prstate->cutoffs);
>>> -                             if (!TransactionIdPrecedes(xmin, =
prstate->cutoffs->OldestXmin))
>>> -                             {
>>> -                                     prstate->all_visible =3D =
false;
>>> -                                     prstate->all_frozen =3D false;
>>> -                                     break;
>>> -                             }
>>> -
>>>                              /* Track newest xmin on page. */
>>>                              if (TransactionIdFollows(xmin, =
prstate->visibility_cutoff_xid) &&
>>>                                      TransactionIdIsNormal(xmin))
>>=20
>> Kinda wonder if this cod eshould be in something like
>> heap_prune_record_freezable() or such, rather than be inside
>> heap_prune_record_unchanged_lp_normal().
>=20
> I played around with it, but it all felt a bit awkward. I wrote it
> down for a future enhancement idea.
>=20
>>> Subject: [PATCH v34 10/14] Unset all_visible sooner if not freezing
>>>=20
>>> In the prune/freeze path, we currently delay clearing all_visible =
and
>>> all_frozen in the presence of dead items to allow opportunistic
>>> freezing.
>>>=20
>>> However, if no freezing will be attempted, there=E2=80=99s no need =
to delay.
>>> Clearing the flags earlier avoids extra bookkeeping in
>>> heap_prune_record_unchanged_lp_normal(). This currently has no =
runtime
>>> effect because all callers that consider setting the VM also prepare
>>> freeze plans, but upcoming changes will allow on-access pruning to =
set
>>> the VM without freezing. The extra bookkeeping was noticeable in a
>>> profile of on-access VM setting.
>>=20
>> What workload was that?
>=20
> It was a select * offset all query with a few fat tuples on each page
> and none of them prunable. I'm planning on digging up the
> case/creating a new one to see if it is reproducible. This was with an
> older version of the code that had more conditionals as well. This
> commit is actually dropped in v35 because I now always keep
> newest_live_xid up-to-date (0009) which means unsetting
> set_all_visible sooner has no benefit.
>=20
>> Theoretically, even if we don't freeze, the page still may be =
all-visible or
>> all frozen after the removal of dead items, no? Practically that =
won't happen,
>> because we don't remove dead items in any of the relevant paths, but =
from the
>> commit message and comments that's not entirely clear.
>=20
> Yea, it's clearer with the commit dropped.
>=20
>>> @@ -678,6 +678,12 @@ typedef struct EState
>>>                                                                      =
 * ExecDoInitialPruning() */
>>>      const char *es_sourceText;      /* Source text from QueryDesc =
*/
>>>=20
>>> +     /*
>>> +      * RT indexes of relations modified by the query through a
>>> +      * UPDATE/DELETE/INSERT/MERGE or targeted by a SELECT FOR =
UPDATE.
>>> +      */
>>> +     Bitmapset  *es_modified_relids;
>>> +
>>=20
>> Other EState fields are initialized in CreateExecutorState, this =
isn't afaict?
>=20
> Oops, yes. I based it on es_unpruned_relids which wasn't initialized
> there either. I've added a commit (0013) to initialize a few EState
> fields that weren't initialized in CreateExecutorState() as well.
>=20
>> Wonder if it's worth adding a crosscheck somewhere, verifying that if =
a
>> relation is modified, it's in es_modified_relids. Otherwise this =
could very
>> well silently get out of date.
>=20
> Done in v35 (0014).
>=20
>> Also, there's some overlap between the informtion collected this way, =
and
>> AcquireExecutorLocks(), ScanQueryForLocks(), which determine the =
needed lock
>> modes via rte->rellockmode.
>=20
> Those are in parser/planner, so it doesn't seem like a good fit. I
> populate es_modified_relids in the executor.
>=20
> I don't know exactly what the overlap would be between RTEs with an
> exclusive rellockmode and es_modified_relids. It seems like you could
> have RTEs which don't end up getting modified that have a lock level
> that would have made you think that they would be modified.
>=20
> But were you imagining a substitution or a cross-check?
>=20
>>> =46rom 8205b2d7da0c3ad3cbc5cead336ced677996b37d Mon Sep 17 00:00:00 =
2001
>>> From: Melanie Plageman <melanieplageman@gmail.com>
>>> Date: Wed, 3 Dec 2025 15:12:18 -0500
>>> Subject: [PATCH v34 12/14] Pass down information on table =
modification to scan
>>> node
>>=20
>> Perhaps worth splitting up, so the addition of the 0 flag is separate =
from the
>> the read only hint aspect.
>=20
> Done.
>=20
> [1] =
https://www.postgresql.org/message-id/CAAKRu_bbaUV8OUjAfVa_iALgKnTSfB4gO3j=
nkfpcFgrxEpSGJQ%40mail.gmail.com
> =
<v35-0001-Move-commonly-used-context-into-PruneState-and-s.patch><v35-0002=
-Add-PageGetPruneXid-helper.patch><v35-0003-Rename-PruneState-all_visible-=
all_frozen.patch><v35-0004-Use-the-newest-to-be-frozen-xid-as-the-conflict=
-.patch><v35-0005-Save-vmbuffer-in-heap-specific-scan-descriptors-.patch><=
v35-0006-Fix-visibility-map-corruption-in-more-cases.patch><v35-0007-Add-p=
runing-fast-path-for-all-visible-and-all-fr.patch><v35-0008-Use-GlobalVisS=
tate-in-vacuum-to-determine-page-l.patch><v35-0009-Keep-newest-live-XID-up=
-to-date-even-if-page-not.patch><v35-0010-Eliminate-XLOG_HEAP2_VISIBLE-fro=
m-vacuum-phase-I.patch><v35-0011-Eliminate-XLOG_HEAP2_VISIBLE-from-empty-p=
age-vac.patch><v35-0012-Remove-XLOG_HEAP2_VISIBLE-entirely.patch><v35-0013=
-Initialize-missing-fields-in-CreateExecutorState.patch><v35-0014-Track-wh=
ich-relations-are-modified-by-a-query.patch><v35-0015-Make-begin_scan-func=
tions-take-a-flags-argument.patch><v35-0016-Pass-down-information-on-table=
-modification-to-s.patch><v35-0017-Allow-on-access-pruning-to-set-pages-al=
l-visible.patch><v35-0018-Set-pd_prune_xid-on-insert.patch>

1 - 0001
```
+prune_freeze_plan(PruneState *prstate, OffsetNumber *off_loc)
 {
-	Page		page =3D BufferGetPage(buffer);
-	BlockNumber blockno =3D BufferGetBlockNumber(buffer);
-	OffsetNumber maxoff =3D PageGetMaxOffsetNumber(page);
+	Page		page =3D prstate->page;
+	BlockNumber blockno =3D prstate->block;
+	OffsetNumber maxoff =3D PageGetMaxOffsetNumber(prstate->page);
```

As there is a local =E2=80=9Cpage=E2=80=9D, maybe just use the local one =
for PageGetMaxOffsetNumber.

0002 looks good.

2 - 0003 - Does it make sense to also do the same renaming in =
PruneFreezeResult?

3 - 0004
```
-
-		/*
-		 * Calculate what the snapshot conflict horizon should =
be for a record
-		 * freezing tuples. We can use the visibility_cutoff_xid =
as our cutoff
-		 * for conflicts when the whole page is eligible to =
become all-frozen
-		 * in the VM once we're done with it. Otherwise, we =
generate a
-		 * conservative cutoff by stepping back from OldestXmin.
-		 */
-		if (prstate->set_all_frozen)
-			prstate->frz_conflict_horizon =3D =
prstate->visibility_cutoff_xid;
-		else
-		{
-			/* Avoids false conflicts when =
hot_standby_feedback in use */
-			prstate->frz_conflict_horizon =3D =
prstate->cutoffs->OldestXmin;
-			=
TransactionIdRetreat(prstate->frz_conflict_horizon);
-		}
+		=
Assert(TransactionIdPrecedesOrEquals(prstate->pagefrz.FreezePageConflictXi=
d,
+										=
	 prstate->cutoffs->OldestXmin));
```

At this point of Assert, can prstate->pagefrz.FreezePageConflictXid be =
InvalidTransactionId? My understanding is no, in that case, would it =
make sense to also Assert(prstate->pagefrz.FreezePageConflictXid !=3D =
InvalidTransactionId)?

Otherwise, if prstate->pagefrz.FreezePageConflictXid is still possibly =
be InvalidTransactionId, then the Assert should be changed to something =
like:

Assert(prstate->pagefrz.FreezePageConflictXid =3D=3D =
InvalidTransactionId ||=20
  TransactionIdPrecedesOrEquals(prstate->pagefrz.FreezePageConflictXid, =
prstate->cutoffs->OldestXmin)

I will continue with 0005 tomorrow.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/