vacuumdb and new VACUUM options

sawada.mshk@gmail.com

over 6 years ago

In reply to: Fujii Masao (#1)

Re: vacuumdb and new VACUUM options

On Wed, May 8, 2019 at 2:41 AM Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

vacuumdb command supports the corresponding options to
any VACUUM parameters except INDEX_CLEANUP and TRUNCATE
that were added recently. Should vacuumdb also support those
new parameters, i.e., add --index-cleanup and --truncate options
to the command?

I think it's a good idea to add new options of these parameters for
vacuumdb. While making INDEX_CLEANUP option patch I also attached the
patch for INDEX_CLEANUP parameter before[1]0002 patch on /messages/by-id/CAD21AoBtM=HGLkMKBgch37mf0-epa3_o=Y1PU0_m9r5YmtS-NQ@mail.gmail.com, although it adds
--disable-index-cleanup option instead.

[1]: 0002 patch on /messages/by-id/CAD21AoBtM=HGLkMKBgch37mf0-epa3_o=Y1PU0_m9r5YmtS-NQ@mail.gmail.com
/messages/by-id/CAD21AoBtM=HGLkMKBgch37mf0-epa3_o=Y1PU0_m9r5YmtS-NQ@mail.gmail.com

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

michael@paquier.xyz

over 6 years ago

In reply to: Masahiko Sawada (#2)

Re: vacuumdb and new VACUUM options

On Wed, May 08, 2019 at 09:26:35AM +0900, Masahiko Sawada wrote:

I think it's a good idea to add new options of these parameters for
vacuumdb. While making INDEX_CLEANUP option patch I also attached the
patch for INDEX_CLEANUP parameter before[1], although it adds
--disable-index-cleanup option instead.

I have added an open item for that. I think that we should added
these options.
--
Michael

Julien Rouhaud

rjuju123@gmail.com

over 6 years ago

In reply to: Michael Paquier (#3)

Re: vacuumdb and new VACUUM options

On Wed, May 8, 2019 at 9:06 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 09:26:35AM +0900, Masahiko Sawada wrote:

I think it's a good idea to add new options of these parameters for
vacuumdb. While making INDEX_CLEANUP option patch I also attached the
patch for INDEX_CLEANUP parameter before[1], although it adds
--disable-index-cleanup option instead.

I have added an open item for that. I think that we should added
these options.

+1, and thanks for adding the open item!

masao.fujii@gmail.com

over 6 years ago

In reply to: Masahiko Sawada (#2)

Re: vacuumdb and new VACUUM options

On Wed, May 8, 2019 at 9:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 8, 2019 at 2:41 AM Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

vacuumdb command supports the corresponding options to
any VACUUM parameters except INDEX_CLEANUP and TRUNCATE
that were added recently. Should vacuumdb also support those
new parameters, i.e., add --index-cleanup and --truncate options
to the command?

I think it's a good idea to add new options of these parameters for
vacuumdb. While making INDEX_CLEANUP option patch I also attached the
patch for INDEX_CLEANUP parameter before[1], although it adds
--disable-index-cleanup option instead.

Regarding INDEX_CLEANUP, now VACUUM has three modes;

(1) VACUUM (INDEX_CLEANUP on) does index cleanup
whatever vacuum_index_cleanup reloption is.
(2) VACUUM (INDEX_CLEANUP off) does not do index cleanup
whatever vacuum_index_cleanup reloption is.
(3) plain VACUUM decides whether to do index cleanup
according to vacuum_index_cleanup reloption.

If no option for index cleanup is specified, vacuumdb command
should work in the mode (3). IMO this is intuitive.

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

Regards,

--
Fujii Masao

Euler Taveira

euler@timbira.com.br

over 6 years ago

In reply to: Fujii Masao (#5)

Re: vacuumdb and new VACUUM options

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

michael@paquier.xyz

over 6 years ago

In reply to: Euler Taveira (#6)

Re: vacuumdb and new VACUUM options

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter, because that's more
intuitive:
- --index-cleanup=3Dfalse =3D> VACUUM (INDEX_CLEANUP=3Dfalse)
- --index-cleanup=3Dtrue =3D> VACUUM (INDEX_CLEANUP=3Dtrue)
- no --index-cleanup means to rely on the reloption.
--
Michael

sawada.mshk@gmail.com

over 6 years ago

In reply to: Michael Paquier (#7)

2 attachment(s)

Re: vacuumdb and new VACUUM options

On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter

+1. Attached the draft version patches for both options.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

v2-0001-Add-index-cleanup-option-to-vacuumdb.patchapplication/octet-stream; name=v2-0001-Add-index-cleanup-option-to-vacuumdb.patchDownload

From 4b8d7165d58776f934e2680cc3c061f84057fb68 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 9 May 2019 20:02:05 +0900
Subject: [PATCH v2 1/2] Add --index-cleanup option to vacuumdb.

---
 doc/src/sgml/ref/vacuumdb.sgml    | 18 ++++++++++++
 src/bin/scripts/t/100_vacuumdb.pl | 16 +++++++++-
 src/bin/scripts/vacuumdb.c        | 62 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 47d9345..78e8c49 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -149,6 +149,24 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
+      <term><option>--index-cleanup=<replaceable class="parameter">boolean</replaceable></option></term>
+      <listitem>
+       <para>
+         Specify that <command>VACUUM</command> should attempt to remove
+         index entries pointing to dead tuples. If not specify this option
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.
+       </para>
+       <note>
+        <para>
+         This option is only available for servers running
+         <productname>PostgreSQL</productname> 12 and later.
+        </para>
+       </note>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><option>-j <replaceable class="parameter">njobs</replaceable></option></term>
       <term><option>--jobs=<replaceable class="parameter">njobs</replaceable></option></term>
       <listitem>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 7f3a9b1..b23e091 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 44;
+use Test::More tests => 50;
 
 program_help_ok('vacuumdb');
 program_version_ok('vacuumdb');
@@ -45,9 +45,23 @@ $node->issues_sql_like(
 	[ 'vacuumdb', '--skip-locked', '--analyze-only', 'postgres' ],
 	qr/statement: ANALYZE \(SKIP_LOCKED\).*;/,
 	'vacuumdb --skip-locked --analyze-only');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--index-cleanup=false', 'postgres' ],
+	qr/statement: VACUUM \(INDEX_CLEANUP FALSE\).*;/,
+	'vacuumdb --index-cleanup=false');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
+	qr/statement: VACUUM \(INDEX_CLEANUP TRUE\).*;/,
+	'vacuumdb --index-cleanup=true');
 $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
 	'--analyze-only and --disable-page-skipping specified together');
+$node->command_fails(
+	[ 'vacuumdb', '--analyze-only', '--index-cleanup=true', 'postgres' ],
+	'--analyze-only and --index-cleanup specified together');
+$node->command_fails(
+	[ 'vacuumdb', '--index-cleanup=invalid', 'postgres' ],
+	'--index-cleanup with an invalid argument');
 $node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
 	'vacuumdb with connection string');
 
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index e9da74c..408b4de 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -46,6 +46,7 @@ typedef struct vacuumingOptions
 	bool		skip_locked;
 	int			min_xid_age;
 	int			min_mxid_age;
+	char		*index_cleanup;
 } vacuumingOptions;
 
 
@@ -87,6 +88,10 @@ static void init_slot(ParallelSlot *slot, PGconn *conn);
 
 static void help(const char *progname);
 
+static void check_bool_str(const char *opt_name, char *opt_str);
+
+static bool get_bool_from_str(char *bool_str);
+
 /* For analyze-in-stages mode */
 #define ANALYZE_NO_STAGE	-1
 #define ANALYZE_NUM_STAGES	3
@@ -118,6 +123,7 @@ main(int argc, char *argv[])
 		{"skip-locked", no_argument, NULL, 5},
 		{"min-xid-age", required_argument, NULL, 6},
 		{"min-mxid-age", required_argument, NULL, 7},
+		{"index-cleanup", required_argument, NULL, 8},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -242,6 +248,12 @@ main(int argc, char *argv[])
 					exit(1);
 				}
 				break;
+			case 8:
+				{
+					vacopts.index_cleanup = pg_strdup(optarg);
+					check_bool_str("index-cleanup", vacopts.index_cleanup);
+				}
+				break;
 			default:
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 				exit(1);
@@ -286,6 +298,13 @@ main(int argc, char *argv[])
 						 "disable-page-skipping");
 			exit(1);
 		}
