On Sat, May 11, 2019 at 6:04 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Fri, May 10, 2019 at 09:25:58PM -0400, Tom Lane wrote:
> > Michael Paquier <mich...@paquier.xyz> writes:
> > > The refactoring bits are fine for HEAD. For back-branches I would
> > > suggest using the simplest patch of upthread.
> >
> > Makes sense to me too. The refactoring is mostly to make future
> > additions easier, so there's not much point for back branches.
>
> For now, I have committed and back-patched all the way down the bug
> fix.
Thanks!
> The refactoring is also kind of nice so I'll be happy to look at
> an updated patch. At the same time, let's get rid of
> reindex_system_catalogs() and integrate it with reindex_one_database()
> with a REINDEX_SYSTEM option in the enum. Julien, could you send a
> new version?
Yes, I had further refactoring in mind including this one (there are
also quite some parameters passed to the functions, passing a struct
instead could be worthwhile), but I thought this should be better done
after branching.
> > Right. Also, I was imagining folding the steps together while
> > building the commands so that there's just one switch() for that,
> > along the lines of
>
> Yes, that makes sense.
Indeed.
I attach the switch refactoring that applies on top of current HEAD,
and the reindex_system_catalogs() removal in a different patch in case
that's too much during feature freeze.
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 7d02a7f54f..64085e79e9 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -21,7 +21,8 @@ typedef enum ReindexType
REINDEX_DATABASE,
REINDEX_SCHEMA,
REINDEX_TABLE,
- REINDEX_INDEX
+ REINDEX_INDEX,
+ REINDEX_SYSTEM
} ReindexType;
static void reindex_one_database(const char *name, const char *dbname,
@@ -34,11 +35,6 @@ static void reindex_all_databases(const char *maintenance_db,
const char *username, enum trivalue prompt_password,
const char *progname, bool echo,
bool quiet, bool verbose, bool concurrently);
-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, bool verbose,
- bool concurrently);
static void help(const char *progname);
int
@@ -228,8 +224,8 @@ main(int argc, char *argv[])
dbname = get_user_name_or_exit(progname);
}
- reindex_system_catalogs(dbname, host, port, username, prompt_password,
- progname, echo, verbose, concurrently);
+ reindex_one_database(NULL, dbname, REINDEX_SYSTEM, host, port,
+ username, prompt_password, progname, echo, verbose, concurrently);
}
else
{
@@ -334,6 +330,10 @@ reindex_one_database(const char *name, const char *dbname, ReindexType type,
appendQualifiedRelation(&sql, name, conn, progname, echo);
appendPQExpBufferStr(&sql, ";");
break;
+ case REINDEX_SYSTEM:
+ appendPQExpBuffer(&sql, "REINDEX%s SYSTEM%s %s;", verbose_str,
+ concurrent_str, fmtId(PQdb(conn)));
+ break;
}
if (!executeMaintenanceCommand(conn, sql.data, echo))
@@ -356,6 +356,10 @@ reindex_one_database(const char *name, const char *dbname, ReindexType type,
pg_log_error("reindexing of database \"%s\" failed: %s",
PQdb(conn), PQerrorMessage(conn));
break;
+ case REINDEX_SYSTEM:
+ pg_log_error("reindexing of system catalogs on database \"%s\" failed: %s",
+ PQdb(conn), PQerrorMessage(conn));
+ break;
}
PQfinish(conn);
exit(1);
@@ -406,41 +410,6 @@ reindex_all_databases(const char *maintenance_db,
PQclear(result);
}
-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, bool verbose, bool concurrently)
-{
- PGconn *conn;
- PQExpBufferData sql;
-
- conn = connectDatabase(dbname, host, port, username, prompt_password,
- progname, echo, false, false);
-
- initPQExpBuffer(&sql);
-
- appendPQExpBuffer(&sql, "REINDEX");
-
- if (verbose)
- appendPQExpBuffer(&sql, " (VERBOSE)");
-
- appendPQExpBufferStr(&sql, " SYSTEM ");
- if (concurrently)
- appendPQExpBuffer(&sql, "CONCURRENTLY ");
- appendPQExpBufferStr(&sql, fmtId(PQdb(conn)));
- appendPQExpBufferChar(&sql, ';');
-
- if (!executeMaintenanceCommand(conn, sql.data, echo))
- {
- pg_log_error("reindexing of system catalogs failed: %s",
- PQerrorMessage(conn));
- PQfinish(conn);
- exit(1);
- }
- PQfinish(conn);
- termPQExpBuffer(&sql);
-}
-
static void
help(const char *progname)
{
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 897ad9a71a..7d02a7f54f 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -16,8 +16,16 @@
#include "fe_utils/string_utils.h"
+typedef enum ReindexType
+{
+ REINDEX_DATABASE,
+ REINDEX_SCHEMA,
+ REINDEX_TABLE,
+ REINDEX_INDEX
+} ReindexType;
+
static void reindex_one_database(const char *name, const char *dbname,
- const char *type, const char *host,
+ ReindexType type, const char *host,
const char *port, const char *username,
enum trivalue prompt_password, const char *progname,
bool echo, bool verbose, bool concurrently);
@@ -241,7 +249,7 @@ main(int argc, char *argv[])
for (cell = schemas.head; cell; cell = cell->next)
{
- reindex_one_database(cell->val, dbname, "SCHEMA", host, port,
+ reindex_one_database(cell->val, dbname, REINDEX_SCHEMA, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -252,7 +260,7 @@ main(int argc, char *argv[])
for (cell = indexes.head; cell; cell = cell->next)
{
- reindex_one_database(cell->val, dbname, "INDEX", host, port,
+ reindex_one_database(cell->val, dbname, REINDEX_INDEX, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -262,7 +270,7 @@ main(int argc, char *argv[])
for (cell = tables.head; cell; cell = cell->next)
{
- reindex_one_database(cell->val, dbname, "TABLE", host, port,
+ reindex_one_database(cell->val, dbname, REINDEX_TABLE, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -272,7 +280,7 @@ main(int argc, char *argv[])
* specified
*/
if (indexes.head == NULL && tables.head == NULL && schemas.head == NULL)
- reindex_one_database(NULL, dbname, "DATABASE", host, port,
+ reindex_one_database(NULL, dbname, REINDEX_DATABASE, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
@@ -280,12 +288,14 @@ main(int argc, char *argv[])
}
static void
-reindex_one_database(const char *name, const char *dbname, const char *type,
+reindex_one_database(const char *name, const char *dbname, ReindexType type,
const char *host, const char *port, const char *username,
enum trivalue prompt_password, const char *progname, bool echo,
bool verbose, bool concurrently)
{
PQExpBufferData sql;
+ const char *verbose_str = verbose ? " (VERBOSE)" : "";
+ const char *concurrent_str = concurrently ? " CONCURRENTLY" : "";
PGconn *conn;
@@ -302,38 +312,51 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
initPQExpBuffer(&sql);
- appendPQExpBufferStr(&sql, "REINDEX ");
-
- if (verbose)
- appendPQExpBufferStr(&sql, "(VERBOSE) ");
-
- appendPQExpBufferStr(&sql, type);
- appendPQExpBufferChar(&sql, ' ');
- if (concurrently)
- appendPQExpBufferStr(&sql, "CONCURRENTLY ");
- if (strcmp(type, "TABLE") == 0 ||
- strcmp(type, "INDEX") == 0)
- appendQualifiedRelation(&sql, name, conn, progname, echo);
- else if (strcmp(type, "SCHEMA") == 0)
- appendPQExpBufferStr(&sql, name);
- else if (strcmp(type, "DATABASE") == 0)
- appendPQExpBufferStr(&sql, fmtId(PQdb(conn)));
- appendPQExpBufferChar(&sql, ';');
+ switch (type)
+ {
+ case REINDEX_DATABASE:
+ appendPQExpBuffer(&sql, "REINDEX%s DATABASE%s %s;", verbose_str,
+ concurrent_str, fmtId(PQdb(conn)));
+ break;
+ case REINDEX_SCHEMA:
+ appendPQExpBuffer(&sql, "REINDEX%s SCHEMA%s %s;", verbose_str,
+ concurrent_str, name);
+ break;
+ case REINDEX_TABLE:
+ appendPQExpBuffer(&sql, "REINDEX%s TABLE%s ", verbose_str,
+ concurrent_str);
+ appendQualifiedRelation(&sql, name, conn, progname, echo);
+ appendPQExpBufferStr(&sql, ";");
+ break;
+ case REINDEX_INDEX:
+ appendPQExpBuffer(&sql, "REINDEX%s INDEX%s ", verbose_str,
+ concurrent_str);
+ appendQualifiedRelation(&sql, name, conn, progname, echo);
+ appendPQExpBufferStr(&sql, ";");
+ break;
+ }
if (!executeMaintenanceCommand(conn, sql.data, echo))
{
- if (strcmp(type, "TABLE") == 0)
- pg_log_error("reindexing of table \"%s\" in database \"%s\" failed: %s",
- name, PQdb(conn), PQerrorMessage(conn));
- else if (strcmp(type, "INDEX") == 0)
- pg_log_error("reindexing of index \"%s\" in database \"%s\" failed: %s",
- name, PQdb(conn), PQerrorMessage(conn));
- else if (strcmp(type, "SCHEMA") == 0)
- pg_log_error("reindexing of schema \"%s\" in database \"%s\" failed: %s",
- name, PQdb(conn), PQerrorMessage(conn));
- else
- pg_log_error("reindexing of database \"%s\" failed: %s",
- PQdb(conn), PQerrorMessage(conn));
+ switch (type)
+ {
+ case REINDEX_TABLE:
+ pg_log_error("reindexing of table \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case REINDEX_INDEX:
+ pg_log_error("reindexing of index \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case REINDEX_SCHEMA:
+ pg_log_error("reindexing of schema \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case REINDEX_DATABASE:
+ pg_log_error("reindexing of database \"%s\" failed: %s",
+ PQdb(conn), PQerrorMessage(conn));
+ break;
+ }
PQfinish(conn);
exit(1);
}
@@ -374,7 +397,7 @@ reindex_all_databases(const char *maintenance_db,
appendPQExpBuffer(&connstr, "dbname=");
appendConnStrVal(&connstr, dbname);
- reindex_one_database(NULL, connstr.data, "DATABASE", host,
+ reindex_one_database(NULL, connstr.data, REINDEX_DATABASE, host,
port, username, prompt_password,
progname, echo, verbose, concurrently);
}
