On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.m...@gmail.com>
wrote:
> >
> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
> > <fabriziome...@gmail.com> wrote:
> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <
sawada.m...@gmail.com>
> > > wrote:
> > >>
> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmh...@gmail.com>
> > >> wrote:
> > >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfr...@snowman.net>
> > >> > wrote:
> > >> >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > >> >>> Sawada Masahiko wrote:
> > >> >>> > Attached WIP patch adds new syntax REINEX SCHEMA which does
> > >> >>> > reindexing
> > >> >>> > all table of specified schema.
> > >> >>> > There are syntax dose reindexing specified index, per table
and per
> > >> >>> > database,
> > >> >>> > but we can not do reindexing per schema for now.
> > >> >>>
> > >> >>> It seems doubtful that there really is much use for this
feature, but
> > >> >>> if
> > >> >>> there is, I think a better syntax precedent is the new ALTER
TABLE ALL
> > >> >>> IN TABLESPACE thingy, rather than your proposed REINDEX SCHEMA.
> > >> >>> Something like REINDEX TABLE ALL IN SCHEMA perhaps.
> > >> >>
> > >> >> Yeah, I tend to agree that we should be looking at the 'ALL IN
> > >> >> TABLESPACE' and 'ALL IN SCHEMA' type of commands to keep things
> > >> >> consistent. This might be an alternative for the vacuum /
analyze /
> > >> >> reindex database commands also..
> > >> >
> > >> > Urgh. I don't have a problem with that syntax in general, but it
> > >> > clashes pretty awfully with what we're already doing for REINDEX
> > >> > otherwise.
> > >> >
> > >>
> > >> Attached patches are latest version patch.
> > >
> > > Ok.
> > >
> > >
> > >> I changed syntax to REINDEX ALL IN SCHEMA, but I felt a sense of
> > >> discomfort a little
> > >> as Robert mentioned.
> > >>
> > >
> > > I understood, but the real problem will in a near future when the
features
> > > will be pushed... :-)
> > >
> > > They are separated features, but maybe we can join this features to a
one
> > > future commit... it's just an idea.
> > >
> > >
> > >> Anyway, you can apply these patches in numerical order,
> > >> can use REINDEX ALL IN SCHEMA feature and "-S/--schema" option in
> > >> reindexdb.
> > >>
> > >> 000_reindex_all_in_schema_v2.patch : It contains REINDEX ALL IN
SCHEMA
> > >> feature
> > >
> > > 1) Compile without warnings
> > >
> > >
> > > 2) IMHO you can add more test cases to better code coverage:
> > >
> > > * reindex a schema that doesn't exists
> > > * try to run "reindex all in schema" inside a transaction block
> > >
> > >
> > > 3) Isn't enough just?
> > >
> > > bool do_database = (kind == OBJECT_DATABASE);
> > >
> > > ... instead of...
> > >
> > > + bool do_database = (kind == OBJECT_DATABASE) ? true : false;
> > >
> > >
> > > 4) IMHO you can add other Assert to check valid relkinds, like:
> > >
> > > Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
> > >
> > >
> > > 5) I think is more legible:
> > >
> > > /* Get OID of object for result */
> > > if (do_database)
> > > objectOid = MyDatabaseId
> > > else
> > > objectOid = get_namespace_oid(objectName, false);
> > >
> > > ... insead of ...
> > >
> > > + /* Get OID of object for result */
> > > + objectOid = (do_database) ? MyDatabaseId :
get_namespace_oid(objectName,
> > > false);
> > >
> > >
> > >
> > >> 001_Add_schema_option_to_reindexdb_v1.patch : It contains reindexdb
> > >> "-S/--schema" supporting
> > >>
> > >
> > > The code itself is good for me, but IMHO you can add test cases to
> > > src/bin/scripts/t/090_reindexdb.pl
> > >
> >
> > Thank you for reviewing.
>
> You're welcome!
>
>
> > I agree 2) - 5).
>
> :-)
>
>
> > Attached patch is latest version patch I modified above.
>
> All is fine to me now... all work as expected and no compiler warnings.
>
> There are just a little fix to do in src/bin/scripts/t/090_reindexdb.pl
>
> -use Test::More tests => 7;
> +use Test::More tests => 8;
>
> Because you added a new testcase to suittest, so you need to increase the
test count at beginning of the file.
>
Patch attached. Now the regress run without errors.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
index 486f5c9..b5b449c 100644
--- a/doc/src/sgml/ref/reindexdb.sgml
+++ b/doc/src/sgml/ref/reindexdb.sgml
@@ -27,6 +27,16 @@ PostgreSQL documentation
<arg choice="plain" rep="repeat">
<arg choice="opt">
<group choice="plain">
+ <arg choice="plain"><option>--schema</option></arg>
+ <arg choice="plain"><option>-S</option></arg>
+ </group>
+ <replaceable>table</replaceable>
+ </arg>
+ </arg>
+
+ <arg choice="plain" rep="repeat">
+ <arg choice="opt">
+ <group choice="plain">
<arg choice="plain"><option>--table</option></arg>
<arg choice="plain"><option>-t</option></arg>
</group>
@@ -162,6 +172,18 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-S <replaceable class="parameter">schema</replaceable></></term>
+ <term><option>--schema=<replaceable class="parameter">schema</replaceable></></term>
+ <listitem>
+ <para>
+ Reindex <replaceable class="parameter">schema</replaceable> only.
+ Multiple schemas can be reindexed by writing multiple
+ <option>-S</> switches.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-t <replaceable class="parameter">table</replaceable></></term>
<term><option>--table=<replaceable class="parameter">table</replaceable></></term>
<listitem>
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 561bbce..dbea347 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -41,6 +41,7 @@ main(int argc, char *argv[])
{"password", no_argument, NULL, 'W'},
{"echo", no_argument, NULL, 'e'},
{"quiet", no_argument, NULL, 'q'},
+ {"schema", required_argument, NULL, 'S'},
{"dbname", required_argument, NULL, 'd'},
{"all", no_argument, NULL, 'a'},
{"system", no_argument, NULL, 's'},
@@ -66,6 +67,7 @@ main(int argc, char *argv[])
bool quiet = false;
SimpleStringList indexes = {NULL, NULL};
SimpleStringList tables = {NULL, NULL};
+ SimpleStringList schemas = {NULL, NULL};
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pgscripts"));
@@ -73,7 +75,7 @@ main(int argc, char *argv[])
handle_help_version_opts(argc, argv, "reindexdb", help);
/* process command-line options */
- while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:ast:i:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:", long_options, &optindex)) != -1)
{
switch (c)
{
@@ -98,6 +100,9 @@ main(int argc, char *argv[])
case 'q':
quiet = true;
break;
+ case 'S':
+ simple_string_list_append(&schemas, optarg);
+ break;
case 'd':
dbname = pg_strdup(optarg);
break;
@@ -154,6 +159,11 @@ main(int argc, char *argv[])
fprintf(stderr, _("%s: cannot reindex all databases and system catalogs at the same time\n"), progname);
exit(1);
}
+ if (schemas.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific schema(s) in all databases\n"), progname);
+ exit(1);
+ }
if (tables.head != NULL)
{
fprintf(stderr, _("%s: cannot reindex specific table(s) in all databases\n"), progname);
@@ -170,6 +180,11 @@ main(int argc, char *argv[])
}
else if (syscatalog)
{
+ if (schemas.head != NULL)
+ {
+ fprintf(stderr, _("%s: cannot reindex specific schema(s) and system catalogs at the same time\n"), progname);
+ exit(1);
+ }
if (tables.head != NULL)
{
fprintf(stderr, _("%s: cannot reindex specific table(s) and system catalogs at the same time\n"), progname);
@@ -206,6 +221,17 @@ main(int argc, char *argv[])
dbname = get_user_name_or_exit(progname);
}
+ if (schemas.head != NULL)
+ {
+ SimpleStringListCell *cell;
+
+ for (cell = schemas.head; cell; cell = cell->next)
+ {
+ reindex_one_database(cell->val, dbname, "SCHEMA", host, port,
+ username, prompt_password, progname, echo);
+ }
+ }
+
if (indexes.head != NULL)
{
SimpleStringListCell *cell;
@@ -226,8 +252,8 @@ main(int argc, char *argv[])
username, prompt_password, progname, echo);
}
}
- /* reindex database only if neither index nor table is specified */
- if (indexes.head == NULL && tables.head == NULL)
+ /* reindex database only if neither index nor table nor schema is specified */
+ if (indexes.head == NULL && tables.head == NULL && schemas.head == NULL)
reindex_one_database(dbname, dbname, "DATABASE", host, port,
username, prompt_password, progname, echo);
}
@@ -251,6 +277,8 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
appendPQExpBuffer(&sql, " TABLE %s", name);
else if (strcmp(type, "INDEX") == 0)
appendPQExpBuffer(&sql, " INDEX %s", name);
+ else if (strcmp(type, "SCHEMA") == 0)
+ appendPQExpBuffer(&sql, " ALL IN SCHEMA %s", name);
else if (strcmp(type, "DATABASE") == 0)
appendPQExpBuffer(&sql, " DATABASE %s", fmtId(name));
appendPQExpBufferStr(&sql, ";");
@@ -266,6 +294,9 @@ reindex_one_database(const char *name, const char *dbname, const char *type,
if (strcmp(type, "INDEX") == 0)
fprintf(stderr, _("%s: reindexing of index \"%s\" in database \"%s\" failed: %s"),
progname, name, dbname, PQerrorMessage(conn));
+ if (strcmp(type, "SCHEMA") == 0)
+ fprintf(stderr, _("%s: reindexing of schema \"%s\" in database \"%s\" failed: %s"),
+ progname, name, dbname, PQerrorMessage(conn));
else
fprintf(stderr, _("%s: reindexing of database \"%s\" failed: %s"),
progname, dbname, PQerrorMessage(conn));
@@ -348,6 +379,7 @@ help(const char *progname)
printf(_(" -i, --index=INDEX recreate specific index(es) only\n"));
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" -s, --system reindex system catalogs\n"));
+ printf(_(" -S, --schema=SCHEMA recreate specific schema(s) only\n"));
printf(_(" -t, --table=TABLE reindex specific table(s) only\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index 24b927c..934fe7d 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -1,7 +1,7 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 7;
+use Test::More tests => 8;
program_help_ok('reindexdb');
program_version_ok('reindexdb');
@@ -29,6 +29,10 @@ issues_sql_like(
'reindex specific index');
issues_sql_like(
+ [ 'reindexdb', 'postgres', '-S', 'pg_catalog' ],
+ qr/statement: REINDEX ALL IN SCHEMA pg_catalog;/,
+ 'reindex specific schema');
+issues_sql_like(
[ 'reindexdb', 'postgres', '-s' ],
qr/statement: REINDEX SYSTEM postgres;/,
'reindex system tables');
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
