Обсуждение: 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.
Вложения
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.
Вложения
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 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.
Вложения
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