BUG #10823: Better REINDEX syntax.
The following bug has been logged on the website:
Bug reference: 10823
Logged by: Daniel Migowski
Email address: dmigowski@ikoffice.de
PostgreSQL version: 9.1.13
Operating system: n/a
Description:
Hello.
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?
PS: Thanks for all your work on this great database!
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski@ikoffice.de wrote:
The following bug has been logged on the website:
Bug reference: 10823
Logged by: Daniel Migowski
Email address: dmigowski@ikoffice.de
PostgreSQL version: 9.1.13
Operating system: n/a
Description:Hello.
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?
Wow, yeah, that is kind of odd, e.g.
REINDEX { INDEX | TABLE | DATABASE | SYSTEM } name [ FORCE ]
...
name
The name of the specific index, table, or database
to be reindexed. Index and table names can be
schema-qualified. Presently, REINDEX DATABASE and REINDEX SYSTEM
can only reindex the current database, so their parameter must
match the current database's name.
Let me look at improving that for 9.5.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski@ikoffice.de wrote:
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?
Wow, yeah, that is kind of odd, e.g.
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski@ikoffice.de wrote:
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?Wow, yeah, that is kind of odd, e.g.
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.
Uh, do we encourage database-wide VACUUM FULL or CLUSTER, as we use them
there with no parameter. Is there a reason REINDEX should be harder,
and require a dummy argument to run?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.
Uh, do we encourage database-wide VACUUM FULL or CLUSTER, as we use them
there with no parameter. Is there a reason REINDEX should be harder,
and require a dummy argument to run?
I believe that REINDEX on system catalogs carries a risk of deadlock
failures against other processes --- there was a recent example of that
in the mailing lists. VACUUM FULL has such risks too, but that's been
pretty well deprecated for many years. (I think CLUSTER is probably
relatively safe on this score because it's not going to think any system
catalogs are clustered.)
If there were a variant of REINDEX that only hit user tables, I'd be fine
with making that easy to invoke.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 07/30/2014 07:35 PM, Bruce Momjian wrote:
On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski@ikoffice.de wrote:
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?Wow, yeah, that is kind of odd, e.g.
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.Uh, do we encourage database-wide VACUUM FULL or CLUSTER, as we use them
there with no parameter. Is there a reason REINDEX should be harder,
and require a dummy argument to run?
I agree. The request isn't for a naked REINDEX command, it's for a
naked REINDEX DATABASE command.
--
Vik
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On Wed, Jul 30, 2014 at 07:48:39PM +0200, Vik Fearing wrote:
On 07/30/2014 07:35 PM, Bruce Momjian wrote:
On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski@ikoffice.de wrote:
Compared to CLUSTER and VACUUM FULL we need to specify a database to the
REINDEX command. Why? It would be logical to reindex the current database,
exactly like CLUSTER does. So why isn't the DATABASE parameter optional?Wow, yeah, that is kind of odd, e.g.
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.Uh, do we encourage database-wide VACUUM FULL or CLUSTER, as we use them
there with no parameter. Is there a reason REINDEX should be harder,
and require a dummy argument to run?I agree. The request isn't for a naked REINDEX command, it's for a
naked REINDEX DATABASE command.
Yes, the question is should we support REINDEX DATABASE without a
database name that matches the current database. REINDEX alone might be
too risky.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 07/30/2014 07:46 PM, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
I don't find it all that odd. We should not be encouraging routine
database-wide reindexes.Uh, do we encourage database-wide VACUUM FULL or CLUSTER, as we use them
there with no parameter. Is there a reason REINDEX should be harder,
and require a dummy argument to run?I believe that REINDEX on system catalogs carries a risk of deadlock
failures against other processes --- there was a recent example of that
in the mailing lists. VACUUM FULL has such risks too, but that's been
pretty well deprecated for many years. (I think CLUSTER is probably
relatively safe on this score because it's not going to think any system
catalogs are clustered.)If there were a variant of REINDEX that only hit user tables, I'd be fine
with making that easy to invoke.
Here are two patches for this.
The first one, reindex_user_tables.v1.patch, implements the variant that
only hits user tables, as suggested by you.
The second one, reindex_no_dbname.v1.patch, allows the three
database-wide variants to omit the database name (voted for by Daniel
Migowski, Bruce, and myself; voted against by you). This patch is to be
applied on top of the first one.
--
Vik
Attachments:
reindex_user_tables.v1.patchtext/x-diff; name=reindex_user_tables.v1.patchDownload
*** a/doc/src/sgml/ref/reindex.sgml
--- b/doc/src/sgml/ref/reindex.sgml
***************
*** 21,27 **** PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
! REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">name</replaceable> [ FORCE ]
</synopsis>
</refsynopsisdiv>
--- 21,27 ----
<refsynopsisdiv>
<synopsis>
! REINDEX { INDEX | TABLE | DATABASE | SYSTEM | USER TABLES } <replaceable class="PARAMETER">name</replaceable> [ FORCE ]
</synopsis>
</refsynopsisdiv>
***************
*** 126,139 **** REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">nam
</varlistentry>
<varlistentry>
<term><replaceable class="PARAMETER">name</replaceable></term>
<listitem>
<para>
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
! Presently, <command>REINDEX DATABASE</> and <command>REINDEX SYSTEM</>
! can only reindex the current database, so their parameter must match
! the current database's name.
</para>
</listitem>
</varlistentry>
--- 126,151 ----
</varlistentry>
<varlistentry>
+ <term><literal>USER TABLES</literal></term>
+ <listitem>
+ <para>
+ Recreate all indexes on user tables within the current database.
+ Indexes on system catalogs are not processed.
+ This form of <command>REINDEX</command> cannot be executed inside a
+ transaction block.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><replaceable class="PARAMETER">name</replaceable></term>
<listitem>
<para>
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
! Presently, <command>REINDEX DATABASE</>, <command>REINDEX SYSTEM</>,
! and <command>REINDEX USER TABLES</> can only reindex the current
! database, so their parameter must match the current database's name.
</para>
</listitem>
</varlistentry>
*** a/doc/src/sgml/ref/reindexdb.sgml
--- b/doc/src/sgml/ref/reindexdb.sgml
***************
*** 65,70 **** PostgreSQL documentation
--- 65,80 ----
</group>
<arg choice="opt"><replaceable>dbname</replaceable></arg>
</cmdsynopsis>
+
+ <cmdsynopsis>
+ <command>reindexdb</command>
+ <arg rep="repeat"><replaceable>connection-option</replaceable></arg>
+ <group choice="plain">
+ <arg choice="plain"><option>--usertables</option></arg>
+ <arg choice="plain"><option>-u</option></arg>
+ </group>
+ <arg choice="opt"><replaceable>dbname</replaceable></arg>
+ </cmdsynopsis>
</refsynopsisdiv>
***************
*** 173,178 **** PostgreSQL documentation
--- 183,198 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-u</></term>
+ <term><option>--usertables</></term>
+ <listitem>
+ <para>
+ Reindex database's user tables.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-V</></term>
<term><option>--version</></term>
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 6984,6989 **** ReindexStmt:
--- 6984,6999 ----
n->do_user = true;
$$ = (Node *)n;
}
+ | REINDEX USER TABLES name opt_force
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = OBJECT_DATABASE;
+ n->name = $4;
+ n->relation = NULL;
+ n->do_system = false;
+ n->do_user = true;
+ $$ = (Node *)n;
+ }
;
reindex_type:
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***************
*** 3145,3151 **** psql_completion(const char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
{
static const char *const list_REINDEX[] =
! {"TABLE", "INDEX", "SYSTEM", "DATABASE", NULL};
COMPLETE_WITH_LIST(list_REINDEX);
}
--- 3145,3151 ----
else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
{
static const char *const list_REINDEX[] =
! {"TABLE", "INDEX", "SYSTEM", "DATABASE", "USER TABLES", NULL};
COMPLETE_WITH_LIST(list_REINDEX);
}
***************
*** 3158,3163 **** psql_completion(const char *text, int start, int end)
--- 3158,3171 ----
else if (pg_strcasecmp(prev_wd, "SYSTEM") == 0 ||
pg_strcasecmp(prev_wd, "DATABASE") == 0)
COMPLETE_WITH_QUERY(Query_for_list_of_databases);
+ else if (pg_strcasecmp(prev_wd, "USER") == 0)
+ COMPLETE_WITH_CONST("TABLES");
+ }
+ else if (pg_strcasecmp(prev3_wd, "REINDEX") == 0)
+ {
+ if (pg_strcasecmp(prev2_wd, "USER") == 0 &&
+ pg_strcasecmp(prev_wd, "TABLES") == 0)
+ COMPLETE_WITH_QUERY(Query_for_list_of_databases);
}
/* SECURITY LABEL */
*** a/src/bin/scripts/reindexdb.c
--- b/src/bin/scripts/reindexdb.c
***************
*** 28,35 **** static void reindex_system_catalogs(const char *dbname,
--- 28,41 ----
const char *host, const char *port,
const char *username, enum trivalue prompt_password,
const char *progname, bool echo);
+ static void reindex_user_tables(const char *dbname,
+ const char *host, const char *port,
+ const char *username, enum trivalue prompt_password,
+ const char *progname, bool echo);
static void help(const char *progname);
+ #define exit_nicely(code) exit(code)
+
int
main(int argc, char *argv[])
{
***************
*** 44,49 **** main(int argc, char *argv[])
--- 50,56 ----
{"dbname", required_argument, NULL, 'd'},
{"all", no_argument, NULL, 'a'},
{"system", no_argument, NULL, 's'},
+ {"usertables", no_argument, NULL, 'u'},
{"table", required_argument, NULL, 't'},
{"index", required_argument, NULL, 'i'},
{"maintenance-db", required_argument, NULL, 2},
***************
*** 61,66 **** main(int argc, char *argv[])
--- 68,74 ----
const char *username = NULL;
enum trivalue prompt_password = TRI_DEFAULT;
bool syscatalog = false;
+ bool usertables = false;
bool alldb = false;
bool echo = false;
bool quiet = false;
***************
*** 73,79 **** main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "reindexdb", help);
/* process command-line options */
! while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:ast:i:", long_options, &optindex)) != -1)
{
switch (c)
{
--- 81,87 ----
handle_help_version_opts(argc, argv, "reindexdb", help);
/* process command-line options */
! while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:asut:i:", long_options, &optindex)) != -1)
{
switch (c)
{
***************
*** 107,112 **** main(int argc, char *argv[])
--- 115,123 ----
case 's':
syscatalog = true;
break;
+ case 'u':
+ usertables = true;
+ break;
case 't':
simple_string_list_append(&tables, optarg);
break;
***************
*** 194,199 **** main(int argc, char *argv[])
--- 205,236 ----
reindex_system_catalogs(dbname, host, port, username, prompt_password,
progname, echo);
}
+ else if (usertables)
+ {
+ if (tables.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific table(s) and all user tables at the same time\n"), progname);
+ exit(1);
+ }
+ if (indexes.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific index(es) and all user tables at the same time\n"), progname);
+ exit(1);
+ }
+
+ if (dbname == NULL)
+ {
+ if (getenv("PGDATABASE"))
+ dbname = getenv("PGDATABASE");
+ else if (getenv("PGUSER"))
+ dbname = getenv("PGUSER");
+ else
+ dbname = get_user_name_or_exit(progname);
+ }
+
+ reindex_user_tables(dbname, host, port, username, prompt_password,
+ progname, echo);
+ }
else
{
if (dbname == NULL)
***************
*** 336,341 **** reindex_system_catalogs(const char *dbname, const char *host, const char *port,
--- 373,413 ----
}
static void
+ reindex_user_tables(const char *dbname, const char *host, const char *port,
+ const char *username, enum trivalue prompt_password,
+ const char *progname, bool echo)
+ {
+ PQExpBufferData sql;
+
+ PGconn *conn;
+
+ initPQExpBuffer(&sql);
+
+ appendPQExpBuffer(&sql, "REINDEX USER TABLES %s;", dbname);
+
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false);
+
+ /* This feature is only available starting in 9.5 */
+ if (PQserverVersion(conn) < 90500)
+ {
+ fprintf(stderr, _("%s: REINDEX USER TABLES is only available in server versions 9.5 and newer\n"),
+ progname);
+ exit_nicely(1);
+ }
+
+ if (!executeMaintenanceCommand(conn, sql.data, echo))
+ {
+ fprintf(stderr, _("%s: reindexing of user tables failed: %s"),
+ progname, PQerrorMessage(conn));
+ PQfinish(conn);
+ exit(1);
+ }
+ PQfinish(conn);
+ termPQExpBuffer(&sql);
+ }
+
+ static void
help(const char *progname)
{
printf(_("%s reindexes a PostgreSQL database.\n\n"), progname);
***************
*** 348,353 **** help(const char *progname)
--- 420,426 ----
printf(_(" -i, --index=INDEX recreate specific index(es) only\n"));
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" -s, --system reindex system catalogs\n"));
+ printf(_(" -u, --usertables reindex all user tables\n"));
printf(_(" -t, --table=TABLE reindex specific table(s) only\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
reindex_no_dbname.v1.patchtext/x-diff; name=reindex_no_dbname.v1.patchDownload
*** a/doc/src/sgml/ref/reindex.sgml
--- b/doc/src/sgml/ref/reindex.sgml
***************
*** 22,27 **** PostgreSQL documentation
--- 22,28 ----
<refsynopsisdiv>
<synopsis>
REINDEX { INDEX | TABLE | DATABASE | SYSTEM | USER TABLES } <replaceable class="PARAMETER">name</replaceable> [ FORCE ]
+ REINDEX { DATABASE | SYSTEM | USER TABLES }
</synopsis>
</refsynopsisdiv>
***************
*** 145,151 **** REINDEX { INDEX | TABLE | DATABASE | SYSTEM | USER TABLES } <replaceable class="
reindexed. Index and table names can be schema-qualified.
Presently, <command>REINDEX DATABASE</>, <command>REINDEX SYSTEM</>,
and <command>REINDEX USER TABLES</> can only reindex the current
! database, so their parameter must match the current database's name.
</para>
</listitem>
</varlistentry>
--- 146,153 ----
reindexed. Index and table names can be schema-qualified.
Presently, <command>REINDEX DATABASE</>, <command>REINDEX SYSTEM</>,
and <command>REINDEX USER TABLES</> can only reindex the current
! database, so their parameter must match the current database's name
! if provided.
</para>
</listitem>
</varlistentry>
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 1784,1799 **** ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
List *relids = NIL;
ListCell *l;
! AssertArg(databaseName);
!
! if (strcmp(databaseName, get_database_name(MyDatabaseId)) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
if (!pg_database_ownercheck(MyDatabaseId, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! databaseName);
/*
* Create a memory context that will survive forced transaction commits we
--- 1784,1797 ----
List *relids = NIL;
ListCell *l;
! if (databaseName != NULL && strcmp(databaseName, get_database_name(MyDatabaseId)) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
if (!pg_database_ownercheck(MyDatabaseId, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! get_database_name(MyDatabaseId));
/*
* Create a memory context that will survive forced transaction commits we
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 6974,6979 **** ReindexStmt:
--- 6974,6989 ----
n->do_user = false;
$$ = (Node *)n;
}
+ | REINDEX SYSTEM_P
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = OBJECT_DATABASE;
+ n->name = NULL;
+ n->relation = NULL;
+ n->do_system = true;
+ n->do_user = false;
+ $$ = (Node *)n;
+ }
| REINDEX DATABASE name opt_force
{
ReindexStmt *n = makeNode(ReindexStmt);
***************
*** 6984,6989 **** ReindexStmt:
--- 6994,7009 ----
n->do_user = true;
$$ = (Node *)n;
}
+ | REINDEX DATABASE
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = OBJECT_DATABASE;
+ n->name = NULL;
+ n->relation = NULL;
+ n->do_system = true;
+ n->do_user = true;
+ $$ = (Node *)n;
+ }
| REINDEX USER TABLES name opt_force
{
ReindexStmt *n = makeNode(ReindexStmt);
***************
*** 6994,6999 **** ReindexStmt:
--- 7014,7029 ----
n->do_user = true;
$$ = (Node *)n;
}
+ | REINDEX USER TABLES
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = OBJECT_DATABASE;
+ n->name = NULL;
+ n->relation = NULL;
+ n->do_system = false;
+ n->do_user = true;
+ $$ = (Node *)n;
+ }
;
reindex_type:
Vik Fearing wrote:
Here are two patches for this.
The first one, reindex_user_tables.v1.patch, implements the variant that
only hits user tables, as suggested by you.The second one, reindex_no_dbname.v1.patch, allows the three
database-wide variants to omit the database name (voted for by Daniel
Migowski, Bruce, and myself; voted against by you). This patch is to be
applied on top of the first one.
Not a fan. Here's a revised version that provides REINDEX USER TABLES,
which can only be used without a database name; other modes are not
affected i.e. they continue to require a database name. I also renamed
your proposed reindexdb's --usertables to --user-tables.
Oh, I just noticed that if you say reindexdb --all --user-tables, the
latter is not honored. Must fix before commit.
Makes sense?
Note: I don't like the reindexdb UI; if you just run "reindexdb -d
foobar" it will reindex everything, including system catalogs. I think
USER TABLES should be the default operation mode for reindex. If you
want plain old "REINDEX DATABASE foobar" which also hits the catalogs,
you should request that separately (how?). This patch doesn't change
this.
Also note: if you say "user tables", information_schema is reindexed too,
which kinda sucks.
Further note: this command is probably pointless in the majority of
cases. Somebody should spend some serious time with REINDEX
CONCURRENTLY ..
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
reindex_user_tables.v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index cabae19..d05e1ac 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">name</replaceable> [ FORCE ]
+REINDEX USER TABLES
</synopsis>
</refsynopsisdiv>
@@ -126,14 +127,26 @@ REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">nam
</varlistentry>
<varlistentry>
+ <term><literal>USER TABLES</literal></term>
+ <listitem>
+ <para>
+ Recreate all indexes on user tables within the current database.
+ Indexes on system catalogs are not processed.
+ This form of <command>REINDEX</command> cannot be executed inside a
+ transaction block.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><replaceable class="PARAMETER">name</replaceable></term>
<listitem>
<para>
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
- Presently, <command>REINDEX DATABASE</> and <command>REINDEX SYSTEM</>
- can only reindex the current database, so their parameter must match
- the current database's name.
+ Presently, <command>REINDEX DATABASE</>, <command>REINDEX SYSTEM</>,
+ and <command>REINDEX USER TABLES</> can only reindex the current
+ database, so their parameter must match the current database's name.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 486f5c9..f69d84b 100644
--- a/doc/src/sgml/ref/reindexdb.sgml
+++ b/doc/src/sgml/ref/reindexdb.sgml
@@ -65,6 +65,15 @@ PostgreSQL documentation
</group>
<arg choice="opt"><replaceable>dbname</replaceable></arg>
</cmdsynopsis>
+
+ <cmdsynopsis>
+ <command>reindexdb</command>
+ <arg rep="repeat"><replaceable>connection-option</replaceable></arg>
+ <group choice="plain">
+ <arg choice="plain"><option>--user-tables</option></arg>
+ <arg choice="plain"><option>-u</option></arg>
+ </group>
+ </cmdsynopsis>
</refsynopsisdiv>
@@ -173,6 +182,16 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-u</></term>
+ <term><option>--user-tables</></term>
+ <listitem>
+ <para>
+ Reindex database's user tables.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-V</></term>
<term><option>--version</></term>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fdfa6ca..23e13f0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1772,6 +1772,9 @@ ReindexTable(RangeVar *relation)
* To reduce the probability of deadlocks, each table is reindexed in a
* separate transaction, so we can release the lock on it right away.
* That means this must not be called within a user transaction block!
+ *
+ * databaseName can be NULL when do_user is set and do_system isn't; this
+ * is the REINDEX USER TABLES case.
*/
Oid
ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
@@ -1784,9 +1787,10 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
List *relids = NIL;
ListCell *l;
- AssertArg(databaseName);
+ AssertArg(databaseName || (do_user && !do_system));
- if (strcmp(databaseName, get_database_name(MyDatabaseId)) != 0)
+ if (databaseName &&
+ strcmp(databaseName, get_database_name(MyDatabaseId)) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6f4d645..0baef83 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -7040,6 +7040,7 @@ opt_if_exists: IF_P EXISTS { $$ = TRUE; }
* QUERY:
*
* REINDEX type <name> [FORCE]
+ * REINDEX USER TABLES
*
* FORCE no longer does anything, but we accept it for backwards compatibility
*****************************************************************************/
@@ -7073,6 +7074,16 @@ ReindexStmt:
n->do_user = true;
$$ = (Node *)n;
}
+ | REINDEX USER TABLES
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = OBJECT_DATABASE;
+ n->name = NULL;
+ n->relation = NULL;
+ n->do_system = false;
+ n->do_user = true;
+ $$ = (Node *)n;
+ }
;
reindex_type:
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 76b2b04..490d200 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3148,7 +3148,7 @@ psql_completion(const char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
{
static const char *const list_REINDEX[] =
- {"TABLE", "INDEX", "SYSTEM", "DATABASE", NULL};
+ {"TABLE", "INDEX", "SYSTEM", "DATABASE", "USER TABLES", NULL};
COMPLETE_WITH_LIST(list_REINDEX);
}
@@ -3161,6 +3161,8 @@ psql_completion(const char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, "SYSTEM") == 0 ||
pg_strcasecmp(prev_wd, "DATABASE") == 0)
COMPLETE_WITH_QUERY(Query_for_list_of_databases);
+ else if (pg_strcasecmp(prev_wd, "USER") == 0)
+ COMPLETE_WITH_CONST("TABLES");
}
/* SECURITY LABEL */
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 561bbce..8e1b52d 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -28,8 +28,14 @@ static void reindex_system_catalogs(const char *dbname,
const char *host, const char *port,
const char *username, enum trivalue prompt_password,
const char *progname, bool echo);
+static void reindex_user_tables(const char *dbname,
+ const char *host, const char *port,
+ const char *username, enum trivalue prompt_password,
+ const char *progname, bool echo);
static void help(const char *progname);
+#define exit_nicely(code) exit(code)
+
int
main(int argc, char *argv[])
{
@@ -46,6 +52,7 @@ main(int argc, char *argv[])
{"system", no_argument, NULL, 's'},
{"table", required_argument, NULL, 't'},
{"index", required_argument, NULL, 'i'},
+ {"user-tables", no_argument, NULL, 'u'},
{"maintenance-db", required_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
@@ -61,6 +68,7 @@ main(int argc, char *argv[])
const char *username = NULL;
enum trivalue prompt_password = TRI_DEFAULT;
bool syscatalog = false;
+ bool usertables = false;
bool alldb = false;
bool echo = false;
bool quiet = false;
@@ -73,7 +81,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "reindexdb", help);
/* process command-line options */
- while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:ast:i:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:ast:i:u", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -113,6 +121,9 @@ main(int argc, char *argv[])
case 'i':
simple_string_list_append(&indexes, optarg);
break;
+ case 'u':
+ usertables = true;
+ break;
case 2:
maintenance_db = pg_strdup(optarg);
break;
@@ -194,6 +205,32 @@ main(int argc, char *argv[])
reindex_system_catalogs(dbname, host, port, username, prompt_password,
progname, echo);
}
+ else if (usertables)
+ {
+ if (tables.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific table(s) and all user tables at the same time\n"), progname);
+ exit(1);
+ }
+ if (indexes.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific index(es) and all user tables at the same time\n"), progname);
+ exit(1);
+ }
+
+ if (dbname == NULL)
+ {
+ if (getenv("PGDATABASE"))
+ dbname = getenv("PGDATABASE");
+ else if (getenv("PGUSER"))
+ dbname = getenv("PGUSER");
+ else
+ dbname = get_user_name_or_exit(progname);
+ }
+
+ reindex_user_tables(dbname, host, port, username, prompt_password,
+ progname, echo);
+ }
else
{
if (dbname == NULL)
@@ -336,6 +373,41 @@ reindex_system_catalogs(const char *dbname, const char *host, const char *port,
}
static void
+reindex_user_tables(const char *dbname, const char *host, const char *port,
+ const char *username, enum trivalue prompt_password,
+ const char *progname, bool echo)
+{
+ PQExpBufferData sql;
+
+ PGconn *conn;
+
+ initPQExpBuffer(&sql);
+
+ appendPQExpBuffer(&sql, "REINDEX USER TABLES;");
+
+ conn = connectDatabase(dbname, host, port, username, prompt_password,
+ progname, false);
+
+ /* This feature is only available starting in 9.5 */
+ if (PQserverVersion(conn) < 90500)
+ {
+ fprintf(stderr, _("%s: REINDEX USER TABLES is only available in server versions 9.5 and newer\n"),
+ progname);
+ exit_nicely(1);
+ }
+
+ if (!executeMaintenanceCommand(conn, sql.data, echo))
+ {
+ fprintf(stderr, _("%s: reindexing of user tables failed: %s"),
+ progname, PQerrorMessage(conn));
+ PQfinish(conn);
+ exit(1);
+ }
+ PQfinish(conn);
+ termPQExpBuffer(&sql);
+}
+
+static void
help(const char *progname)
{
printf(_("%s reindexes a PostgreSQL database.\n\n"), progname);
@@ -349,6 +421,7 @@ help(const char *progname)
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" -s, --system reindex system catalogs\n"));
printf(_(" -t, --table=TABLE reindex specific table(s) only\n"));
+ printf(_(" -u, --user-tables reindex all user tables\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nConnection options:\n"));
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d2c0b29..226fffd 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2689,9 +2689,9 @@ typedef struct ConstraintsSetStmt
typedef struct ReindexStmt
{
NodeTag type;
- ObjectType kind; /* OBJECT_INDEX, OBJECT_TABLE, etc. */
- RangeVar *relation; /* Table or index to reindex */
- const char *name; /* name of database to reindex */
+ ObjectType kind; /* OBJECT_INDEX, OBJECT_TABLE, OBJECT_DATABASE */
+ RangeVar *relation; /* Table or index to reindex; NULL for all */
+ const char *name; /* name of database to reindex, or NULL */
bool do_system; /* include system tables in database case */
bool do_user; /* include user tables in database case */
} ReindexStmt;
On 2014-08-29 01:00, Alvaro Herrera wrote:
Vik Fearing wrote:
Here are two patches for this.
The first one, reindex_user_tables.v1.patch, implements the variant that
only hits user tables, as suggested by you.The second one, reindex_no_dbname.v1.patch, allows the three
database-wide variants to omit the database name (voted for by Daniel
Migowski, Bruce, and myself; voted against by you). This patch is to be
applied on top of the first one.Not a fan. Here's a revised version that provides REINDEX USER TABLES,
which can only be used without a database name; other modes are not
affected i.e. they continue to require a database name.
Yeah, I think I like this better than allowing all of them without the
database name.
I also renamed
your proposed reindexdb's --usertables to --user-tables.
I agree with this change.
Oh, I just noticed that if you say reindexdb --all --user-tables, the
latter is not honored. Must fix before commit.
Definitely.
Note: I don't like the reindexdb UI; if you just run "reindexdb -d
foobar" it will reindex everything, including system catalogs. I think
USER TABLES should be the default operation mode for reindex. If you
want plain old "REINDEX DATABASE foobar" which also hits the catalogs,
you should request that separately (how?). This patch doesn't change
this.
This should probably be a separate patch if it's going to happen. But
the idea seems reasonable.
Also note: if you say "user tables", information_schema is reindexed too,
which kinda sucks.
*shrug* It sort of makes sense if you think of this as the opposite of
REINDEX SYSTEM. I'm not at all sure whether including or excluding it
would be the better choice here.
Do we have some kind of an agreement on what this patch should look
like? Is someone going to prepare an updated patch? Vik?
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Marko Tiikkaja wrote:
On 2014-08-29 01:00, Alvaro Herrera wrote:
Note: I don't like the reindexdb UI; if you just run "reindexdb -d
foobar" it will reindex everything, including system catalogs. I think
USER TABLES should be the default operation mode for reindex. If you
want plain old "REINDEX DATABASE foobar" which also hits the catalogs,
you should request that separately (how?). This patch doesn't change
this.This should probably be a separate patch if it's going to happen.
Yeh, no argument there.
Also note: if you say "user tables", information_schema is reindexed too,
which kinda sucks.*shrug* It sort of makes sense if you think of this as the opposite
of REINDEX SYSTEM. I'm not at all sure whether including or
excluding it would be the better choice here.
Yeah, probably not worth bothering.
Do we have some kind of an agreement on what this patch should look
like? Is someone going to prepare an updated patch? Vik?
I think the only issue left for this to be committable is reindexdb
--all previously mentioned.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-09-02 22:24, Alvaro Herrera wrote:
Marko Tiikkaja wrote:
Do we have some kind of an agreement on what this patch should look
like? Is someone going to prepare an updated patch? Vik?I think the only issue left for this to be committable is reindexdb
--all previously mentioned.
I scanned through the patch and found the exit_nicely() business a bit
weird, so that might be another thing worth looking at.
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Marko Tiikkaja wrote:
On 2014-09-02 22:24, Alvaro Herrera wrote:
Marko Tiikkaja wrote:
Do we have some kind of an agreement on what this patch should look
like? Is someone going to prepare an updated patch? Vik?I think the only issue left for this to be committable is reindexdb
--all previously mentioned.I scanned through the patch and found the exit_nicely() business a
bit weird, so that might be another thing worth looking at.
Yeah, just rip that out and do PQfinish(conn); exit(1); as other exit
paths do, I'd think.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/02/2014 10:17 PM, Marko Tiikkaja wrote:
On 2014-08-29 01:00, Alvaro Herrera wrote:
Vik Fearing wrote:
Here are two patches for this.
The first one, reindex_user_tables.v1.patch, implements the variant that
only hits user tables, as suggested by you.The second one, reindex_no_dbname.v1.patch, allows the three
database-wide variants to omit the database name (voted for by Daniel
Migowski, Bruce, and myself; voted against by you). This patch is to be
applied on top of the first one.Not a fan. Here's a revised version that provides REINDEX USER TABLES,
which can only be used without a database name; other modes are not
affected i.e. they continue to require a database name.Yeah, I think I like this better than allowing all of them without the
database name.
Why? It's just a noise word!
I also renamed
your proposed reindexdb's --usertables to --user-tables.I agree with this change.
Me, too.
Oh, I just noticed that if you say reindexdb --all --user-tables, the
latter is not honored. Must fix before commit.Definitely.
Okay, I'll look at that.
Is someone going to prepare an updated patch? Vik?
Yes, I will update the patch.
--
Vik
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Vik Fearing (vik.fearing@dalibo.com) wrote:
On 09/02/2014 10:17 PM, Marko Tiikkaja wrote:
Yeah, I think I like this better than allowing all of them without the
database name.Why? It's just a noise word!
Eh, because it ends up reindexing system tables too, which is probably
not what new folks are expecting. Also, it's not required when you say
'user tables', so it's similar to your user_tables v1 patch in that
regard.
Yes, I will update the patch.
Still planning to do this..?
Marking this back to waiting-for-author.
Thanks!
Stephen
On 09/08/2014 06:17 AM, Stephen Frost wrote:
* Vik Fearing (vik.fearing@dalibo.com) wrote:
On 09/02/2014 10:17 PM, Marko Tiikkaja wrote:
Yeah, I think I like this better than allowing all of them without the
database name.Why? It's just a noise word!
Eh, because it ends up reindexing system tables too, which is probably
not what new folks are expecting.
No behavior is changed at all. REINDEX DATABASE dbname; has always hit
the system tables. Since dbname can *only* be the current database,
there's no logic nor benefit in requiring it to be specified.
Also, it's not required when you say
'user tables', so it's similar to your user_tables v1 patch in that
regard.
The fact that REINDEX USER TABLES; is the only one that doesn't require
the dbname seems very inconsistent and confusing.
Yes, I will update the patch.
Still planning to do this..?
Marking this back to waiting-for-author.
Yes, but probably not for this commitfest unfortunately.
--
Vik
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Vik Fearing (vik.fearing@dalibo.com) wrote:
On 09/08/2014 06:17 AM, Stephen Frost wrote:
* Vik Fearing (vik.fearing@dalibo.com) wrote:
On 09/02/2014 10:17 PM, Marko Tiikkaja wrote:
Yeah, I think I like this better than allowing all of them without the
database name.Why? It's just a noise word!
Eh, because it ends up reindexing system tables too, which is probably
not what new folks are expecting.No behavior is changed at all. REINDEX DATABASE dbname; has always hit
the system tables. Since dbname can *only* be the current database,
there's no logic nor benefit in requiring it to be specified.
Sure, but I think the point is that reindexing the system tables as part
of a database-wide reindex is a *bad* thing which we shouldn't be
encouraging by making it easier.
I realize you're a bit 'stuck' here because we don't like the current
behavior, but we don't want to change it either.
Also, it's not required when you say
'user tables', so it's similar to your user_tables v1 patch in that
regard.The fact that REINDEX USER TABLES; is the only one that doesn't require
the dbname seems very inconsistent and confusing.
I understand, but the alternative would be a 'reindex;' which *doesn't*
reindex the system tables- would that be less confusing? Or getting rid
of the current 'reindex database' which also reindexes system tables...
Yes, I will update the patch.
Still planning to do this..?
Marking this back to waiting-for-author.
Yes, but probably not for this commitfest unfortunately.
Fair enough, I'll mark it 'returned with feedback'.
Thanks!
Stephen
Stephen Frost wrote:
Yes, I will update the patch.
Still planning to do this..?
Marking this back to waiting-for-author.
Yes, but probably not for this commitfest unfortunately.
Fair enough, I'll mark it 'returned with feedback'.
We lost this patch for the October commitfest, didn't we?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
We lost this patch for the October commitfest, didn't we?
I'm guessing you missed that a new version just got submitted..?
I'd be fine with today's being added to the october commitfest..
Of course, there's a whole independent discussion to be had about how
there wasn't any break between last commitfest and this one, but that
probably deserves its own thread.
THanks,
Stephen
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
We lost this patch for the October commitfest, didn't we?
I'm guessing you missed that a new version just got submitted..?
Which one, reindex schema? Isn't that a completely different patch?
I'd be fine with today's being added to the october commitfest..
Of course, there's a whole independent discussion to be had about how
there wasn't any break between last commitfest and this one, but that
probably deserves its own thread.
It's not the first that that happens, and honestly I don't see all that
much cause for concern. Heikki did move pending patches to the current
one, and closed a lot of inactive ones as 'returned with feedback'.
Attentive patch authors should have submitted new versions ... if they
don't, then someone else with an interest in the patch should do so.
If no one update the patches, what do we want them for?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
We lost this patch for the October commitfest, didn't we?
I'm guessing you missed that a new version just got submitted..?
Which one, reindex schema? Isn't that a completely different patch?
Err, sorry, wasn't looking close enough, evidently. :/
I'd be fine with today's being added to the october commitfest..
Of course, there's a whole independent discussion to be had about how
there wasn't any break between last commitfest and this one, but that
probably deserves its own thread.It's not the first that that happens, and honestly I don't see all that
much cause for concern. Heikki did move pending patches to the current
one, and closed a lot of inactive ones as 'returned with feedback'.
Inactive due to lack of review is the concern, but there is also a
concern that it's intended as a way to ensure committers have time to
work on their own patches instead of just working on patches submitted
through the commitfest process. Now, I think we all end up trying to
balance making progress on our own patches while also providing help to
the commitfest, but that's the situation we were in constantly before
the commitfest process was put in place because it didn't scale very
well.
If we're always in 'commitfest' mode then we might as well eliminate the
notion of timing them.
Attentive patch authors should have submitted new versions ... if they
don't, then someone else with an interest in the patch should do so.
If no one update the patches, what do we want them for?
As for this, sure, if there's a review and no response then it's fair to
mark the patch as returned with feedback. The issue is both when no
patch gets a review and when the commitfest never ends.
Thanks,
Stephen