+		if (vacopts.index_cleanup &&
+			get_bool_from_str(vacopts.index_cleanup))
+		{
+			pg_log_error("cannot use the \"%s\" option when performing only analyze",
+						 "index-cleanup");
+			exit(1);
+		}
 		/* allow 'and_analyze' with 'analyze_only' */
 	}
 
@@ -414,6 +433,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 		exit(1);
 	}
 
+	if (vacopts->index_cleanup != NULL && PQserverVersion(conn) < 120000)
+	{
+		PQfinish(conn);
+		pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL 12",
+					 "index-cleanup");
+		exit(1);
+	}
+
 	if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
 	{
 		PQfinish(conn);
@@ -874,6 +901,15 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
 				appendPQExpBuffer(sql, "%sSKIP_LOCKED", sep);
 				sep = comma;
 			}
+			if (vacopts->index_cleanup)
+			{
+				/* INDEX_CLEANUP is supported since 12 */
+				Assert(serverVersion >= 120000);
+				appendPQExpBuffer(sql, "%sINDEX_CLEANUP %s", sep,
+								  get_bool_from_str(vacopts->index_cleanup)
+								  ? "TRUE" : "FALSE");
+				sep = comma;
+			}
 			if (vacopts->full)
 			{
 				appendPQExpBuffer(sql, "%sFULL", sep);
@@ -1209,6 +1245,31 @@ init_slot(ParallelSlot *slot, PGconn *conn)
 	slot->isFree = true;
 }
 
