On Tue, Mar 3, 2026 at 2:33 AM Chao Li <[email protected]> wrote:
>
> 2 - 0003 - Does it make sense to also do the same renaming in
> PruneFreezeResult?
I could do that. Later commits remove them, so I thought it didn't
make sense. If only this commit goes in though, it would make sense.
> - * 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 =
> prstate->visibility_cutoff_xid;
> - else
> - {
> - /* Avoids false conflicts when hot_standby_feedback
> in use */
> - prstate->frz_conflict_horizon =
> prstate->cutoffs->OldestXmin;
> - TransactionIdRetreat(prstate->frz_conflict_horizon);
> - }
> +
> Assert(TransactionIdPrecedesOrEquals(prstate->pagefrz.FreezePageConflictXid,
> +
> 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 !=
> InvalidTransactionId)?
I think it is possible if we are doing some kind of freezing to a
multixact that we reach here and FreezePageConflictXid is
InvalidTransactionId.
> Otherwise, if prstate->pagefrz.FreezePageConflictXid is still possibly be
> InvalidTransactionId, then the Assert should be changed to something like:
>
> Assert(prstate->pagefrz.FreezePageConflictXid == InvalidTransactionId ||
> TransactionIdPrecedesOrEquals(prstate->pagefrz.FreezePageConflictXid,
> prstate->cutoffs->OldestXmin)
This is covered by TransactionIdPrecedesOrEquals because
InvalidTransactionId is 0. We assume that in many places throughout
the code.
> I will continue with 0005 tomorrow.
Thanks for the review!
I noticed a serious bug in v35-0017: I pass hscan->modifies_base_rel
to heap_page_prune_opt() as rel_read_only, which is the opposite of
what I want to do -- it should be !hscan->modifies_base_rel. I'm going
to wait to fix it though and post a new v36 once I've batched up more
fixups.
- Melanie
