> On Mar 5, 2026, at 16:52, Chao Li <[email protected]> wrote:
>
>
>
>> On Mar 4, 2026, at 16:59, Chao Li <[email protected]> wrote:
>>
>>
>>
>>> On Mar 3, 2026, at 23:52, Melanie Plageman <[email protected]>
>>> wrote:
>>>
>>>
>>>> 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 understood that TransactionIdPrecedesOrEquals(InvalidTransactionId,
>> prstate->cutoffs->OldestXmin) is true, but that would leave an impression to
>> code readers that prstate->pagefrz.FreezePageConflictXid could not be
>> InvalidTransactionId. Thus I think my version explicitly tells that
>> prstate->pagefrz.FreezePageConflictXid could be InvalidTransactionId at the
>> point.
>>
>>
>>>> I will continue with 0005 tomorrow.
>>>
>>
>> 4 - 0005
>> ```
>> * Caller must have pin on the buffer, and must *not* have a lock on it.
>> */
>> void
>> -heap_page_prune_opt(Relation relation, Buffer buffer)
>> +heap_page_prune_opt(Relation relation, Buffer buffer, Buffer *vmbuffer)
>> ```
>>
>> I don’t see why vmbuffer has to be of pointer type. Buffer type is
>> underlying int, I checked the last commit, vmbuffer only passes in data into
>> the function without passing out anything.
>>
>> As we add the new parameter vmbuffer, though it’s not used in this commit, I
>> think it’d be better to update the header commit to explain what this
>> parameter will do.
>>
>> 5 - 0006
>> ```
>> + *
>> + * heap_fix_vm_corruption() makes changes to the VM and, potentially, the
>> heap
>> + * page, but it does not need to be done in a critical section because
>> + * clearing the VM is not WAL-logged.
>> + */
>> +static void
>> +heap_fix_vm_corruption(PruneState *prstate, OffsetNumber offnum)
>> ```
>>
>> Nit: why the last paragraph of the header comments uses the function name
>> instead of “this function”? Looks like a copy-pasto.
>>
>> 6 - 0006
>> ```
>> + if (prstate->lpdead_items > 0)
>> + {
>> + ereport(WARNING,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("LP_DEAD item found on page marked as all-visible"),
>> + errdetail("relation \"%s\", page %u, tuple %u",
>> + RelationGetRelationName(prstate->relation),
>> + prstate->block, offnum)));
>> + }
>> + else
>> + {
>> + ereport(WARNING,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("tuple not visible to all found on page marked as all-visible"),
>> + errdetail("relation \"%s\", page %u, tuple %u",
>> + RelationGetRelationName(prstate->relation),
>> + prstate->block, offnum)));
>> + }
>> ```
>>
>> I recently just learned that a detail message should use complete sentences,
>> and end each with a period, and capitalize the first word of sentences. See
>> https://www.postgresql.org/docs/current/error-style-guide.html.
>>
>> 7 - 0006
>> ```
>> + else if (prstate->vmbits & VISIBILITYMAP_VALID_BITS)
>> + {
>> + /*
>> + * As of PostgreSQL 9.2, the visibility map bit should never be set if
>> + * the page-level bit is clear. However, it's possible that the bit
>> + * got cleared after heap_vac_scan_next_block() was called, so we must
>> + * recheck with buffer lock before concluding that the VM is corrupt.
>> + */
>> + ereport(WARNING,
>> + (errcode(ERRCODE_DATA_CORRUPTED),
>> + errmsg("page %u in \"%s\" is not marked all-visible but visibility map bit
>> is set",
>> + prstate->block,
>> + RelationGetRelationName(prstate->relation))));
>> + }
>> ```
>>
>> The comment says “we must recheck with buffer lock before…”, but it only log
>> a warning message. Is the comment stale?
>>
>> 8 - 0007
>> ```
>> +static void
>> +heap_page_bypass_prune_freeze(PruneState *prstate, PruneFreezeResult
>> *presult)
>> +{
>> + OffsetNumber maxoff = PageGetMaxOffsetNumber(prstate->page);
>> + Page page = prstate->page;
>> +
>> + Assert(prstate->vmbits & VISIBILITYMAP_ALL_FROZEN ||
>> + (prstate->vmbits & VISIBILITYMAP_ALL_VISIBLE &&
>> + !prstate->attempt_freeze));
>> +
>> + /* We'll fill in presult for the caller */
>> + memset(presult, 0, sizeof(PruneFreezeResult));
>> +
>> + /*
>> + * Since the page is all-visible, a count of the normal ItemIds on the
>> + * page should be sufficient for vacuum's live tuple count.
>> + */
>> + for (OffsetNumber off = FirstOffsetNumber;
>> + off <= maxoff;
>> + off = OffsetNumberNext(off))
>> + {
>> + if (ItemIdIsNormal(PageGetItemId(page, off)))
>> + prstate->live_tuples++;
>> + }
>> +
>> + presult->live_tuples = prstate->live_tuples;
>> +
>> + /* Clear any stale prune hint */
>> + if (TransactionIdIsValid(PageGetPruneXid(page)))
>> + {
>> + PageClearPrunable(page);
>> + MarkBufferDirtyHint(prstate->buffer, true);
>> + }
>> +
>> + presult->vmbits = prstate->vmbits;
>> +
>> + if (!PageIsEmpty(page))
>> + presult->hastup = true;
>> +}
>> ```
>>
>> * Given this function has done PageIsEmpty(page), that that is true, we
>> don’t need to count live_tuples, right? That could be a tiny optimization.
>> * I see heap_page_bypass_prune_freeze() is only called in one place and
>> immediately after prune_freeze_setup() and heap_fix_vm_corruption(), so
>> prstate->vmbits must be 0, so do we need to do presult->vmbits =
>> prstate->vmbits;?
>> * Do we need to set all_visible and all_frozen to presult?
>>
>> 0008 LGTM
>>
>> I will continue with 0009 tomorrow.
>>
>
> 9 - 0009
> ···
> + * Currently, only VACUUM performs freezing, but other callers may in the
> + * future. Other callers must initialize prstate.all_frozen to false,
> ···
>
> Nit: prstate.all_frozen -> prstate.set_all_frozen
>
> I saw you have fixed this in 0010, but I think it’s better also fix it here.
>
> 10 - 0010
> ```
> + * Whether or not the page was newly set all-visible and all-frozen during
> + * phase I of vacuuming.
> */
> - uint8 vmbits;
> + BlockNumber new_all_visible_pages;
> + BlockNumber new_all_visible_frozen_pages;
> + BlockNumber new_all_frozen_pages;
> ```
>
> These 3 fields are actually counts rather than pointers to blocks, using type
> BlockNumber are quite confusing, though underlying BlockNumber is uint32. I
> think they can be just int type.
>
> 11 - 0010
> ```
> + BlockNumber new_all_visible_pages;
> + BlockNumber new_all_visible_frozen_pages;
> + BlockNumber new_all_frozen_pages;
> ```
>
> I don’t see where these 3 fields are initialized. In lazy_scan_prune(),
> presult is defined as:
> ```
> PruneFreezeResult presult;
> ```
> So, those fields will hold random values.
>
> 12 - 0010
> ```
> + * conflict would ahve been handled in reaction to the WAL record freezing
> ```
>
> Nit: ahve -> have
>
> 0011 LGTM
>
> 13 - 0012 - bufmask.c
> ```
> + * we don't mark the page all-visible. See heap_xlog_prune_and_freeze()
> + * for more details.
> ```
>
> I don’t find a function named heap_xlog_prune_and_freeze().
>
> 14 - 0012 - heapam_xlog.c
> ```
> + * same approach is taken when replaying XLOG_HEAP2_PRUNE* records (see
> + * heap_xlog_prune_and_freeze()).
> ```
>
> Same as 13.
>
> 0013 LGTM
>
> I will try to finish the rest 5 commits tomorrow.
>
15 - 0014 - execMain.c
```
@@ -3027,6 +3035,7 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
rcestate->es_range_table_size = parentestate->es_range_table_size;
rcestate->es_relations = parentestate->es_relations;
rcestate->es_rowmarks = parentestate->es_rowmarks;
+ rcestate->es_modified_relids = parentestate->es_modified_relids;
```
Here it just assigns the BMS pointer to rcestate->es_modified_relids. I am not
sure if further bms_add_member() will still happen, if yes, it might be safer
to do bms_copy(parentestate->es_modified_relids), because a further
bms_add_member() may cause a new memory allocated and the old pointer stale.
16 - 0014 - execUtils.c
```
for (rti = 1; rti <= estate->es_range_table_size; rti++)
```
Nit: I have seen several recent commits that performed cleanups to switch to
use for loop var like:
```
for (Index rti = 1; rti <= estate->es_range_table_size; rti++)
```
17 - 0015
The commit message subject line says “Make begin_scan() functions take a flags
argument”, where begin_scan() seems inaccurate, for example,
table_index_fetch_begin() is not “begin scan”.
Otherwise 0015 LGTM.
18 - 0016 - tableam.h
```
/* unregister snapshot at scan end? */
SO_TEMP_SNAPSHOT = 1 << 9,
+ /* set if the query doesn't modify the relation */
+ SO_HINT_REL_READ_ONLY = 1 << 10,
} ScanOptions;
```
Nit: maybe add an empty line before the new flag.
19 - 0017 - heapam_handler.c
```
@@ -147,7 +147,8 @@ heapam_index_fetch_tuple(struct IndexFetchTableData *scan,
*/
if (prev_buf != hscan->xs_cbuf)
heap_page_prune_opt(hscan->xs_base.rel, hscan->xs_cbuf,
-
&hscan->xs_vmbuffer);
+
&hscan->xs_vmbuffer,
+
hscan->modifies_base_rel);
```
This feels like a bug. heap_page_prune_opt takes the first parameter
rel_read_only, but hscan->modifies_base_rel means not read-only, so here we
should use “!hscan->modifies_base_rel”.
Oh, when I read back your previous email, you have found this bug.
20 - 0018
In heap_insert(), you do:
```
+ if (TransactionIdIsNormal(xid) && !(options & HEAP_INSERT_FROZEN))
+ PageSetPrunable(page, xid);
```
But in heap_multi_insert(), you do:
```
+ if (!all_frozen_set && TransactionIdIsNormal(xid))
+ PageSetPrunable(page, xid);
```
Is the option check " !(options & HEAP_INSERT_FROZEN))” also needed by
heap_multi_insert?
~~ Done of this round review ~~
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