+/* Check the given string is either "true" or "false" */
+static void
+check_bool_str(const char *opt_name, char *opt_str)
+{
+	if (strncasecmp(opt_str, "true", 4) != 0 &&
+		strncasecmp(opt_str, "false", 5) != 0)
+	{
+		pg_log_error("the \"%s\" option requires a boolean value", opt_name);
+		exit(1);
+	}
+}
+
+/*
+ * Return a bool value according to the given boolean string. 'bool_str'
+ * must not be NULL and be either "true" or "false".
+ */
+static bool
+get_bool_from_str(char *bool_str)
+{
+	if (strncasecmp(bool_str, "true", 4) == 0)
+		return true;
+	else
+		return false;
+}
+
 static void
 help(const char *progname)
 {
@@ -1222,6 +1283,7 @@ help(const char *progname)
 	printf(_("  -e, --echo                      show the commands being sent to the server\n"));
 	printf(_("  -f, --full                      do full vacuuming\n"));
 	printf(_("  -F, --freeze                    freeze row transaction information\n"));
+	printf(_("      --index-cleanup=BOOLEAN     do or do not index vacuuming and index cleanup\n"));
 	printf(_("  -j, --jobs=NUM                  use this many concurrent connections to vacuum\n"));
 	printf(_("      --min-mxid-age=MXID_AGE     minimum multixact ID age of tables to vacuum\n"));
 	printf(_("      --min-xid-age=XID_AGE       minimum transaction ID age of tables to vacuum\n"));
-- 
2.10.5

v2-0002-Add-truncate-option-to-vacuumdb.patchapplication/octet-stream; name=v2-0002-Add-truncate-option-to-vacuumdb.patchDownload

From 2bbb787c1e6ba5d1355c80abc5096309db4772cc Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 9 May 2019 20:02:25 +0900
Subject: [PATCH v2 2/2] Add --truncate option to vacuumdb.

---
 doc/src/sgml/ref/vacuumdb.sgml    | 18 ++++++++++++++++++
 src/bin/scripts/t/100_vacuumdb.pl | 13 ++++++++++++-
 src/bin/scripts/vacuumdb.c        | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 78e8c49..d250b3e 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -290,6 +290,24 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
+      <term><option>--truncate=<replaceable class="parameter">boolean</replaceable></option></term>
+      <listitem>
+       <para>
+         Specify that <command>VACUUM</command> should attempt to truncate off
+         any empty pages at the end of the table. If not specify this option
+         the behavior depends on <literal>vacuum_truncate</literal> option
+         for the table to be vacuumed.
+       </para>
+       <note>
+        <para>
+         This option is only available for servers running
+         <productname>PostgreSQL</productname> 12 and later.
+        </para>
+       </note>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><option>-v</option></term>
       <term><option>--verbose</option></term>
       <listitem>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index b23e091..dcfe53b 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 50;
+use Test::More tests => 55;
 
 program_help_ok('vacuumdb');
 program_version_ok('vacuumdb');
@@ -53,6 +53,14 @@ $node->issues_sql_like(
 	[ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
 	qr/statement: VACUUM \(INDEX_CLEANUP TRUE\).*;/,
 	'vacuumdb --index-cleanup=true');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--truncate=false', 'postgres' ],
+	qr/statement: VACUUM \(TRUNCATE FALSE\).*;/,
+	'vacuumdb --truncate=false');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--truncate=true', 'postgres' ],
+	qr/statement: VACUUM \(TRUNCATE TRUE\).*;/,
+	'vacuumdb --truncate=true');
 $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
 	'--analyze-only and --disable-page-skipping specified together');
@@ -60,6 +68,9 @@ $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--index-cleanup=true', 'postgres' ],
 	'--analyze-only and --index-cleanup specified together');
 $node->command_fails(
+	[ 'vacuumdb', '--analyze-only', '--truncate=true', 'postgres' ],
+	'--analyze-only and --truncate specified together');
+$node->command_fails(
 	[ 'vacuumdb', '--index-cleanup=invalid', 'postgres' ],
 	'--index-cleanup with an invalid argument');
 $node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 408b4de..6aab34b 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -47,6 +47,7 @@ typedef struct vacuumingOptions
 	int			min_xid_age;
 	int			min_mxid_age;
 	char		*index_cleanup;
+	char		*truncate;
 } vacuumingOptions;
 
 
@@ -124,6 +125,7 @@ main(int argc, char *argv[])
 		{"min-xid-age", required_argument, NULL, 6},
 		{"min-mxid-age", required_argument, NULL, 7},
 		{"index-cleanup", required_argument, NULL, 8},
+		{"truncate", required_argument, NULL, 9},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -254,6 +256,12 @@ main(int argc, char *argv[])
 					check_bool_str("index-cleanup", vacopts.index_cleanup);
 				}
 				break;
+			case 9:
+				{
+					vacopts.truncate = pg_strdup(optarg);
+					check_bool_str("trauncate", vacopts.truncate);
+				}
+				break;
 			default:
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 				exit(1);
@@ -305,6 +313,13 @@ main(int argc, char *argv[])
 						 "index-cleanup");
 			exit(1);
 		}
+		if (vacopts.truncate &&
+			get_bool_from_str(vacopts.truncate))
+		{
+			pg_log_error("cannot use the \"%s\" option when performing only analyze",
+						 "truncate");
+			exit(1);
+		}
 		/* allow 'and_analyze' with 'analyze_only' */
 	}
 
@@ -441,6 +456,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 		exit(1);
 	}
 
+	if (vacopts->truncate != NULL && PQserverVersion(conn) < 120000)
+	{
+		PQfinish(conn);
+		pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL 12",
+					 "truncate");
+		exit(1);
+	}
+
 	if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
 	{
 		PQfinish(conn);
@@ -910,6 +933,15 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
 								  ? "TRUE" : "FALSE");
 				sep = comma;
 			}
