On Thu, Mar 6, 2025 at 5:33 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Some minor review comments for patch v10-0001. > > ====== > src/include/access/tableam.h > > 1. > struct IndexInfo; > +struct ParallelVacuumState; > +struct ParallelContext; > +struct ParallelWorkerContext; > struct SampleScanState; > > Use alphabetical order for consistency with existing code. > > ~~~ > > 2. > + /* > + * Estimate the size of shared memory that the parallel table vacuum needs > + * for AM > + * > > 2a. > Missing period (.) > > 2b. > Change the wording to be like below, for consistency with the other > 'estimate' function comments, and for consistency with the comment > where this function is implemented. > > Estimate the size of shared memory needed for a parallel table vacuum > of this relation. > > ~~~ > > 3. > + * The state_out is the output parameter so that an arbitrary data can be > + * passed to the subsequent callback, parallel_vacuum_remove_dead_items. > > Typo? "an arbitrary data" > > ~~~ > > 4. General/Asserts > > All the below functions have a comment saying "Not called if parallel > table vacuum is disabled." > - parallel_vacuum_estimate > - parallel_vacuum_initialize > - parallel_vacuum_initialize_worker > - parallel_vacuum_collect_dead_items > > But, it's only a comment. I wondered if they should all have some > Assert as an integrity check on that. > > ~~~ > > 5. > +/* > + * Return the number of parallel workers for a parallel vacuum scan of this > + * relation. > + */ > > "Return the number" or "Compute the number"? > The comment should match the comment in the fwd declaration of this function. > > ~~~ > > 6. > +/* > + * Perform a parallel vacuums scan to collect dead items. > + */ > > 6a. > "Perform" or "Execute"? > The comment should match the one in the fwd declaration of this function. > > 6b. > Typo "vacuums" >
Thank you for reviewing the patch. I'll address these comments and submit the updated version patches soon. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
