Re: [BUGS] BUG #10823: Better REINDEX syntax.

The following bug has been logged on the website:

Bug reference: 10823
Logged by: Daniel Migowski
Email address: dmigowski(at)ikoffice(dot)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!

On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski(at)ikoffice(dot)de wrote:
> The following bug has been logged on the website:
>
> Bug reference: 10823
> Logged by: Daniel Migowski
> Email address: dmigowski(at)ikoffice(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski(at)ikoffice(dot)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

On Wed, Jul 30, 2014 at 01:29:31PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski(at)ikoffice(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)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

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(at)momjian(dot)us> writes:
>>> On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski(at)ikoffice(dot)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

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(at)momjian(dot)us> writes:
> >>> On Tue, Jul 1, 2014 at 10:33:07AM +0000, dmigowski(at)ikoffice(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 07/30/2014 07:46 PM, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)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

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

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

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

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

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

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

* Vik Fearing (vik(dot)fearing(at)dalibo(dot)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(dot)fearing(at)dalibo(dot)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

* Vik Fearing (vik(dot)fearing(at)dalibo(dot)com) wrote:
> On 09/08/2014 06:17 AM, Stephen Frost wrote:
> > * Vik Fearing (vik(dot)fearing(at)dalibo(dot)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

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> Stephen Frost wrote:
> > * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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