Horton

Thread

Commits

  1. Fix error reporting in reindexdb

  1. Bug in reindexdb's error reporting

    Julien Rouhaud <rjuju123@gmail.com> — 2019-05-10T09:02:52Z

    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.

  2. Re: Bug in reindexdb's error reporting

    Michael Paquier <michael@paquier.xyz> — 2019-05-10T10:24:47Z

    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

  3. Re: Bug in reindexdb's error reporting

    Daniel Gustafsson <daniel@yesql.se> — 2019-05-10T10:38:06Z

    > 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

  4. Re: Bug in reindexdb's error reporting

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-10T14:18:09Z

    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

  5. Re: Bug in reindexdb's error reporting

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-10T14:43:50Z

    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

  6. Re: Bug in reindexdb's error reporting

    Julien Rouhaud <rjuju123@gmail.com> — 2019-05-10T15:18:11Z

    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.

  7. Re: Bug in reindexdb's error reporting

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-10T15:33:23Z

    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

  8. Re: Bug in reindexdb's error reporting

    Julien Rouhaud <rjuju123@gmail.com> — 2019-05-10T15:58:03Z

    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.

  9. Re: Bug in reindexdb's error reporting

    Michael Paquier <michael@paquier.xyz> — 2019-05-11T00:42:00Z

    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

  10. Re: Bug in reindexdb's error reporting

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-11T01:25:58Z

    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

  11. Re: Bug in reindexdb's error reporting

    Michael Paquier <michael@paquier.xyz> — 2019-05-11T04:04:50Z

    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

  12. Re: Bug in reindexdb's error reporting

    Julien Rouhaud <rjuju123@gmail.com> — 2019-05-11T08:28:43Z

    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.

  13. Re: Bug in reindexdb's error reporting

    Michael Paquier <michael@paquier.xyz> — 2019-05-11T12:09:02Z

    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

  14. Re: Bug in reindexdb's error reporting

    Julien Rouhaud <rjuju123@gmail.com> — 2019-05-11T18:03:20Z

    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.

  15. Re: Bug in reindexdb's error reporting

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-11T22:28:00Z

    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