+			if (vacopts->truncate)
+			{
+				/* TRUNCATE is supported since 12 */
+				Assert(serverVersion >= 120000);
+				appendPQExpBuffer(sql, "%sTRUNCATE %s", sep,
+								  get_bool_from_str(vacopts->truncate)
+								  ? "TRUE" : "FALSE");
+				sep = comma;
+			}
 			if (vacopts->full)
 			{
 				appendPQExpBuffer(sql, "%sFULL", sep);
@@ -1290,6 +1322,7 @@ help(const char *progname)
 	printf(_("  -q, --quiet                     don't write any messages\n"));
 	printf(_("      --skip-locked               skip relations that cannot be immediately locked\n"));
 	printf(_("  -t, --table='TABLE[(COLUMNS)]'  vacuum specific table(s) only\n"));
+	printf(_("      --truncate=BOOLEAN          do or do not truncate off empty pages at the end of the table\n"));
 	printf(_("  -v, --verbose                   write a lot of output\n"));
 	printf(_("  -V, --version                   output version information, then exit\n"));
 	printf(_("  -z, --analyze                   update optimizer statistics\n"));
-- 
2.10.5

Kyotaro HORIGUCHI

horiguchi.kyotaro@lab.ntt.co.jp

over 6 years ago

In reply to: Masahiko Sawada (#8)

Re: vacuumdb and new VACUUM options

At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>

On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter

+1. Attached the draft version patches for both options.

+	printf(_("      --index-cleanup=BOOLEAN     do or do not index vacuuming and index cleanup\n"));
+	printf(_("      --truncate=BOOLEAN          do or do not truncate off empty pages at the end of the table\n"));

I *feel* that force/inhibit is suitable than true/false for the
options.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#10

Julien Rouhaud

rjuju123@gmail.com

over 6 years ago

In reply to: Kyotaro HORIGUCHI (#9)

Re: vacuumdb and new VACUUM options

On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>

On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter

+1. Attached the draft version patches for both options.
+       printf(_("      --index-cleanup=BOOLEAN     do or do not index vacuuming and index cleanup\n"));
+       printf(_("      --truncate=BOOLEAN          do or do not truncate off empty pages at the end of the table\n"));
I *feel* that force/inhibit is suitable than true/false for the
options.

Indeed.

+         If not specify this option
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.

+         If not specify this option
+         the behavior depends on <literal>vacuum_truncate</literal> option
+         for the table to be vacuumed.

Those sentences should be rephrased to something like "If this option
is not specified, the bahvior...".

#11

sawada.mshk@gmail.com

over 6 years ago

In reply to: Julien Rouhaud (#10)

Re: vacuumdb and new VACUUM options

On Fri, May 10, 2019 at 9:03 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>

On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter

+1. Attached the draft version patches for both options.
+       printf(_("      --index-cleanup=BOOLEAN     do or do not index vacuuming and index cleanup\n"));
+       printf(_("      --truncate=BOOLEAN          do or do not truncate off empty pages at the end of the table\n"));
I *feel* that force/inhibit is suitable than true/false for the
options.
Indeed.

The new VACUUM command option for these option take true and false as
the same meaning. What is the motivation is to change a 1-1 mapping
name?

+         If not specify this option
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.

+         If not specify this option
+         the behavior depends on <literal>vacuum_truncate</literal> option
+         for the table to be vacuumed.

Those sentences should be rephrased to something like "If this option
is not specified, the bahvior...".

Thank you! I've incorporated your comment in my branch. I'll post the
updated version patch after the above discussion got a consensus.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

#12

michael@paquier.xyz

over 6 years ago

In reply to: Masahiko Sawada (#11)

Re: vacuumdb and new VACUUM options

On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote:

Thank you! I've incorporated your comment in my branch. I'll post the
updated version patch after the above discussion got a consensus.

Fujii-san, any input about the way to move forward here? Beta1 is
planned for next week, hence it would be nice to progress on this
front this week.
--
Michael

#13

masao.fujii@gmail.com

over 6 years ago

In reply to: Michael Paquier (#12)

Re: vacuumdb and new VACUUM options

On Tue, May 14, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote:

Thank you! I've incorporated your comment in my branch. I'll post the
updated version patch after the above discussion got a consensus.

Fujii-san, any input about the way to move forward here? Beta1 is
planned for next week, hence it would be nice to progress on this
front this week.

I think that we can push "--index-cleanup=BOOLEAN" version into beta1,
and then change the interface of the options if we received many
complaints about "--index-cleanup=BOOLEAN" from users. So this week,
I'd like to review Sawada's patch and commit it if that's ok.

Regards,

--
Fujii Masao

#14

masao.fujii@gmail.com

over 6 years ago

In reply to: Masahiko Sawada (#8)

Re: vacuumdb and new VACUUM options

On Thu, May 9, 2019 at 8:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:

Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:

The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?

--index-cleanup=BOOL

I agree with Euler's suggestion to have a 1-1 mapping between the
option of vacuumdb and the VACUUM parameter

+1. Attached the draft version patches for both options.

Thanks for the patch!

+ if (strncasecmp(opt_str, "true", 4) != 0 &&
+ strncasecmp(opt_str, "false", 5) != 0)

Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value,
like VACUUM does?

+ char *index_cleanup;

The patch would be simpler if enum trivalue is used for index_cleanup
variable as the type.

Regards,

--
Fujii Masao

#15

michael@paquier.xyz

over 6 years ago

In reply to: Fujii Masao (#14)

Re: vacuumdb and new VACUUM options

On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote:

+ if (strncasecmp(opt_str, "true", 4) != 0 &&
+ strncasecmp(opt_str, "false", 5) != 0)
Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value,
like VACUUM does?

I am wondering, in order to keep this patch simple, if you shouldn't
accept any value and just let the parsing logic on the backend side
do all the work. That's what we do for other things like the
connection parameter replication for example, and there is no need to
mimic a boolean parsing equivalent on the frontend with something like
check_bool_str() as presented in the patch. The main downside is that
the error message gets linked to VACUUM and not vacuumdb.

Another thing which you may be worth looking at would be to make
parse_bool() frontend aware, where pg_strncasecmp() is actually
available.
--
Michael

#16

sawada.mshk@gmail.com

over 6 years ago

In reply to: Michael Paquier (#15)

Re: vacuumdb and new VACUUM options

On Wed, May 15, 2019 at 7:51 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote:
+ if (strncasecmp(opt_str, "true", 4) != 0 &&
+ strncasecmp(opt_str, "false", 5) != 0)
Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value,
like VACUUM does?
I am wondering, in order to keep this patch simple, if you shouldn't
accept any value and just let the parsing logic on the backend side
do all the work. That's what we do for other things like the
connection parameter replication for example, and there is no need to
mimic a boolean parsing equivalent on the frontend with something like
check_bool_str() as presented in the patch. The main downside is that
the error message gets linked to VACUUM and not vacuumdb.

I might be missing something but if the frontend code doesn't check
arguments and we let the backend parsing logic do all the work then it
allows user to execute an arbitrary SQL command via vacuumdb.

Another thing which you may be worth looking at would be to make
parse_bool() frontend aware, where pg_strncasecmp() is actually
available.

Or how about add a function that parse a boolean string value, as a
common routine among frontend programs, maybe in common.c or fe_utils?
We results in having the duplicate code between frontend and backend
but it may be less side effects than making parse_bool available on
frontend code.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

#17

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Masahiko Sawada (#16)

Re: vacuumdb and new VACUUM options

Hi,

On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote:

I might be missing something but if the frontend code doesn't check
arguments and we let the backend parsing logic do all the work then it
allows user to execute an arbitrary SQL command via vacuumdb.

But, so what? The user could just have used psql to do so?

Greetings,

Andres Freund

#18

sawada.mshk@gmail.com

over 6 years ago

In reply to: Andres Freund (#17)

Re: vacuumdb and new VACUUM options

On Wed, May 15, 2019 at 11:45 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote:

I might be missing something but if the frontend code doesn't check
arguments and we let the backend parsing logic do all the work then it
allows user to execute an arbitrary SQL command via vacuumdb.

But, so what? The user could just have used psql to do so?

Indeed. It shouldn't be a problem and we even now can do that by
specifying for example --table="t(c1);select 1" but doesn't work.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

#19

sawada.mshk@gmail.com

over 6 years ago

In reply to: Masahiko Sawada (#18)

2 attachment(s)

Re: vacuumdb and new VACUUM options

On Wed, May 15, 2019 at 1:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 15, 2019 at 11:45 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote:

I might be missing something but if the frontend code doesn't check
arguments and we let the backend parsing logic do all the work then it
allows user to execute an arbitrary SQL command via vacuumdb.

But, so what? The user could just have used psql to do so?

Indeed. It shouldn't be a problem and we even now can do that by
specifying for example --table="t(c1);select 1" but doesn't work.

I've attached new version patch that takes the way to let the backend
parser do all work.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

v3-0001-Add-index-cleanup-option-to-vacuumdb.patchapplication/octet-stream; name=v3-0001-Add-index-cleanup-option-to-vacuumdb.patchDownload

From de60d212b50a6412e483c995b83e28c5597089ad Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 9 May 2019 20:02:05 +0900
Subject: [PATCH v3 1/2] Add --index-cleanup option to vacuumdb.

---
 doc/src/sgml/ref/vacuumdb.sgml    | 18 ++++++++++++++++++
 src/bin/scripts/t/100_vacuumdb.pl | 16 +++++++++++++++-
 src/bin/scripts/vacuumdb.c        | 29 +++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 47d9345..5ec043e 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -149,6 +149,24 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
+      <term><option>--index-cleanup=<replaceable class="parameter">boolean</replaceable></option></term>
+      <listitem>
+       <para>
+         Specify that <command>VACUUM</command> should attempt to remove
+         index entries pointing to dead tuples. If this option is not specified
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.
+       </para>
+       <note>
+        <para>
+         This option is only available for servers running
+         <productname>PostgreSQL</productname> 12 and later.
+        </para>
+       </note>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><option>-j <replaceable class="parameter">njobs</replaceable></option></term>
       <term><option>--jobs=<replaceable class="parameter">njobs</replaceable></option></term>
       <listitem>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 7f3a9b1..984f2a8 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 44;
+use Test::More tests => 50;
 
 program_help_ok('vacuumdb');
 program_version_ok('vacuumdb');
@@ -45,9 +45,23 @@ $node->issues_sql_like(
 	[ 'vacuumdb', '--skip-locked', '--analyze-only', 'postgres' ],
 	qr/statement: ANALYZE \(SKIP_LOCKED\).*;/,
 	'vacuumdb --skip-locked --analyze-only');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--index-cleanup=false', 'postgres' ],
+	qr/statement: VACUUM \(INDEX_CLEANUP false\).*;/,
+	'vacuumdb --index-cleanup=false');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
+	qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/,
+	'vacuumdb --index-cleanup=true');
 $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
 	'--analyze-only and --disable-page-skipping specified together');
+$node->command_fails(
+	[ 'vacuumdb', '--analyze-only', '--index-cleanup=true', 'postgres' ],
+	'--analyze-only and --index-cleanup specified together');
+$node->command_fails(
+	[ 'vacuumdb', '--index-cleanup=invalid', 'postgres' ],
+	'--index-cleanup with an invalid argument');
 $node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
 	'vacuumdb with connection string');
 
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 6d216aa..7f91d18 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -46,6 +46,7 @@ typedef struct vacuumingOptions
 	bool		skip_locked;
 	int			min_xid_age;
 	int			min_mxid_age;
+	char		*index_cleanup;
 } vacuumingOptions;
 
 
@@ -87,6 +88,7 @@ static void init_slot(ParallelSlot *slot, PGconn *conn);
 
 static void help(const char *progname);
 
+
 /* For analyze-in-stages mode */
 #define ANALYZE_NO_STAGE	-1
 #define ANALYZE_NUM_STAGES	3
@@ -118,6 +120,7 @@ main(int argc, char *argv[])
 		{"skip-locked", no_argument, NULL, 5},
 		{"min-xid-age", required_argument, NULL, 6},
 		{"min-mxid-age", required_argument, NULL, 7},
+		{"index-cleanup", required_argument, NULL, 8},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -242,6 +245,9 @@ main(int argc, char *argv[])
 					exit(1);
 				}
 				break;
+			case 8:
+				vacopts.index_cleanup = pg_strdup(optarg);
+				break;
 			default:
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 				exit(1);
@@ -286,6 +292,12 @@ main(int argc, char *argv[])
 						 "disable-page-skipping");
 			exit(1);
 		}
+		if (vacopts.index_cleanup != NULL)
+		{
+			pg_log_error("cannot use the \"%s\" option when performing only analyze",
+						 "index-cleanup");
+			exit(1);
+		}
 		/* allow 'and_analyze' with 'analyze_only' */
 	}
 
@@ -414,6 +426,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 		exit(1);
 	}
 
+	if (vacopts->index_cleanup != NULL && PQserverVersion(conn) < 120000)
+	{
+		PQfinish(conn);
+		pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL 12",
+					 "index-cleanup");
+		exit(1);
+	}
+
 	if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
 	{
 		PQfinish(conn);
@@ -874,6 +894,14 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
 				appendPQExpBuffer(sql, "%sSKIP_LOCKED", sep);
 				sep = comma;
 			}
+			if (vacopts->index_cleanup)
+			{
+				/* INDEX_CLEANUP is supported since 12 */
+				Assert(serverVersion >= 120000);
+				appendPQExpBuffer(sql, "%sINDEX_CLEANUP %s", sep,
+								  vacopts->index_cleanup);
+				sep = comma;
+			}
 			if (vacopts->full)
 			{
 				appendPQExpBuffer(sql, "%sFULL", sep);
@@ -1222,6 +1250,7 @@ help(const char *progname)
 	printf(_("  -e, --echo                      show the commands being sent to the server\n"));
 	printf(_("  -f, --full                      do full vacuuming\n"));
 	printf(_("  -F, --freeze                    freeze row transaction information\n"));
+	printf(_("      --index-cleanup=BOOLEAN     do or do not index vacuuming and index cleanup\n"));
 	printf(_("  -j, --jobs=NUM                  use this many concurrent connections to vacuum\n"));
 	printf(_("      --min-mxid-age=MXID_AGE     minimum multixact ID age of tables to vacuum\n"));
 	printf(_("      --min-xid-age=XID_AGE       minimum transaction ID age of tables to vacuum\n"));
-- 
2.10.5

v3-0002-Add-truncate-option-to-vacuumdb.patchapplication/octet-stream; name=v3-0002-Add-truncate-option-to-vacuumdb.patchDownload

From 59e3146f585e288d41738daa9a1d18687e2851d1 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 15 May 2019 15:27:51 +0900
Subject: [PATCH v3 2/2] Add --truncate option to vacuumdb.

---
 doc/src/sgml/ref/vacuumdb.sgml    | 18 ++++++++++++++++++
 src/bin/scripts/t/100_vacuumdb.pl | 13 ++++++++++++-
 src/bin/scripts/vacuumdb.c        | 28 ++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 5ec043e..6bb63c7 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -290,6 +290,24 @@ PostgreSQL documentation
      </varlistentry>
 
      <varlistentry>
+      <term><option>--truncate=<replaceable class="parameter">boolean</replaceable></option></term>
+      <listitem>
+       <para>
+         Specify that <command>VACUUM</command> should attempt to truncate off
+         any empty pages at the end of the table. If this option is not specified
+         the behavior depends on <literal>vacuum_truncate</literal> option
+         for the table to be vacuumed.
+       </para>
+       <note>
+        <para>
+         This option is only available for servers running
+         <productname>PostgreSQL</productname> 12 and later.
+        </para>
+       </note>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><option>-v</option></term>
       <term><option>--verbose</option></term>
       <listitem>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 984f2a8..555e9b6 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -3,7 +3,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 50;
+use Test::More tests => 55;
 
 program_help_ok('vacuumdb');
 program_version_ok('vacuumdb');
@@ -53,6 +53,14 @@ $node->issues_sql_like(
 	[ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
 	qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/,
 	'vacuumdb --index-cleanup=true');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--truncate=false', 'postgres' ],
+	qr/statement: VACUUM \(TRUNCATE false\).*;/,
+	'vacuumdb --truncate=false');
+$node->issues_sql_like(
+	[ 'vacuumdb', '--truncate=true', 'postgres' ],
+	qr/statement: VACUUM \(TRUNCATE true\).*;/,
+	'vacuumdb --truncate=true');
 $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
 	'--analyze-only and --disable-page-skipping specified together');
@@ -60,6 +68,9 @@ $node->command_fails(
 	[ 'vacuumdb', '--analyze-only', '--index-cleanup=true', 'postgres' ],
 	'--analyze-only and --index-cleanup specified together');
 $node->command_fails(
+	[ 'vacuumdb', '--analyze-only', '--truncate=true', 'postgres' ],
+	'--analyze-only and --truncate specified together');
+$node->command_fails(
 	[ 'vacuumdb', '--index-cleanup=invalid', 'postgres' ],
 	'--index-cleanup with an invalid argument');
 $node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7f91d18..a7aaa82 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -47,6 +47,7 @@ typedef struct vacuumingOptions
 	int			min_xid_age;
 	int			min_mxid_age;
 	char		*index_cleanup;
+	char		*truncate;
 } vacuumingOptions;
 
 
