Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
* Alvaro Herrera (alvhe...@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 signature.asc Description: Digital signature
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
Stephen Frost wrote: * Alvaro Herrera (alvhe...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: * Alvaro Herrera (alvhe...@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 signature.asc Description: Digital signature
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
On 09/08/2014 06:17 AM, Stephen Frost wrote: * Vik Fearing (vik.fear...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
* Vik Fearing (vik.fear...@dalibo.com) wrote: On 09/08/2014 06:17 AM, Stephen Frost wrote: * Vik Fearing (vik.fear...@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 signature.asc Description: Digital signature
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
* Vik Fearing (vik.fear...@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 signature.asc Description: Digital signature
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index cabae19..d05e1ac 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX USER TABLES
/synopsis
/refsynopsisdiv
@@ -126,14 +127,26 @@ REINDEX { INDEX | TABLE | DATABASE | SYSTEM } replaceable class=PARAMETERnam
/varlistentry
varlistentry
+termliteralUSER TABLES/literal/term
+listitem
+ para
+ Recreate all indexes on user tables within the current database.
+ Indexes on system catalogs are not processed.
+ This form of commandREINDEX/command cannot be executed inside a
+ transaction block.
+ /para
+/listitem
+ /varlistentry
+
+ varlistentry
termreplaceable class=PARAMETERname/replaceable/term
listitem
para
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
- Presently, commandREINDEX DATABASE/ and commandREINDEX SYSTEM/
- can only reindex the current database, so their parameter must match
- the current database's name.
+ Presently, commandREINDEX DATABASE/, commandREINDEX SYSTEM/,
+ and commandREINDEX USER TABLES/ can only reindex the current
+ database, so their parameter must match the current database's name.
/para
/listitem
/varlistentry
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 486f5c9..f69d84b 100644
--- a/doc/src/sgml/ref/reindexdb.sgml
+++ b/doc/src/sgml/ref/reindexdb.sgml
@@ -65,6 +65,15 @@ PostgreSQL documentation
/group
arg choice=optreplaceabledbname/replaceable/arg
/cmdsynopsis
+
+ cmdsynopsis
+ commandreindexdb/command
+ arg rep=repeatreplaceableconnection-option/replaceable/arg
+ group choice=plain
+arg choice=plainoption--user-tables/option/arg
+arg choice=plainoption-u/option/arg
+ /group
+ /cmdsynopsis
/refsynopsisdiv
@@ -173,6 +182,16 @@ PostgreSQL documentation
/listitem
/varlistentry
+ varlistentry
+ termoption-u//term
+ termoption--user-tables//term
+ listitem
+ para
+Reindex database's user tables.
+ /para
+ /listitem
+ /varlistentry
+
varlistentry
termoption-V//term
termoption--version//term
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fdfa6ca..23e13f0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1772,6 +1772,9 @@ ReindexTable(RangeVar *relation)
* To reduce the probability of deadlocks, each table is reindexed in a
* separate transaction, so we can release the lock on it right away.
* That means this must not be called within a user transaction block!
+ *
+ * databaseName can be NULL when do_user is set and do_system isn't; this
+ * is the REINDEX USER TABLES case.
*/
Oid
ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
@@ -1784,9 +1787,10 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
List *relids = NIL;
ListCell *l;
- AssertArg(databaseName);
+ AssertArg(databaseName || (do_user !do_system));
- if (strcmp(databaseName, get_database_name(MyDatabaseId)) != 0)
+ if (databaseName
+
Re: [HACKERS] [BUGS] BUG #10823: Better REINDEX syntax.
On 07/30/2014 07:46 PM, Tom Lane wrote:
Bruce Momjian br...@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
*** a/doc/src/sgml/ref/reindex.sgml
--- b/doc/src/sgml/ref/reindex.sgml
***
*** 21,27 PostgreSQL documentation
refsynopsisdiv
synopsis
! REINDEX { INDEX | TABLE | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
/synopsis
/refsynopsisdiv
--- 21,27
refsynopsisdiv
synopsis
! REINDEX { INDEX | TABLE | DATABASE | SYSTEM | USER TABLES } replaceable class=PARAMETERname/replaceable [ FORCE ]
/synopsis
/refsynopsisdiv
***
*** 126,139 REINDEX { INDEX | TABLE | DATABASE | SYSTEM } replaceable class=PARAMETERnam
/varlistentry
varlistentry
termreplaceable class=PARAMETERname/replaceable/term
listitem
para
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
! Presently, commandREINDEX DATABASE/ and commandREINDEX SYSTEM/
! can only reindex the current database, so their parameter must match
! the current database's name.
/para
/listitem
/varlistentry
--- 126,151
/varlistentry
varlistentry
+ termliteralUSER TABLES/literal/term
+ listitem
+ para
+ Recreate all indexes on user tables within the current database.
+ Indexes on system catalogs are not processed.
+ This form of commandREINDEX/command cannot be executed inside a
+ transaction block.
+ /para
+ /listitem
+/varlistentry
+
+varlistentry
termreplaceable class=PARAMETERname/replaceable/term
listitem
para
The name of the specific index, table, or database to be
reindexed. Index and table names can be schema-qualified.
! Presently, commandREINDEX DATABASE/, commandREINDEX SYSTEM/,
! and commandREINDEX USER TABLES/ can only reindex the current
! database, so their parameter must match the current database's name.
/para
/listitem
/varlistentry
*** a/doc/src/sgml/ref/reindexdb.sgml
--- b/doc/src/sgml/ref/reindexdb.sgml
***
*** 65,70 PostgreSQL documentation
--- 65,80
/group
arg choice=optreplaceabledbname/replaceable/arg
/cmdsynopsis
+
+ cmdsynopsis
+commandreindexdb/command
+arg rep=repeatreplaceableconnection-option/replaceable/arg
+group choice=plain
+ arg choice=plainoption--usertables/option/arg
+ arg choice=plainoption-u/option/arg
+/group
+arg choice=optreplaceabledbname/replaceable/arg
+ /cmdsynopsis
/refsynopsisdiv
***
*** 173,178 PostgreSQL documentation
--- 183,198
/listitem
/varlistentry
+ varlistentry
+ termoption-u//term
+ termoption--usertables//term
+ listitem
+para
+ Reindex database's user tables.
+/para
+ /listitem
+ /varlistentry
+
varlistentry
termoption-V//term
termoption--version//term
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 6984,6989 ReindexStmt:
--- 6984,6999
n-do_user = true;
$$ = (Node *)n;
}
+ | REINDEX USER TABLES name opt_force
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n-kind = OBJECT_DATABASE;
+ n-name = $4;
+ n-relation = NULL;
+ n-do_system = false;
+ n-do_user = true;
+ $$ = (Node *)n;
+ }
;
reindex_type:
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 3145,3151 psql_completion(const char *text, int start, int end)
else if (pg_strcasecmp(prev_wd, REINDEX) == 0)
{
static const char
