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

On Fri, Mar 29, 2024 at 4:21 PM John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
>
> On Thu, Mar 28, 2024 at 12:55 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > I think the patch is in good shape. Do you have other comments or
> > suggestions, John?
>
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -1918,11 +1918,6 @@ include_dir 'conf.d'
> too high. It may be useful to control for this by separately
> setting <xref linkend="guc-autovacuum-work-mem"/>.
> </para>
> - <para>
> - Note that for the collection of dead tuple identifiers,
> - <command>VACUUM</command> is only able to utilize up to a maximum of
> - <literal>1GB</literal> of memory.
> - </para>
> </listitem>
> </varlistentry>
>
> This is mentioned twice for two different GUCs -- need to remove the
> other one, too.

Good catch, removed.

> Other than that, I just have minor nits:
>
> - * The major space usage for vacuuming is storage for the array of dead TIDs
> + * The major space usage for vacuuming is TID store, a storage for dead TIDs
>
> I think I've helped edit this sentence before, but I still don't quite
> like it. I'm thinking now "is storage for the dead tuple IDs".
>
> - * set upper bounds on the number of TIDs we can keep track of at once.
> + * set upper bounds on the maximum memory that can be used for keeping track
> + * of dead TIDs at once.
>
> I think "maximum" is redundant with "upper bounds".

Fixed.

>
> I also feel the commit message needs more "meat" -- we need to clearly
> narrate the features and benefits. I've attached how I would write it,
> but feel free to use what you like to match your taste.

Well, that's much better than mine.

>
> I've marked it Ready for Committer.

Thank you! I've attached the patch that I'm going to push tomorrow.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com