On Mon, Jan 23, 2023 at 6:00 PM John Naylor <john.nay...@enterprisedb.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