Issue #422: Make --error-on-invalid-index the default behavior (#433)

Skipping repacking the invalid indexes could lead to corrupting the
invalid indexes. In some rare cases, this might cause inserts to
tables to fail as changes to a table continuously be applied to
invalid indexes on it and updates to corrupted invalid indexes
potentially might fail by the checks done during insertion.
Therefore, by default do not repack a table with invalid indexes.
Repack only user explicitly specifies --no-error-on-invalid-index.

bin/pg_repack.c

@@ -258,7 +258,9 @@ static unsigned int		temp_obj_num = 0; /* temporary objects counter */
 static bool				no_kill_backend = false; /* abandon when timed-out */
 static bool				no_superuser_check = false;
 static SimpleStringList	exclude_extension_list = {NULL, NULL}; /* don't repack tables of these extensions */
-static bool 			error_on_invalid_index = false; /* don't repack when invalid index is found */
+static bool 			no_error_on_invalid_index = false; /* repack even though invalid index is found */
+static bool 			error_on_invalid_index = false; /* don't repack when invalid index is found,
+								 * deprecated, this the default behavior now */
 static int				apply_count = APPLY_COUNT_DEFAULT;
 static int				switch_threshold = SWITCH_THRESHOLD_DEFAULT;
 
@@ -290,6 +292,7 @@ static pgut_option options[] =
 	{ 'b', 'D', "no-kill-backend", &no_kill_backend },
 	{ 'b', 'k', "no-superuser-check", &no_superuser_check },
 	{ 'l', 'C', "exclude-extension", &exclude_extension_list },
+	{ 'b', 4, "no-error-on-invalid-index", &no_error_on_invalid_index },
 	{ 'b', 3, "error-on-invalid-index", &error_on_invalid_index },
 	{ 'i', 2, "apply-count", &apply_count },
 	{ 'i', 1, "switch-threshold", &switch_threshold },
