On Thu, Mar 6, 2025 at 5:33 PM Peter Smith <smithpb2250@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