@@ -121,6 +122,7 @@ main(int argc, char *argv[])
 		{"min-xid-age", required_argument, NULL, 6},
 		{"min-mxid-age", required_argument, NULL, 7},
 		{"index-cleanup", required_argument, NULL, 8},
+		{"truncate", required_argument, NULL, 9},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -248,6 +250,9 @@ main(int argc, char *argv[])
 			case 8:
 				vacopts.index_cleanup = pg_strdup(optarg);
 				break;
+			case 9:
+				vacopts.truncate = pg_strdup(optarg);
+				break;
 			default:
 				fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
 				exit(1);
@@ -298,6 +303,12 @@ main(int argc, char *argv[])
 						 "index-cleanup");
 			exit(1);
 		}
+		if (vacopts.truncate != NULL)
+		{
+			pg_log_error("cannot use the \"%s\" option when performing only analyze",
+						 "truncate");
+			exit(1);
+		}
 		/* allow 'and_analyze' with 'analyze_only' */
 	}
 
@@ -434,6 +445,14 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
 		exit(1);
 	}
 
+	if (vacopts->truncate != NULL && PQserverVersion(conn) < 120000)
+	{
+		PQfinish(conn);
+		pg_log_error("cannot use the \"%s\" option on server versions older than PostgreSQL 12",
+					 "truncate");
+		exit(1);
+	}
+
 	if (vacopts->skip_locked && PQserverVersion(conn) < 120000)
 	{
 		PQfinish(conn);
@@ -902,6 +921,14 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
 								  vacopts->index_cleanup);
 				sep = comma;
 			}