@@ -1311,7 +1314,7 @@ repack_one_table(repack_table *table, const char *orderby)
 
 	/* First, just display a warning message for any invalid indexes
 	 * which may be on the table (mostly to match the behavior of 1.1.8),
-	 * if --error-on-invalid-index is not set
+	 * if --no-error-on-invalid-index is set
 	 */
 	indexres = execute(
 		"SELECT pg_get_indexdef(indexrelid)"
@@ -1322,7 +1325,8 @@ repack_one_table(repack_table *table, const char *orderby)
 	{
 		const char *indexdef;
 		indexdef = getstr(indexres, j, 0);
-		if (error_on_invalid_index) {
+
+		if (!no_error_on_invalid_index) {
 			elog(WARNING, "Invalid index: %s", indexdef);
 			goto cleanup;
 		} else {
@@ -2402,24 +2406,25 @@ pgut_help(bool details)
 		return;
 
 	printf("Options:\n");
-	printf("  -a, --all                     repack all databases\n");
-	printf("  -t, --table=TABLE             repack specific table only\n");
-	printf("  -I, --parent-table=TABLE      repack specific parent table and its inheritors\n");
-	printf("  -c, --schema=SCHEMA           repack tables in specific schema only\n");
-	printf("  -s, --tablespace=TBLSPC       move repacked tables to a new tablespace\n");
-	printf("  -S, --moveidx                 move repacked indexes to TBLSPC too\n");
-	printf("  -o, --order-by=COLUMNS        order by columns instead of cluster keys\n");
-	printf("  -n, --no-order                do vacuum full instead of cluster\n");
-	printf("  -N, --dry-run                 print what would have been repacked\n");
-	printf("  -j, --jobs=NUM                Use this many parallel jobs for each table\n");
-	printf("  -i, --index=INDEX             move only the specified index\n");
-	printf("  -x, --only-indexes            move only indexes of the specified table\n");
-	printf("  -T, --wait-timeout=SECS       timeout to cancel other backends on conflict\n");
-	printf("  -D, --no-kill-backend         don't kill other backends when timed out\n");
-	printf("  -Z, --no-analyze              don't analyze at end\n");
-	printf("  -k, --no-superuser-check      skip superuser checks in client\n");
-	printf("  -C, --exclude-extension       don't repack tables which belong to specific extension\n");
-	printf("      --error-on-invalid-index  don't repack when invalid index is found\n");
-	printf("      --apply-count             number of tuples to apply in one transaction during replay\n");
-	printf("      --switch-threshold        switch tables when that many tuples are left to catchup\n");
+	printf("  -a, --all                          repack all databases\n");
+	printf("  -t, --table=TABLE                  repack specific table only\n");
+	printf("  -I, --parent-table=TABLE           repack specific parent table and its inheritors\n");
+	printf("  -c, --schema=SCHEMA                repack tables in specific schema only\n");
+	printf("  -s, --tablespace=TBLSPC            move repacked tables to a new tablespace\n");
+	printf("  -S, --moveidx                      move repacked indexes to TBLSPC too\n");
+	printf("  -o, --order-by=COLUMNS             order by columns instead of cluster keys\n");
+	printf("  -n, --no-order                     do vacuum full instead of cluster\n");
+	printf("  -N, --dry-run                      print what would have been repacked\n");
+	printf("  -j, --jobs=NUM                     Use this many parallel jobs for each table\n");
+	printf("  -i, --index=INDEX                  move only the specified index\n");
+	printf("  -x, --only-indexes                 move only indexes of the specified table\n");
+	printf("  -T, --wait-timeout=SECS            timeout to cancel other backends on conflict\n");
+	printf("  -D, --no-kill-backend              don't kill other backends when timed out\n");
+	printf("  -Z, --no-analyze                   don't analyze at end\n");
+	printf("  -k, --no-superuser-check           skip superuser checks in client\n");
+	printf("  -C, --exclude-extension            don't repack tables which belong to specific extension\n");
+	printf("      --no-error-on-invalid-index    repack even though invalid index is found\n");
+	printf("      --error-on-invalid-index       don't repack when invalid index is found, deprecated, as this is the default behavior now\n");
+	printf("      --apply-count                  number of tuples to apply in one transaction during replay\n");
+	printf("      --switch-threshold             switch tables when that many tuples are left to catchup\n");
 }

doc/pg_repack.rst

@@ -109,40 +109,41 @@ Usage
 The following options can be specified in ``OPTIONS``.
 
 Options:
-  -a, --all                     repack all databases
-  -t, --table=TABLE             repack specific table only
-  -I, --parent-table=TABLE      repack specific parent table and its inheritors
-  -c, --schema=SCHEMA           repack tables in specific schema only
-  -s, --tablespace=TBLSPC       move repacked tables to a new tablespace
-  -S, --moveidx                 move repacked indexes to *TBLSPC* too
-  -o, --order-by=COLUMNS        order by columns instead of cluster keys
-  -n, --no-order                do vacuum full instead of cluster
-  -N, --dry-run                 print what would have been repacked and exit
-  -j, --jobs=NUM                Use this many parallel jobs for each table
-  -i, --index=INDEX             move only the specified index
-  -x, --only-indexes            move only indexes of the specified table
-  -T, --wait-timeout=SECS       timeout to cancel other backends on conflict
-  -D, --no-kill-backend         don't kill other backends when timed out
-  -Z, --no-analyze              don't analyze at end
-  -k, --no-superuser-check      skip superuser checks in client
-  -C, --exclude-extension       don't repack tables which belong to specific extension
-      --error-on-invalid-index  don't repack when invalid index is found
-      --apply-count             number of tuples to apply in one trasaction during replay
-      --switch-threshold        switch tables when that many tuples are left to catchup
+  -a, --all                          repack all databases
+  -t, --table=TABLE                  repack specific table only
+  -I, --parent-table=TABLE           repack specific parent table and its inheritors
+  -c, --schema=SCHEMA                repack tables in specific schema only
+  -s, --tablespace=TBLSPC            move repacked tables to a new tablespace
+  -S, --moveidx                      move repacked indexes to *TBLSPC* too
+  -o, --order-by=COLUMNS             order by columns instead of cluster keys
+  -n, --no-order                     do vacuum full instead of cluster
+  -N, --dry-run                      print what would have been repacked and exit
+  -j, --jobs=NUM                     Use this many parallel jobs for each table
+  -i, --index=INDEX                  move only the specified index
+  -x, --only-indexes                 move only indexes of the specified table
+  -T, --wait-timeout=SECS            timeout to cancel other backends on conflict
+  -D, --no-kill-backend              don't kill other backends when timed out
+  -Z, --no-analyze                   don't analyze at end
+  -k, --no-superuser-check           skip superuser checks in client
+  -C, --exclude-extension            don't repack tables which belong to specific extension
+      --no-error-on-invalid-index    repack even though invalid index is found
+      --error-on-invalid-index       don't repack when invalid index is found, deprecated, as this is the default behavior now
+      --apply-count                  number of tuples to apply in one trasaction during replay
+      --switch-threshold             switch tables when that many tuples are left to catchup
 
 Connection options:
-  -d, --dbname=DBNAME           database to connect
-  -h, --host=HOSTNAME           database server host or socket directory
-  -p, --port=PORT               database server port
-  -U, --username=USERNAME       user name to connect as
-  -w, --no-password             never prompt for password
-  -W, --password                force password prompt
+  -d, --dbname=DBNAME                database to connect
+  -h, --host=HOSTNAME                database server host or socket directory
+  -p, --port=PORT                    database server port
+  -U, --username=USERNAME            user name to connect as
+  -w, --no-password                  never prompt for password
+  -W, --password                     force password prompt
 
 Generic options:
-  -e, --echo                    echo queries
-  -E, --elevel=LEVEL            set output message level
-  --help                        show this help, then exit
-  --version                     output version information, then exit
+  -e, --echo                         echo queries
+  -E, --elevel=LEVEL                 set output message level
+  --help                             show this help, then exit
+  --version                          output version information, then exit
 
 
 Reorg Options

doc/pg_repack_jp.rst

@@ -197,39 +197,40 @@ pg_repackもしくはpg_reorgの古いバージョンからのアップグレー
   The following options can be specified in ``OPTIONS``.
   
   Options:
-    -a, --all                     repack all databases
-    -t, --table=TABLE             repack specific table only
-    -I, --parent-table=TABLE      repack specific parent table and its inheritors
-    -c, --schema=SCHEMA           repack tables in specific schema only
-    -s, --tablespace=TBLSPC       move repacked tables to a new tablespace
-    -S, --moveidx                 move repacked indexes to *TBLSPC* too
-    -o, --order-by=COLUMNS        order by columns instead of cluster keys
-    -n, --no-order                do vacuum full instead of cluster
-    -N, --dry-run                 print what would have been repacked and exit
-    -j, --jobs=NUM                Use this many parallel jobs for each table
-    -i, --index=INDEX             move only the specified index
-    -x, --only-indexes            move only indexes of the specified table
-    -T, --wait-timeout=SECS       timeout to cancel other backends on conflict
-    -D, --no-kill-backend         don't kill other backends when timed out
-    -Z, --no-analyze              don't analyze at end
-    -k, --no-superuser-check      skip superuser checks in client
-    -C, --exclude-extension       don't repack tables which belong to specific extension
-        --error-on-invalid-index  don't repack when invalid index is found
-        --switch-threshold        switch tables when that many tuples are left to catchup
+    -a, --all                          repack all databases
+    -t, --table=TABLE                  repack specific table only
+    -I, --parent-table=TABLE           repack specific parent table and its inheritors
+    -c, --schema=SCHEMA                repack tables in specific schema only
+    -s, --tablespace=TBLSPC            move repacked tables to a new tablespace
+    -S, --moveidx                      move repacked indexes to *TBLSPC* too
+    -o, --order-by=COLUMNS             order by columns instead of cluster keys
+    -n, --no-order                     do vacuum full instead of cluster
+    -N, --dry-run                      print what would have been repacked and exit
+    -j, --jobs=NUM                     Use this many parallel jobs for each table
+    -i, --index=INDEX                  move only the specified index
+    -x, --only-indexes                 move only indexes of the specified table
+    -T, --wait-timeout=SECS            timeout to cancel other backends on conflict
+    -D, --no-kill-backend              don't kill other backends when timed out
+    -Z, --no-analyze                   don't analyze at end
+    -k, --no-superuser-check           skip superuser checks in client
+    -C, --exclude-extension            don't repack tables which belong to specific extension
+        --no-error-on-invalid-index    repack even though invalid index is found
+        --error-on-invalid-index       don't repack when invalid index is found, deprecated, as this is the default behavior now
+        --switch-threshold             switch tables when that many tuples are left to catchup
   
   Connection options:
-    -d, --dbname=DBNAME           database to connect
-    -h, --host=HOSTNAME           database server host or socket directory
-    -p, --port=PORT               database server port
-    -U, --username=USERNAME       user name to connect as
-    -w, --no-password             never prompt for password
-    -W, --password                force password prompt
+    -d, --dbname=DBNAME                database to connect
+    -h, --host=HOSTNAME                database server host or socket directory
+    -p, --port=PORT                    database server port
+    -U, --username=USERNAME            user name to connect as
+    -w, --no-password                  never prompt for password
+    -W, --password                     force password prompt
   
   Generic options:
-    -e, --echo                    echo queries
-    -E, --elevel=LEVEL            set output message level
-    --help                        show this help, then exit
-    --version                     output version information, then exit
+    -e, --echo                         echo queries
+    -E, --elevel=LEVEL                 set output message level
+    --help                             show this help, then exit
+    --version                          output version information, then exit
 
 利用方法
 ---------

regress/Makefile

@@ -17,7 +17,7 @@ INTVERSION := $(shell echo $$(($$(echo $(VERSION).0 | sed 's/\([[:digit:]]\{1,\}
 # Test suite
 #
 
-REGRESS := init-extension repack-setup repack-run error-on-invalid-idx after-schema repack-check nosuper tablespace get_order_by trigger
+REGRESS := init-extension repack-setup repack-run error-on-invalid-idx no-error-on-invalid-idx after-schema repack-check nosuper tablespace get_order_by trigger
 
 USE_PGXS = 1	# use pgxs if not in contrib directory
 PGXS := $(shell $(PG_CONFIG) --pgxs)

regress/expected/no-error-on-invalid-idx.out

@@ -0,0 +1,22 @@
+--
+-- do repack
+--
+\! pg_repack --dbname=contrib_regression --table=tbl_cluster --no-error-on-invalid-index
+INFO: repacking table "public.tbl_cluster"
+\! pg_repack --dbname=contrib_regression --table=tbl_badindex --no-error-on-invalid-index
+INFO: repacking table "public.tbl_badindex"
+WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+\! pg_repack --dbname=contrib_regression --no-error-on-invalid-index
+INFO: repacking table "public.tbl_badindex"
+WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+INFO: repacking table "public.tbl_cluster"
+INFO: repacking table "public.tbl_gistkey"
+INFO: repacking table "public.tbl_idxopts"
+INFO: repacking table "public.tbl_incl_pkey"
+INFO: repacking table "public.tbl_only_pkey"
+INFO: repacking table "public.tbl_order"
+INFO: repacking table "public.tbl_storage_plain"
+INFO: repacking table "public.tbl_with_dropped_column"
+INFO: repacking table "public.tbl_with_dropped_toast"
+INFO: repacking table "public.tbl_with_mod_column_storage"
+INFO: repacking table "public.tbl_with_toast"

regress/expected/no-error-on-invalid-idx_1.out

@@ -0,0 +1,21 @@
+--
+-- do repack
+--
+\! pg_repack --dbname=contrib_regression --table=tbl_cluster --no-error-on-invalid-index
+INFO: repacking table "public.tbl_cluster"
+\! pg_repack --dbname=contrib_regression --table=tbl_badindex --no-error-on-invalid-index
+INFO: repacking table "public.tbl_badindex"
+WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+\! pg_repack --dbname=contrib_regression --no-error-on-invalid-index
+INFO: repacking table "public.tbl_badindex"
+WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+INFO: repacking table "public.tbl_cluster"
+INFO: repacking table "public.tbl_gistkey"
+INFO: repacking table "public.tbl_idxopts"
+INFO: repacking table "public.tbl_only_pkey"
+INFO: repacking table "public.tbl_order"
+INFO: repacking table "public.tbl_storage_plain"
+INFO: repacking table "public.tbl_with_dropped_column"
+INFO: repacking table "public.tbl_with_dropped_toast"
+INFO: repacking table "public.tbl_with_mod_column_storage"
+INFO: repacking table "public.tbl_with_toast"

regress/expected/repack-run.out

@@ -5,10 +5,10 @@
 INFO: repacking table "public.tbl_cluster"
 \! pg_repack --dbname=contrib_regression --table=tbl_badindex
 INFO: repacking table "public.tbl_badindex"
-WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+WARNING: Invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
 \! pg_repack --dbname=contrib_regression
 INFO: repacking table "public.tbl_badindex"
-WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+WARNING: Invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
 INFO: repacking table "public.tbl_cluster"
 INFO: repacking table "public.tbl_gistkey"
 INFO: repacking table "public.tbl_idxopts"

regress/expected/repack-run_1.out

@@ -5,10 +5,10 @@
 INFO: repacking table "public.tbl_cluster"
 \! pg_repack --dbname=contrib_regression --table=tbl_badindex
 INFO: repacking table "public.tbl_badindex"
-WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+WARNING: Invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
 \! pg_repack --dbname=contrib_regression
 INFO: repacking table "public.tbl_badindex"
-WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
+WARNING: Invalid index: CREATE UNIQUE INDEX idx_badindex_n ON public.tbl_badindex USING btree (n)
 INFO: repacking table "public.tbl_cluster"
 INFO: repacking table "public.tbl_gistkey"
 INFO: repacking table "public.tbl_idxopts"

regress/sql/no-error-on-invalid-idx.sql

@@ -0,0 +1,7 @@
+--
+-- do repack
+--
+
+\! pg_repack --dbname=contrib_regression --table=tbl_cluster --no-error-on-invalid-index
+\! pg_repack --dbname=contrib_regression --table=tbl_badindex --no-error-on-invalid-index
+\! pg_repack --dbname=contrib_regression --no-error-on-invalid-index