Обсуждение: Add support for INDEX_CLEANUP and TRUNCATE to vacuumdb

Hi hackers,

I quickly put together a patch to add INDEX_CLEANUP and TRUNCATE to
vacuumdb before noticing a previous thread for it [0].  My  take on it
was to just name the options --skip-index-cleanup and --skip-truncate.
While that does not give you a direct mapping to the corresponding
VACUUM options, it simplifies the patch by avoiding the boolean
parameter parsing stuff altogether.

Nathan

[0] https://postgr.es/m/CAHGQGwENx3Kvxq0U%2BwkGAdoAd89iaaWo_Pd5LBPUO4AqqhgyYQ%40mail.gmail.com

On Thu, Jun 11, 2020 at 12:41:17AM +0000, Bossart, Nathan wrote:
> I quickly put together a patch to add INDEX_CLEANUP and TRUNCATE to
> vacuumdb before noticing a previous thread for it [0].  My  take on it
> was to just name the options --skip-index-cleanup and --skip-truncate.
> While that does not give you a direct mapping to the corresponding
> VACUUM options, it simplifies the patch by avoiding the boolean
> parameter parsing stuff altogether.

Cannot blame you for that.  There is little sense to have a pure
mapping with the options here with some custom boolean parsing.  What
about naming them --no-index-cleanup and --no-truncate instead?  I
would suggest to track the option values with variables named like
do_truncate and do_index_cleanup.  That would be similar with what we
do with --no-sync for example.
--
Michael

On Thu, 11 Jun 2020 at 09:41, Bossart, Nathan <bossartn@amazon.com> wrote:
>
> Hi hackers,
>
> I quickly put together a patch to add INDEX_CLEANUP and TRUNCATE to
> vacuumdb before noticing a previous thread for it [0].  My  take on it
> was to just name the options --skip-index-cleanup and --skip-truncate.
> While that does not give you a direct mapping to the corresponding
> VACUUM options, it simplifies the patch by avoiding the boolean
> parameter parsing stuff altogether.
>

Thank you for updating the patch!

I looked at this patch.

@@ -412,6 +434,13 @@ vacuum_one_database(const char *dbname,
vacuumingOptions *vacopts,
        exit(1);
    }

+   if (vacopts->skip_index_cleanup && PQserverVersion(conn) < 120000)
+   {
+       PQfinish(conn);
+       pg_log_error("cannot use the \"%s\" option on server versions
older than PostgreSQL %s",
+                    "skip-index-cleanup", "12");
+   }
+
    if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
    {
        PQfinish(conn);

exit(1) is missing after pg_log_error().

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thanks for the quick feedback.  Here is a new patch.

Nathan

On 6/11/20, 10:13 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
> Thanks for the quick feedback.  Here is a new patch.

It looks like I missed a couple of tags in the documentation changes.
That should be fixed in v3.

Nathan

On Thu, Jun 18, 2020 at 09:26:50PM +0000, Bossart, Nathan wrote:
> It looks like I missed a couple of tags in the documentation changes.
> That should be fixed in v3.

Thanks.  This flavor looks good to me in terms of code, and the test
coverage is what's needed for all the code paths added.  This version
is using my suggestion of upthread for the option names: --no-truncate
and --no-index-cleanup.  Are people fine with this choice?
--
Michael

On Fri, Jun 19, 2020 at 10:57:01AM +0900, Michael Paquier wrote:
> Thanks.  This flavor looks good to me in terms of code, and the test
> coverage is what's needed for all the code paths added.  This version
> is using my suggestion of upthread for the option names: --no-truncate
> and --no-index-cleanup.  Are people fine with this choice?

Okay.  I have gone through the patch again, and applied it as of
9550ea3.  Thanks.
--
Michael

On 6/21/20, 9:36 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Okay.  I have gone through the patch again, and applied it as of
> 9550ea3.  Thanks.

Thanks!

Nathan