+			if (vacopts->truncate)
+			{
+				/* TRUNCATE is supported since 12 */
+				Assert(serverVersion >= 120000);
+				appendPQExpBuffer(sql, "%sTRUNCATE %s", sep,
+								  vacopts->truncate);
+				sep = comma;
+			}
 			if (vacopts->full)
 			{
 				appendPQExpBuffer(sql, "%sFULL", sep);
@@ -1257,6 +1284,7 @@ help(const char *progname)
 	printf(_("  -q, --quiet                     don't write any messages\n"));
 	printf(_("      --skip-locked               skip relations that cannot be immediately locked\n"));
 	printf(_("  -t, --table='TABLE[(COLUMNS)]'  vacuum specific table(s) only\n"));
+	printf(_("      --truncate=BOOLEAN          do or do not truncate off empty pages at the end of the table\n"));
 	printf(_("  -v, --verbose                   write a lot of output\n"));
 	printf(_("  -V, --version                   output version information, then exit\n"));
 	printf(_("  -z, --analyze                   update optimizer statistics\n"));
-- 
2.10.5

#20

michael@paquier.xyz

over 6 years ago

In reply to: Masahiko Sawada (#19)

Re: vacuumdb and new VACUUM options

On Wed, May 15, 2019 at 03:44:22PM +0900, Masahiko Sawada wrote:

I've attached new version patch that takes the way to let the backend
parser do all work.

I was wondering how the error handling gets by not having the parsing
on the frontend, and it could be worse:
$ vacuumdb --index-cleanup=popo
vacuumdb: vacuuming database "postgres"
vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database
"postgres" failed: ERROR: index_cleanup requires a Boolean value
$ vacuumdb --truncate=popo
vacuumdb: vacuuming database "postgres"
vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database
"postgres" failed: ERROR: truncate requires a Boolean value

For TRUNCATE, we actually get to the same error, and INDEX_CLEANUP
just defers with the separator between the two terms. I think that we
could live with that for simplicity's sake. Perhaps others have
different opinions though.

+ if (vacopts.index_cleanup != NULL)
Checking directly for NULL-ness here is inconsistent with the previous
callers.

+$node->issues_sql_like(
+   [ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
+   qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/,
+   'vacuumdb --index-cleanup=true')
We should have a failure test here instead of testing two times the
same boolean parameter with opposite values to make sure that we still
complain on invalid input values.

+         Specify that <command>VACUUM</command> should attempt to remove
+         index entries pointing to dead tuples. If this option is not specified
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.
The description of other commands do not mention directly VACUUM, and
have a more straight-forward description about the goal of the option.
So the first sentence could be reworked as follows:
Removes index entries pointing to dead tuples.

And the second for --truncate:
Truncates any empty pages at the end of the relation.

My 2c.
--
Michael

#21

sawada.mshk@gmail.com

over 6 years ago

In reply to: Michael Paquier (#20)

Re: vacuumdb and new VACUUM options

On Fri, May 17, 2019 at 11:09 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 15, 2019 at 03:44:22PM +0900, Masahiko Sawada wrote:

I've attached new version patch that takes the way to let the backend
parser do all work.

I was wondering how the error handling gets by not having the parsing
on the frontend, and it could be worse:
$ vacuumdb --index-cleanup=popo
vacuumdb: vacuuming database "postgres"
vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database
"postgres" failed: ERROR: index_cleanup requires a Boolean value
$ vacuumdb --truncate=popo
vacuumdb: vacuuming database "postgres"
vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database
"postgres" failed: ERROR: truncate requires a Boolean value

For TRUNCATE, we actually get to the same error, and INDEX_CLEANUP
just defers with the separator between the two terms. I think that we
could live with that for simplicity's sake.

Perhaps others have different opinions though.

Is it helpful for user if we show executed SQL when fails as the
frontend programs does in executeCommand?

$ vacuumdb --index-cleanup=foo postgres
vacuumdb: vacuuming database "postgres"
vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database
"postgres" failed: ERROR: index_cleanup requires a Boolean value
vacuumdb: query was: VACUUM (INDEX_CLEANUP foo) pg_catalog.pg_proc;

+ if (vacopts.index_cleanup != NULL)
Checking directly for NULL-ness here is inconsistent with the previous
callers.

+$node->issues_sql_like(
+   [ 'vacuumdb', '--index-cleanup=true', 'postgres' ],
+   qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/,
+   'vacuumdb --index-cleanup=true')
We should have a failure test here instead of testing two times the
same boolean parameter with opposite values to make sure that we still
complain on invalid input values.

Fixed.

+         Specify that <command>VACUUM</command> should attempt to remove
+         index entries pointing to dead tuples. If this option is not specified
+         the behavior depends on <literal>vacuum_index_cleanup</literal> option
+         for the table to be vacuumed.
The description of other commands do not mention directly VACUUM, and
have a more straight-forward description about the goal of the option.
So the first sentence could be reworked as follows:
Removes index entries pointing to dead tuples.

And the second for --truncate:
Truncates any empty pages at the end of the relation.

Fixed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

#22

Andres Freund

andres@anarazel.de

over 6 years ago

In reply to: Masahiko Sawada (#19)

Re: vacuumdb and new VACUUM options

Hi,

On 2019-05-15 15:44:22 +0900, Masahiko Sawada wrote:

From de60d212b50a6412e483c995b83e28c5597089ad Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 9 May 2019 20:02:05 +0900
Subject: [PATCH v3 1/2] Add --index-cleanup option to vacuumdb.

From 59e3146f585e288d41738daa9a1d18687e2851d1 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 15 May 2019 15:27:51 +0900
Subject: [PATCH v3 2/2] Add --truncate option to vacuumdb.

My impression is that these are better treated as feature work, to be
tackled in v13. I see no urgency to push this for v12. There's still
some disagreements on how parts of this are implemented, and we've beta1
coming up.

IMO we should just register this patch for the next CF, and drop the
open item.

Greetings,

Andres Freund

#23

michael@paquier.xyz

over 6 years ago

In reply to: Andres Freund (#22)

Re: vacuumdb and new VACUUM options

On Fri, May 17, 2019 at 01:11:53PM -0700, Andres Freund wrote:

My impression is that these are better treated as feature work, to be
tackled in v13. I see no urgency to push this for v12. There's still
some disagreements on how parts of this are implemented, and we've beta1
coming up.

It is true that we have lived without some options in vacuumdb while
these were already introduced at the SQL level, so I am in favor of
what you suggest here. Fujii-san, what do you think?
--
Michael

#24

masao.fujii@gmail.com

over 6 years ago

In reply to: Michael Paquier (#23)

Re: vacuumdb and new VACUUM options

On Sat, May 18, 2019 at 7:19 PM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, May 17, 2019 at 01:11:53PM -0700, Andres Freund wrote:

My impression is that these are better treated as feature work, to be
tackled in v13. I see no urgency to push this for v12. There's still
some disagreements on how parts of this are implemented, and we've beta1
coming up.

It is true that we have lived without some options in vacuumdb while
these were already introduced at the SQL level, so I am in favor of
what you suggest here. Fujii-san, what do you think?

I'm ok to drop this from open items for v12 because this is not a bug.
Let's work on this next CommitFest.

Regards,

--
Fujii Masao

#25