Re: [PoC] Improve dead tuple storage for lazy vacuum

On Mon, Jan 23, 2023 at 6:00 PM John Naylor
<john(dot)naylor(at)enterprisedb(dot)com> wrote:
>
> Attached is a rebase to fix conflicts from recent commits.

I have reviewed v22-0022* patch and I have some comments.

1.
>It also changes to the column names max_dead_tuples and num_dead_tuples and to
>show the progress information in bytes.

I think this statement needs to be rephrased.

2.

/*
* vac_tid_reaped() -- is a particular tid deletable?
*
* This has the right signature to be an IndexBulkDeleteCallback.
*
* Assumes dead_items array is sorted (in ascending TID order).
*/

I think this comment 'Assumes dead_items array is sorted' is not valid anymore.

3.

We are changing the min value of 'maintenance_work_mem' to 2MB. Should
we do the same for the 'autovacuum_work_mem'?

4.
+
+ /* collected LP_DEAD items including existing LP_DEAD items */
+ int lpdead_items;
+ OffsetNumber deadoffsets[MaxHeapTuplesPerPage];

We are actually collecting dead offsets but the variable name says
'lpdead_items' instead of something like ndeadoffsets num_deadoffsets.
And the comment is also saying dead items.

5.
/*
* lazy_vacuum_heap_page() -- free page's LP_DEAD items listed in the
* vacrel->dead_items array.
*
* Caller must have an exclusive buffer lock on the buffer (though a full
* cleanup lock is also acceptable). vmbuffer must be valid and already have
* a pin on blkno's visibility map page.
*
* index is an offset into the vacrel->dead_items array for the first listed
* LP_DEAD item on the page. The return value is the first index immediately
* after all LP_DEAD items for the same page in the array.
*/

This comment needs to be changed as this is referring to the
'vacrel->dead_items array' which no longer exists.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com