Bug in reindexdb's error reporting
Hi,
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.
Trivial fix attached.
Attachments:
fix_error_reporting_reindexdb-v1.difftext/x-patch; charset=US-ASCII; name=fix_error_reporting_reindexdb-v1.diffDownload
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index d6f3efd313..897ad9a71a 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -325,10 +325,10 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
if (strcmp(type, "TABLE") == 0)
pg_log_error("reindexing of table \"%s\" in database \"%s\" failed: %s",
name, PQdb(conn), PQerrorMessage(conn));
- if (strcmp(type, "INDEX") == 0)
+ else if (strcmp(type, "INDEX") == 0)
pg_log_error("reindexing of index \"%s\" in database \"%s\" failed: %s",
name, PQdb(conn), PQerrorMessage(conn));
- if (strcmp(type, "SCHEMA") == 0)
+ else if (strcmp(type, "SCHEMA") == 0)
pg_log_error("reindexing of schema \"%s\" in database \"%s\" failed: %s",
name, PQdb(conn), PQerrorMessage(conn));
else
On Fri, May 10, 2019 at 11:02:52AM +0200, Julien Rouhaud wrote:
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.Trivial fix attached.
Oops. That's true, nice catch. This is older than 9.4, so it needs
to go all the way down. Let's fix this. Do others have any comments?
--
Michael
On 10 May 2019, at 12:24, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 10, 2019 at 11:02:52AM +0200, Julien Rouhaud wrote:
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.Trivial fix attached.
Oops. That's true, nice catch. This is older than 9.4, so it needs
to go all the way down. Let's fix this. Do others have any comments?
Nice catch indeed, LGTM.
cheers ./daniel
On 2019-May-10, Julien Rouhaud wrote:
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.
Kudos, good find -- that's a 14 years old bug, introduced in this commit:
Author: Bruce Momjian <bruce@momjian.us>
Branch: master Release: REL8_1_BR [85e9a5a01] 2005-07-29 15:13:11 +0000
Move reindexdb from /contrib to /bin.
Euler Taveira de Oliveira
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-May-10, Julien Rouhaud wrote:
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.
Kudos, good find -- that's a 14 years old bug, introduced in this commit:
Yeah :-(.
Patch is good as far as it goes, but I wonder if it'd be smarter to
convert the function's "type" argument from a string to an enum,
and then replace the if/else chains with switches?
regards, tom lane
On Fri, May 10, 2019 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2019-May-10, Julien Rouhaud wrote:
I just noticed that reindexdb could report an extraneous message
saying an error happened while reindexing a database if it failed
reindexing a table or an index.Kudos, good find -- that's a 14 years old bug, introduced in this commit:
Yeah :-(.
Patch is good as far as it goes, but I wonder if it'd be smarter to
convert the function's "type" argument from a string to an enum,
and then replace the if/else chains with switches?
I've also thought about it. I think the reason why type argument was
kept as a string is that reindex_one_database is doing:
appendPQExpBufferStr(&sql, type);
to avoid an extra switch to append the textual reindex type. I don't
have a strong opinion on whether to change that on master or not.
On 2019-May-10, Julien Rouhaud wrote:
On Fri, May 10, 2019 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Patch is good as far as it goes, but I wonder if it'd be smarter to
convert the function's "type" argument from a string to an enum,
and then replace the if/else chains with switches?I've also thought about it. I think the reason why type argument was
kept as a string is that reindex_one_database is doing:appendPQExpBufferStr(&sql, type);
to avoid an extra switch to append the textual reindex type. I don't
have a strong opinion on whether to change that on master or not.
I did have the same thought. It seem clear now that we should do it :-)
ISTM that the way to fix that problem is to use the proposed enum
everywhere and turn it into a string when generating the SQL command,
not before.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 10, 2019 at 5:33 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-May-10, Julien Rouhaud wrote:
On Fri, May 10, 2019 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Patch is good as far as it goes, but I wonder if it'd be smarter to
convert the function's "type" argument from a string to an enum,
and then replace the if/else chains with switches?I've also thought about it. I think the reason why type argument was
kept as a string is that reindex_one_database is doing:appendPQExpBufferStr(&sql, type);
to avoid an extra switch to append the textual reindex type. I don't
have a strong opinion on whether to change that on master or not.I did have the same thought. It seem clear now that we should do it :-)
ISTM that the way to fix that problem is to use the proposed enum
everywhere and turn it into a string when generating the SQL command,
not before.
ok :) Patch v2 attached.
Attachments:
fix_error_reporting_reindexdb-v2.difftext/x-patch; charset=US-ASCII; name=fix_error_reporting_reindexdb-v2.diffDownload
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index d6f3efd313..93aecfd733 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -16,8 +16,15 @@
#include "fe_utils/string_utils.h"
+typedef enum ReindexType {
+ DATABASE,
+ SCHEMA,
+ TABLE,
+ 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 +248,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, SCHEMA, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -252,7 +259,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, INDEX, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -262,7 +269,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, TABLE, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
}
@@ -272,7 +279,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, DATABASE, host, port,
username, prompt_password, progname, echo, verbose, concurrently);
}
@@ -280,7 +287,7 @@ 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)
@@ -307,33 +314,73 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
if (verbose)
appendPQExpBufferStr(&sql, "(VERBOSE) ");
- appendPQExpBufferStr(&sql, type);
+ switch(type)
+ {
+ case DATABASE:
+ appendPQExpBufferStr(&sql, "DATABASE");
+ break;
+ case SCHEMA:
+ appendPQExpBufferStr(&sql, "SCHEMA");
+ break;
+ case TABLE:
+ appendPQExpBufferStr(&sql, "TABLE");
+ break;
+ case INDEX:
+ appendPQExpBufferStr(&sql, "INDEX");
+ break;
+ default:
+ pg_log_error("Unrecognized reindex type %d", type);
+ exit(1);
+ break;
+ }
+
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)));
+ switch(type)
+ {
+ case TABLE:
+ /* Fall through. */
+ case INDEX:
+ appendQualifiedRelation(&sql, name, conn, progname, echo);
+ break;
+ case SCHEMA:
+ appendPQExpBufferStr(&sql, name);
+ break;
+ case DATABASE:
+ appendPQExpBufferStr(&sql, fmtId(PQdb(conn)));
+ break;
+ default:
+ pg_log_error("Unrecognized reindex type %d", type);
+ exit(1);
+ break;
+ }
appendPQExpBufferChar(&sql, ';');
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));
- if (strcmp(type, "INDEX") == 0)
- pg_log_error("reindexing of index \"%s\" in database \"%s\" failed: %s",
- name, PQdb(conn), PQerrorMessage(conn));
- 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 TABLE:
+ pg_log_error("reindexing of table \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case INDEX:
+ pg_log_error("reindexing of index \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case SCHEMA:
+ pg_log_error("reindexing of schema \"%s\" in database \"%s\" failed: %s",
+ name, PQdb(conn), PQerrorMessage(conn));
+ break;
+ case DATABASE:
+ pg_log_error("reindexing of database \"%s\" failed: %s",
+ PQdb(conn), PQerrorMessage(conn));
+ break;
+ default:
+ pg_log_error("Unrecognized reindex type %d", type);
+ break;
+ }
PQfinish(conn);
exit(1);
}
@@ -374,7 +421,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, DATABASE, host,
port, username, prompt_password,
progname, echo, verbose, concurrently);
}
On Fri, May 10, 2019 at 05:58:03PM +0200, Julien Rouhaud wrote:
On Fri, May 10, 2019 at 5:33 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I did have the same thought. It seem clear now that we should do it :-)
ISTM that the way to fix that problem is to use the proposed enum
everywhere and turn it into a string when generating the SQL command,
not before.ok :) Patch v2 attached.
The refactoring bits are fine for HEAD. For back-branches I would
suggest using the simplest patch of upthread.
+typedef enum ReindexType { + DATABASE, + SCHEMA, + TABLE, + INDEX +} ReindexType;
That's perhaps too much generic when it comes to grep in the source
code, why not appending REINDEX_ to each element?
+ switch(type) + { + case DATABASE: + appendPQExpBufferStr(&sql, "DATABASE"); + break; + case SCHEMA: + appendPQExpBufferStr(&sql, "SCHEMA"); + break; + case TABLE: + appendPQExpBufferStr(&sql, "TABLE"); + break; + case INDEX: + appendPQExpBufferStr(&sql, "INDEX"); + break; + default: + pg_log_error("Unrecognized reindex type %d", type); + exit(1); + break; + }
We could actually remove this default part, so as we get compiler
warning when introducing a new element.
--
Michael
Michael Paquier <michael@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.
That's perhaps too much generic when it comes to grep in the source
code, why not appending REINDEX_ to each element?
+1
We could actually remove this default part, so as we get compiler
warning when introducing a new element.
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
const char *verbose_option = verbose ? " (VERBOSE)" : "";
const char *concurrent_option = concurrently ? " CONCURRENTLY" : "";
switch (type)
{
case REINDEX_DATABASE:
appendPQExpBufferStr(&sql, "REINDEX%s DATABASE%s %s",
verbose_option, concurrent_option,
fmtId(PQdb(conn)));
break;
case REINDEX_TABLE:
appendPQExpBufferStr(&sql, "REINDEX%s TABLE%s ",
verbose_option, concurrent_option);
appendQualifiedRelation(&sql, name, conn, progname, echo);
break;
....
It seemed to me that this would be more understandable and flexible
than the way it's being done now, though of course others might see
that differently. I'm not dead set on that, just suggesting it.
regards, tom lane
On Fri, May 10, 2019 at 09:25:58PM -0400, Tom Lane wrote:
Michael Paquier <michael@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. 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?
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.
--
Michael
On Sat, May 11, 2019 at 6:04 AM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, May 10, 2019 at 09:25:58PM -0400, Tom Lane wrote:
Michael Paquier <michael@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 ofYes, 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.
Attachments:
0002-merge_reindex_system_catalogs-v1.difftext/x-patch; charset=US-ASCII; name=0002-merge_reindex_system_catalogs-v1.diffDownload
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)
{
0001-use_enum-v1.difftext/x-patch; charset=US-ASCII; name=0001-use_enum-v1.diffDownload
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);
}
On Sat, May 11, 2019 at 10:28:43AM +0200, Julien Rouhaud wrote:
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.
Both Look fine to me at quick glance, but I have not tested them. I
am not sure about refactoring all the options into a structure,
perhaps it depends on what kind of patch it gives. Regarding a merge
into the tree, I think that this refactoring should wait until
REL_12_STABLE has been created. It is no time to take risks in
destabilizing the code.
Also, as this thread's problem has been solved, perhaps it would be
better to spawn a new thread, and to add a new entry in the CF app for
the refactoring set so as it attracts the correct audience? The
current thread topic is unfortunately misleading based on the latest
messages exchanged.
--
Michael
On Sat, May 11, 2019 at 2:09 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, May 11, 2019 at 10:28:43AM +0200, Julien Rouhaud wrote:
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.Both Look fine to me at quick glance, but I have not tested them. I
am not sure about refactoring all the options into a structure,
perhaps it depends on what kind of patch it gives. Regarding a merge
into the tree, I think that this refactoring should wait until
REL_12_STABLE has been created. It is no time to take risks in
destabilizing the code.
I've run the TAP tests and it's running fine, but this should
definitely wait for branching.
Also, as this thread's problem has been solved, perhaps it would be
better to spawn a new thread, and to add a new entry in the CF app for
the refactoring set so as it attracts the correct audience? The
current thread topic is unfortunately misleading based on the latest
messages exchanged.
Unless someone argue it should be applied in v12, I'll do that soon.
On 2019-May-11, Julien Rouhaud wrote:
On Sat, May 11, 2019 at 2:09 PM Michael Paquier <michael@paquier.xyz> wrote:
Also, as this thread's problem has been solved, perhaps it would be
better to spawn a new thread, and to add a new entry in the CF app for
the refactoring set so as it attracts the correct audience? The
current thread topic is unfortunately misleading based on the latest
messages exchanged.Unless someone argue it should be applied in v12, I'll do that soon.
Certainly not.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services