Thread: Re: [BUGS] BUG #10823: Better REINDEX syntax.
On 07/30/2014 07:46 PM, Tom Lane wrote: > Bruce Momjian <bruce@momjian.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
Attachment
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
Attachment
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.fearing@dalibo.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.fearing@dalibo.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.fearing@dalibo.com) wrote:
> On 09/08/2014 06:17 AM, Stephen Frost wrote:
> > * Vik Fearing (vik.fearing@dalibo.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Alvaro Herrera (alvherre@2ndquadrant.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