Thread: Proposal : REINDEX SCHEMA
Hi all, 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. So we must use reindexdb command if we want to do. This new syntax supports it as SQL command. This use similar logic as REINDEX DATABASE, but we can use it in transaction block. Here is some example, -- Table information [postgres][5432](1)=# \d n1.hoge Table "n1.hoge" Column | Type | Modifiers --------+---------+----------- col | integer | not null Indexes: "hoge_pkey" PRIMARY KEY, btree (col) [postgres][5432](1)=# \d n2.hoge Table "n2.hoge" Column | Type | Modifiers --------+---------+----------- col | integer | [postgres][5432](1)=# \d n3.hoge Did not find any relation named "n3.hoge". -- Do reindexing [postgres][5432](1)=# reindex schema n1; NOTICE: table "n1.hoge" was reindexed REINDEX [postgres][5432](1)=# reindex schema n2; REINDEX [postgres][5432](1)=# reindex schema n3; NOTICE: schema"n3" does not hava any table REINDEX Please review and comment. Regards, ------- Sawada Masahiko
Attachment
Sawada Masahiko wrote: > Hi all, > > 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. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
* Alvaro Herrera (alvherre@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.. Thanks, Stephen
<div dir="ltr"><div class="gmail_extra">On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <<a href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>>wrote:<br />><br />> * Alvaro Herrera (<a href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>)wrote:<br />> > Sawada Masahiko wrote:<br />>> > Attached WIP patch adds new syntax REINEX SCHEMA which does reindexing<br />> > > all table of specifiedschema.<br />> > > There are syntax dose reindexing specified index, per table and per database,<br />>> > but we can not do reindexing per schema for now.<br />> ><br />> > It seems doubtful that therereally is much use for this feature, but if<br />> > there is, I think a better syntax precedent is the new ALTERTABLE ALL<br />> > IN TABLESPACE thingy, rather than your proposed REINDEX SCHEMA.<br />> > Something likeREINDEX TABLE ALL IN SCHEMA perhaps.<br />><br />> Yeah, I tend to agree that we should be looking at the 'ALLIN<br />> TABLESPACE' and 'ALL IN SCHEMA' type of commands to keep things<br />> consistent. This might be analternative for the vacuum / analyze /<br />> reindex database commands also..<br />><br /><br /></div><div class="gmail_extra">Somereview:<br /><br /></div><div class="gmail_extra">1) +1 to "REINDEX ALL IN SCHEMA name"<br /><br/></div><div class="gmail_extra"><br />2) IMHO the logic should be exactly the same as REINDEX DATABASE, including thetransaction control. Imagine a schema with a lot of tables, you can lead to a deadlock using just one transaction block.<br/></div><div class="gmail_extra"><br /><br />3) The patch was applied to master and compile without warnings<br/><br /></div><div class="gmail_extra"><br />4) Typo (... does not have any table)</div><div class="gmail_extra"><br/>+ if (!reindex_schema(heapOid))<br />+ ereport(NOTICE,<br />+ (errmsg("schema\"%s\"does not hava any table",<br />+ schema->relname)));<br /><br /><br /></div><divclass="gmail_extra">5) Missing of regression tests, please add it to src/test/regress/sql/create_index.sql<br/></div><div class="gmail_extra"><br /></div><div class="gmail_extra">6) You needto add psql complete tabs<br /><br /></div><div class="gmail_extra">7) I think we can add "-S / --schema" option do reindexdbin this patch too. What do you think?<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><br />--<br />Fabrízio de Royes Mello<br />Consultoria/CoachingPostgreSQL<br />>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>
On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <sfrost@snowman.net> wrote: >> >> * Alvaro Herrera (alvherre@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.. >> > > Some review: > > 1) +1 to "REINDEX ALL IN SCHEMA name" > Thank you for comment and reviewing! I agree with this. I will change syntax to above like that. > > 2) IMHO the logic should be exactly the same as REINDEX DATABASE, including > the transaction control. Imagine a schema with a lot of tables, you can lead > to a deadlock using just one transaction block. > Yep, it should be same as REINDEX DATABASE. IMO, we can put them into one function if they use exactly same logic. Thought? > > 3) The patch was applied to master and compile without warnings > > > 4) Typo (... does not have any table) > > + if (!reindex_schema(heapOid)) > + ereport(NOTICE, > + (errmsg("schema\"%s\" does not hava any table", > + schema->relname))); > Thanks! I will correct typo. > > 5) Missing of regression tests, please add it to > src/test/regress/sql/create_index.sql > > 6) You need to add psql complete tabs > Next version patch, that I will submit, will have 5), 6) things you pointed. > 7) I think we can add "-S / --schema" option do reindexdb in this patch too. > What do you think? > +1 to add "-S / --schema" option I was misunderstanding about that. I will make the patch which adds this option as separate patch. -- Regards, ------- Sawada Masahiko
On Mon, Oct 13, 2014 at 5:39 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >>
> >> * Alvaro Herrera (alvherre@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..
> >>
> >
> > Some review:
> >
> > 1) +1 to "REINDEX ALL IN SCHEMA name"
> >
>
> Thank you for comment and reviewing!
>
> I agree with this.
> I will change syntax to above like that.
>
> >
> > 2) IMHO the logic should be exactly the same as REINDEX DATABASE, including
> > the transaction control. Imagine a schema with a lot of tables, you can lead
> > to a deadlock using just one transaction block.
> >
>
> Yep, it should be same as REINDEX DATABASE.
> IMO, we can put them into one function if they use exactly same logic.
> Thought?
>
> >
> > 3) The patch was applied to master and compile without warnings
> >
> >
> > 4) Typo (... does not have any table)
> >
> > + if (!reindex_schema(heapOid))
> > + ereport(NOTICE,
> > + (errmsg("schema\"%s\" does not hava any table",
> > + schema->relname)));
> >
>
> Thanks! I will correct typo.
>
> >
> > 5) Missing of regression tests, please add it to
> > src/test/regress/sql/create_index.sql
> >
> > 6) You need to add psql complete tabs
> >
>
> Next version patch, that I will submit, will have 5), 6) things you pointed.
>
> > 7) I think we can add "-S / --schema" option do reindexdb in this patch too.
> > What do you think?
> >
>
> +1 to add "-S / --schema" option
> I was misunderstanding about that.
> I will make the patch which adds this option as separate patch.
>
[1] https://commitfest.postgresql.org/action/patch_view?id=1598
--
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
On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Alvaro Herrera (alvherre@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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> wrote: >> * Alvaro Herrera (alvherre@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. I changed syntax to REINDEX ALL IN SCHEMA, but I felt a sense of discomfort a little as Robert mentioned. 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 001_Add_schema_option_to_reindexdb_v1.patch : It contains reindexdb "-S/--schema" supporting Please review and comments. Regards, ------- Sawada Masahiko
Attachment
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> * Alvaro Herrera (alvherre@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.
>
> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> * Alvaro Herrera (alvherre@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... :-)
> 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
> 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
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
> "-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
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
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.mshk@gmail.com> > wrote: >> >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com> >> wrote: >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> >> > wrote: >> >> * Alvaro Herrera (alvherre@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. I agree 2) - 5). Attached patch is latest version patch I modified above. Also, I noticed I had forgotten to add the patch regarding document of reindexdb. Please review and comments. Regards, ------- Sawada Masahiko
Attachment
On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.mshk@gmail.com>
> > wrote:
> >>
> >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com>
> >> wrote:
> >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net>
> >> > wrote:
> >> >> * Alvaro Herrera (alvherre@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.
> Also, I noticed I had forgotten to add the patch regarding document of
> reindexdb.
>
> reindexdb.
>
Yeah... I forgot it too... :-) I'm not a native speaker but IMHO the docs is fine.
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
--
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
On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
> > <fabriziomello@gmail.com> wrote:
> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.mshk@gmail.com>
> > > wrote:
> > >>
> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com>
> > >> wrote:
> > >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net>
> > >> > wrote:
> > >> >> * Alvaro Herrera (alvherre@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
Attachment
On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > > On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> >> >> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com> >> wrote: >> > >> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello >> > <fabriziomello@gmail.com> wrote: >> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko >> > > <sawada.mshk@gmail.com> >> > > wrote: >> > >> >> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com> >> > >> wrote: >> > >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net> >> > >> > wrote: >> > >> >> * Alvaro Herrera (alvherre@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. > Thank you for reviewing and revising! I also did successfully. It looks good to me :) Regards, ------- Sawada Masahiko
On Mon, Oct 20, 2014 at 11:24 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >
> >
> > On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
> > <fabriziomello@gmail.com> wrote:
> >>
> >>
> >> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com>
> >> wrote:
> >> >
> >> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
> >> > <fabriziomello@gmail.com> wrote:
> >> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko
> >> > > <sawada.mshk@gmail.com>
> >> > > wrote:
> >> > >>
> >> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com>
> >> > >> wrote:
> >> > >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfrost@snowman.net>
> >> > >> > wrote:
> >> > >> >> * Alvaro Herrera (alvherre@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.
> >
>
> Thank you for reviewing and revising!
You're welcome!
> I also did successfully.
> It looks good to me :)
>
Changed status to "Ready for commiter".
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
> It looks good to me :)
>
Changed status to "Ready for commiter".
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
Sawada Masahiko wrote: > Thank you for reviewing. > I agree 2) - 5). > Attached patch is latest version patch I modified above. > Also, I noticed I had forgotten to add the patch regarding document of > reindexdb. Please don't use pg_catalog in the regression test. That way we will need to update the expected file whenever a new catalog is added, which seems pointless. Maybe create a schema with a couple of tables specifically for this, instead. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Sawada Masahiko wrote:
>
> > Thank you for reviewing.
> > I agree 2) - 5).
> > Attached patch is latest version patch I modified above.
> > Also, I noticed I had forgotten to add the patch regarding document of
> > reindexdb.
>
> Please don't use pg_catalog in the regression test. That way we will
> need to update the expected file whenever a new catalog is added, which
> seems pointless. Maybe create a schema with a couple of tables
> specifically for this, instead.
>
Attached new regression test.
>
> Sawada Masahiko wrote:
>
> > Thank you for reviewing.
> > I agree 2) - 5).
> > Attached patch is latest version patch I modified above.
> > Also, I noticed I had forgotten to add the patch regarding document of
> > reindexdb.
>
> Please don't use pg_catalog in the regression test. That way we will
> need to update the expected file whenever a new catalog is added, which
> seems pointless. Maybe create a schema with a couple of tables
> specifically for this, instead.
>
Attached new regression test.
Isn't better join the two patches in just one?
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
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
Attachment
On Fri, Oct 24, 2014 at 3:41 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> > wrote: >> >> Sawada Masahiko wrote: >> >> > Thank you for reviewing. >> > I agree 2) - 5). >> > Attached patch is latest version patch I modified above. >> > Also, I noticed I had forgotten to add the patch regarding document of >> > reindexdb. >> >> Please don't use pg_catalog in the regression test. That way we will >> need to update the expected file whenever a new catalog is added, which >> seems pointless. Maybe create a schema with a couple of tables >> specifically for this, instead. >> > > Attached new regression test. Hunk #1 FAILED at 1. 1 out of 2 hunks FAILED -- saving rejects to file src/bin/scripts/t/090_reindexdb.pl.rej I tried to apply the 001 patch after applying the 000, but it was not applied cleanly. At least to me, it's more intuitive to use "SCHEMA" instead of "ALL IN SCHEMA" here because we already use "DATABASE" instead of "ALL IN DATABASE". Regards, -- Fujii Masao
On Thu, Nov 6, 2014 at 8:00 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Fri, Oct 24, 2014 at 3:41 AM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> >> wrote: >>> >>> Sawada Masahiko wrote: >>> >>> > Thank you for reviewing. >>> > I agree 2) - 5). >>> > Attached patch is latest version patch I modified above. >>> > Also, I noticed I had forgotten to add the patch regarding document of >>> > reindexdb. >>> >>> Please don't use pg_catalog in the regression test. That way we will >>> need to update the expected file whenever a new catalog is added, which >>> seems pointless. Maybe create a schema with a couple of tables >>> specifically for this, instead. >>> >> >> Attached new regression test. > > Hunk #1 FAILED at 1. > 1 out of 2 hunks FAILED -- saving rejects to file > src/bin/scripts/t/090_reindexdb.pl.rej > > I tried to apply the 001 patch after applying the 000, but it was not > applied cleanly. > > At least to me, it's more intuitive to use "SCHEMA" instead of "ALL IN SCHEMA" > here because we already use "DATABASE" instead of "ALL IN DATABASE". > Thank you for reporting. Um, that's one way of looking at it I think It's not quite new syntax, and we have not used "ALL IN" clause in REINDEX command by now. If we add SQL command to reindex table of all in table space as newly syntax, we might be able to name it "REINDEX TABLE ALL IN TABLESPACE". Anyway, I created patch of "REINDEX SCHEMA" version, and attached. Also inapplicable problem has been solved. Regards, ------- Sawada Masahiko
Attachment
On 23 October 2014 00:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> Attached patch is latest version patch I modified above. >> Also, I noticed I had forgotten to add the patch regarding document of >> reindexdb. > > Please don't use pg_catalog in the regression test. That way we will > need to update the expected file whenever a new catalog is added, which > seems pointless. Maybe create a schema with a couple of tables > specifically for this, instead. These patches look fine to me. Don't see anybody objecting either. Are you looking to commit them, or shall I? -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 23 October 2014 00:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >>> Attached patch is latest version patch I modified above. >>> Also, I noticed I had forgotten to add the patch regarding document of >>> reindexdb. >> >> Please don't use pg_catalog in the regression test. That way we will >> need to update the expected file whenever a new catalog is added, which >> seems pointless. Maybe create a schema with a couple of tables >> specifically for this, instead. > > These patches look fine to me. Don't see anybody objecting either. > > Are you looking to commit them, or shall I? IMO, SCHEMA is just but fine, that's more consistent with the existing syntax we have for database and tables. Btw, there is an error in this patch, there are no ACL checks on the schema for the user doing the REINDEX, so any user is able to perform a REINDEX on any schema. Here is an example for a given user, let's say "poo'": => create table foo.g (a int); ERROR: 42501: permission denied for schema foo LOCATION: aclcheck_error, aclchk.c:3371 => reindex schema foo ; NOTICE: 00000: table "foo.c" was reindexed LOCATION: ReindexDatabaseOrSchema, indexcmds.c:1930 REINDEX A regression test to check that would be good as well. Also, ISTM that it is awkward to modify the values of do_user and do_system like that in ReindexDatabase for two reasons: 1) They should be set in gram.y 2) This patch passes as a new argument of ReindexDatabase the object type, something that is overlapping with what do_user and do_system are aimed for. Why not simply defining a new object type OBJECT_SYSTEM? As this patch introduces a new object category for REINDEX, this patch seems to be a good occasion to remove the boolean dance in REINDEX at the cost of having a new object type for the concept of a system, which would be a way to define the system catalogs as a whole. Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. So, I think that we need to think a bit more here. We are not far from smth that could be committed, so marking as "Waiting on Author" for now. Thoughts? -- Michael
On Wed, Nov 26, 2014 at 3:48 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On 23 October 2014 00:21, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >>>> Attached patch is latest version patch I modified above. >>>> Also, I noticed I had forgotten to add the patch regarding document of >>>> reindexdb. >>> >>> Please don't use pg_catalog in the regression test. That way we will >>> need to update the expected file whenever a new catalog is added, which >>> seems pointless. Maybe create a schema with a couple of tables >>> specifically for this, instead. >> >> These patches look fine to me. Don't see anybody objecting either. >> >> Are you looking to commit them, or shall I? > > IMO, SCHEMA is just but fine, that's more consistent with the existing > syntax we have for database and tables. > > Btw, there is an error in this patch, there are no ACL checks on the > schema for the user doing the REINDEX, so any user is able to perform > a REINDEX on any schema. Here is an example for a given user, let's > say "poo'": > => create table foo.g (a int); > ERROR: 42501: permission denied for schema foo > LOCATION: aclcheck_error, aclchk.c:3371 > => reindex schema foo ; > NOTICE: 00000: table "foo.c" was reindexed > LOCATION: ReindexDatabaseOrSchema, indexcmds.c:1930 > REINDEX > A regression test to check that would be good as well. > Thank you for testing this patch. It's a bug. I will fix it and add test case to regression test. > Also, ISTM that it is awkward to modify the values of do_user and > do_system like that in ReindexDatabase for two reasons: > 1) They should be set in gram.y > 2) This patch passes as a new argument of ReindexDatabase the object > type, something that is overlapping with what do_user and do_system > are aimed for. Why not simply defining a new object type > OBJECT_SYSTEM? As this patch introduces a new object category for > REINDEX, this patch seems to be a good occasion to remove the boolean > dance in REINDEX at the cost of having a new object type for the > concept of a system, which would be a way to define the system > catalogs as a whole. +1 to define new something object type and remove do_user and do_system. But if we add OBJECT_SYSTEM to ObjectType data type, system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. It's a bit redundant? > Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. > So, I think that we need to think a bit more here. We are not far from > smth that could be committed, so marking as "Waiting on Author" for > now. Thoughts? Is the table also kind of "object"? Regards, ------- Sawada Masahiko
On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > +1 to define new something object type and remove do_user and do_system. > But if we add OBJECT_SYSTEM to ObjectType data type, > system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. > It's a bit redundant? Yes, kind of. That's a superset of a type of relations, aka a set of catalog tables. If you find something cleaner to propose, feel free. >> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. >> So, I think that we need to think a bit more here. We are not far from >> smth that could be committed, so marking as "Waiting on Author" for >> now. Thoughts? > > Is the table also kind of "object"? Sorry, I am not sure I follow you here. Indexes and tables have already their relkind set in ReindexStmt, and I think that we're fine to continue letting them go in their own reindex code path for now. -- Michael
On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> +1 to define new something object type and remove do_user and do_system. >> But if we add OBJECT_SYSTEM to ObjectType data type, >> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. >> It's a bit redundant? > Yes, kind of. That's a superset of a type of relations, aka a set of > catalog tables. If you find something cleaner to propose, feel free. I thought we can add new struct like ReindexObjectType which has REINDEX_OBJECT_TABLE, REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax. >>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. >>> So, I think that we need to think a bit more here. We are not far from >>> smth that could be committed, so marking as "Waiting on Author" for >>> now. Thoughts? >> >> Is the table also kind of "object"? > Sorry, I am not sure I follow you here. Indexes and tables have > already their relkind set in ReindexStmt, and I think that we're fine > to continue letting them go in their own reindex code path for now. It was not enough, sorry. I mean that there is already ReindexTable() function. if we renamed ReindexObject, I would feel uncomfortable feeling. Because table is also kind of "object". Regards, ------- Sawada Masahiko
On Thu, Nov 27, 2014 at 8:18 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> +1 to define new something object type and remove do_user and do_system. >>> But if we add OBJECT_SYSTEM to ObjectType data type, >>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. >>> It's a bit redundant? >> Yes, kind of. That's a superset of a type of relations, aka a set of >> catalog tables. If you find something cleaner to propose, feel free. > > I thought we can add new struct like ReindexObjectType which has > REINDEX_OBJECT_TABLE, > REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax. Check. >>>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. >>>> So, I think that we need to think a bit more here. We are not far from >>>> smth that could be committed, so marking as "Waiting on Author" for >>>> now. Thoughts? >>> >>> Is the table also kind of "object"? >> Sorry, I am not sure I follow you here. Indexes and tables have >> already their relkind set in ReindexStmt, and I think that we're fine >> to continue letting them go in their own reindex code path for now. > > It was not enough, sorry. > I mean that there is already ReindexTable() function. > if we renamed ReindexObject, I would feel uncomfortable feeling. > Because table is also kind of "object". Check. If you get that done, I'll have an extra look at it and then let's have a committer look at it. Regards, -- Michael
On Thu, Nov 27, 2014 at 11:43 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Nov 27, 2014 at 8:18 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>>> +1 to define new something object type and remove do_user and do_system. >>>> But if we add OBJECT_SYSTEM to ObjectType data type, >>>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE. >>>> It's a bit redundant? >>> Yes, kind of. That's a superset of a type of relations, aka a set of >>> catalog tables. If you find something cleaner to propose, feel free. >> >> I thought we can add new struct like ReindexObjectType which has >> REINDEX_OBJECT_TABLE, >> REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax. > Check. > >>>>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject. >>>>> So, I think that we need to think a bit more here. We are not far from >>>>> smth that could be committed, so marking as "Waiting on Author" for >>>>> now. Thoughts? >>>> >>>> Is the table also kind of "object"? >>> Sorry, I am not sure I follow you here. Indexes and tables have >>> already their relkind set in ReindexStmt, and I think that we're fine >>> to continue letting them go in their own reindex code path for now. >> >> It was not enough, sorry. >> I mean that there is already ReindexTable() function. >> if we renamed ReindexObject, I would feel uncomfortable feeling. >> Because table is also kind of "object". > Check. > > If you get that done, I'll have an extra look at it and then let's > have a committer look at it. Attached patch is latest version. I added new enum values like REINDEX_OBJECT_XXX, and changed ReindexObject() function. Also ACL problem is fixed for this version. Regards, ------- Sawada Masahiko
Attachment
On Mon, Dec 1, 2014 at 11:29 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, Nov 27, 2014 at 11:43 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> If you get that done, I'll have an extra look at it and then let's >> have a committer look at it. > > Attached patch is latest version. > I added new enum values like REINDEX_OBJECT_XXX, > and changed ReindexObject() function. > Also ACL problem is fixed for this version. Thanks for the updated version. I have been looking at this patch more deeply, and I noticed a couple of things that were missing or mishandled: - The patch completely ignored that a catalog schema could be reindexed - When reindexing pg_catalog as a schema, it is critical to reindex pg_class first as reindex_relation updates pg_class. - gram.y generated some warnings because ReindexObjectType stuff was casted as ObjectType. - There was a memory leak, the scan keys palloc'd in ReindexObject were not pfree'd - Using do_user, do_system and now do_database was really confusing and reduced code lisibility. I reworked it using the object kind - The patch to support SCHEMA in reindexdb has been forgotten. Reattaching it here. Adding on top of that a couple of things cleaned up, like docs and typos, and I got the patch attached. Let's have a committer have a look a it now, I am marking that as "Ready for Committer". Regards, -- Michael
Attachment
On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Adding on top of that a couple of things cleaned up, like docs and > typos, and I got the patch attached. Let's have a committer have a > look a it now, I am marking that as "Ready for Committer". For the archives, this has been committed as fe263d1. Thanks Simon for looking and the final push. And sorry that I didn't spot the issue with tap tests when reviewing, check-world passed but my dev VM missed necessary perl packages. Regards, -- Michael
On Tue, Dec 9, 2014 at 10:10 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Adding on top of that a couple of things cleaned up, like docs and >> typos, and I got the patch attached. Let's have a committer have a >> look a it now, I am marking that as "Ready for Committer". > For the archives, this has been committed as fe263d1. Thanks Simon for > looking and the final push. And sorry that I didn't spot the issue > with tap tests when reviewing, check-world passed but my dev VM missed > necessary perl packages. While re-looking at that. I just found that when selecting the relations that are reindexed for a schema we ignore materialized view as the key scan is only done using 'r' as relkind. The patch attached fixes that. Thanks, -- Michael
Attachment
On Tue, Dec 9, 2014 at 4:44 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Dec 9, 2014 at 10:10 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> Adding on top of that a couple of things cleaned up, like docs and >>> typos, and I got the patch attached. Let's have a committer have a >>> look a it now, I am marking that as "Ready for Committer". >> For the archives, this has been committed as fe263d1. Thanks Simon for >> looking and the final push. And sorry that I didn't spot the issue >> with tap tests when reviewing, check-world passed but my dev VM missed >> necessary perl packages. > While re-looking at that. I just found that when selecting the > relations that are reindexed for a schema we ignore materialized view > as the key scan is only done using 'r' as relkind. The patch attached > fixes that. Here is an updated patch doing as well that: - Regression test checking if user has permissions on schema was broken - Silent NOTICE messages of REINDEX by having client_min_messages set to WARINING (thoughts about having a plpgsql function doing consistency checks of relfilenode before and after reindex?) -- Michael
Attachment
On 9 December 2014 at 17:17, Michael Paquier <michael.paquier@gmail.com> wrote: >> While re-looking at that. I just found that when selecting the >> relations that are reindexed for a schema we ignore materialized view >> as the key scan is only done using 'r' as relkind. The patch attached >> fixes that. > Here is an updated patch doing as well that: > - Regression test checking if user has permissions on schema was broken > - Silent NOTICE messages of REINDEX by having client_min_messages set > to WARINING (thoughts about having a plpgsql function doing > consistency checks of relfilenode before and after reindex?) ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in the way it issues NOTICE messages. I'm inclined to simply remove the NOTICE messages, except when a REINDEX ... VERBOSE is requested. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tuesday, December 9, 2014, Simon Riggs <simon@2ndquadrant.com> wrote:
On 9 December 2014 at 17:17, Michael Paquier <michael.paquier@gmail.com> wrote:
>> While re-looking at that. I just found that when selecting the
>> relations that are reindexed for a schema we ignore materialized view
>> as the key scan is only done using 'r' as relkind. The patch attached
>> fixes that.
> Here is an updated patch doing as well that:
> - Regression test checking if user has permissions on schema was broken
> - Silent NOTICE messages of REINDEX by having client_min_messages set
> to WARINING (thoughts about having a plpgsql function doing
> consistency checks of relfilenode before and after reindex?)
ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
the way it issues NOTICE messages.
I'm inclined to simply remove the NOTICE messages, except when a
REINDEX ... VERBOSE is requested.
+1 to remove the NOTICE messages except when specifying VERBOSE.
It would output a lot of messages if there are many table in schema.
If nobody objects to it, I will work on this.
Regards,
--
Sawada Masahiko
--
Regards,
-------
Sawada Masahiko
On Tue, Dec 9, 2014 at 6:00 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 9 December 2014 at 17:17, Michael Paquier <michael.paquier@gmail.com> wrote: > >>> While re-looking at that. I just found that when selecting the >>> relations that are reindexed for a schema we ignore materialized view >>> as the key scan is only done using 'r' as relkind. The patch attached >>> fixes that. >> Here is an updated patch doing as well that: >> - Regression test checking if user has permissions on schema was broken >> - Silent NOTICE messages of REINDEX by having client_min_messages set >> to WARINING (thoughts about having a plpgsql function doing >> consistency checks of relfilenode before and after reindex?) > > ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in > the way it issues NOTICE messages. > > I'm inclined to simply remove the NOTICE messages, except when a > REINDEX ... VERBOSE is requested. OK. Perhaps that's not worth mentioning in the release notes, but some users may be used to the old behavior. What about the other issues (regression test for permissions incorrect and matviews)? -- Michael
On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > OK. Perhaps that's not worth mentioning in the release notes, but some > users may be used to the old behavior. What about the other issues > (regression test for permissions incorrect and matviews)? Here is in any case an updated patch to fix all those things rebased on latest HEAD (ae4e688). Thanks, -- Michael
Attachment
Simon Riggs <simon@2ndQuadrant.com> writes: > ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in > the way it issues NOTICE messages. > I'm inclined to simply remove the NOTICE messages, except when a > REINDEX ... VERBOSE is requested. My recollection is that those other commands do issue messages always, but at some DEBUGn log level when not VERBOSE. Ideally REINDEX would adopt that same behavior. regards, tom lane
On Tue, Dec 9, 2014 at 11:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in >> the way it issues NOTICE messages. > >> I'm inclined to simply remove the NOTICE messages, except when a >> REINDEX ... VERBOSE is requested. > > My recollection is that those other commands do issue messages always, > but at some DEBUGn log level when not VERBOSE. Ideally REINDEX would > adopt that same behavior. > So it seems to that REINDEX command issues messages as INFO level when that is specified VERBOSE option. If not specified, it issue message as DEBUG2. Regards, ------- Sawada Masahiko
<div dir="ltr"><div class="gmail_extra">On Tue, Dec 9, 2014 at 10:21 AM, Michael Paquier <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />><br />> On Tue, Dec 9, 2014 at6:43 PM, Michael Paquier<br />> <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>> wrote:<br/>> > OK. Perhaps that's not worth mentioning in the release notes, but some<br />> > users may be usedto the old behavior. What about the other issues<br />> > (regression test for permissions incorrect and matviews)?<br/>> Here is in any case an updated patch to fix all those things rebased<br />> on latest HEAD (ae4e688).<br/>><br /><br /></div><div class="gmail_extra">The patch is fine:<br /></div><div class="gmail_extra">- Nocompiler warnings<br /></div><div class="gmail_extra">- Added properly regressions tests and run ok<br /></div><div class="gmail_extra"><br/></div><div class="gmail_extra">There are no changes in the docs. Maybe we can mention matviews onREINDEX SCHEMA docs, what do you think?<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><br />--<br />Fabrízio de Royes Mello<br />Consultoria/CoachingPostgreSQL<br />>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>
On Wed, Dec 10, 2014 at 1:37 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > On Tue, Dec 9, 2014 at 10:21 AM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >> > OK. Perhaps that's not worth mentioning in the release notes, but some >> > users may be used to the old behavior. What about the other issues >> > (regression test for permissions incorrect and matviews)? >> Here is in any case an updated patch to fix all those things rebased >> on latest HEAD (ae4e688). >> > > The patch is fine: > - No compiler warnings > - Added properly regressions tests and run ok > > There are no changes in the docs. Maybe we can mention matviews on REINDEX > SCHEMA docs, what do you think? Current documentation tells that all the indexes in schema are reindexed, only matviews and relations can have one, so that's implicitly specified IMO. If in the future we add support for another relkind and that it can have indexes, we won't need to update the docs as well. -- Michael
On 9 December 2014 at 12:21, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> OK. Perhaps that's not worth mentioning in the release notes, but some >> users may be used to the old behavior. What about the other issues >> (regression test for permissions incorrect and matviews)? > Here is in any case an updated patch to fix all those things rebased > on latest HEAD (ae4e688). > Thanks, I rewrote and applied this patch to ensure we didn't introduce further bugs. Tests for the reindex part were rewritten from scratch also. Rather unluckily that seems to have coincided with Tom's changes, so I've only added the bits that have nothing to do with Tom's changes - all of which stand. (I just got going again after my flight back from Tokyo). -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Dec 12, 2014 at 8:00 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Glad that things are in order now. I have nothing else left to complain about with this feature. Thanks.
I understand going east makes one day longer :)I rewrote and applied this patch to ensure we didn't introduce further bugs.On 9 December 2014 at 12:21, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> OK. Perhaps that's not worth mentioning in the release notes, but some
>> users may be used to the old behavior. What about the other issues
>> (regression test for permissions incorrect and matviews)?
> Here is in any case an updated patch to fix all those things rebased
> on latest HEAD (ae4e688).
> Thanks,
Tests for the reindex part were rewritten from scratch also.
Rather unluckily that seems to have coincided with Tom's changes, so
I've only added the bits that have nothing to do with Tom's changes -
all of which stand.
Glad that things are in order now. I have nothing else left to complain about with this feature. Thanks.
(I just got going again after my flight back from Tokyo).
--
Michael
On Fri, Dec 12, 2014 at 8:41 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
Actually there is one thing left: this patch from Sawada-san to finish the cleanup of ReindexStmt for the boolean flags.
http://www.postgresql.org/message-id/CAD21AoCXFE1J+hSkbeJ80rqqnhR8m_YUxdGKwZ4dL8zPqT8gjg@mail.gmail.com
On Fri, Dec 12, 2014 at 8:00 AM, Simon Riggs <simon@2ndquadrant.com> wrote:Rather unluckily that seems to have coincided with Tom's changes, so
I've only added the bits that have nothing to do with Tom's changes -
all of which stand.Glad that things are in order now. I have nothing else left to complain about with this feature. Thanks.
http://www.postgresql.org/message-id/CAD21AoCXFE1J+hSkbeJ80rqqnhR8m_YUxdGKwZ4dL8zPqT8gjg@mail.gmail.com
Regards,
--
Michael
On 11 December 2014 at 23:41, Michael Paquier <michael.paquier@gmail.com> wrote: > Glad that things are in order now. I have nothing else left to complain > about with this feature. Thanks. I think you've discovered a new meaning of Ready for Committer -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Fri, Dec 12, 2014 at 10:19 AM, Simon Riggs<span dir="ltr"><<a href="mailto:simon@2ndquadrant.com" target="_blank">simon@2ndquadrant.com</a>></span> wrote:<blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On11 December 2014 at 23:41, Michael Paquier <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br /><br /> > Glad that things are inorder now. I have nothing else left to complain<br /> > about with this feature. Thanks.<br /><br /></span>I think you'vediscovered a new meaning of Ready for Committer<br clear="all" /></blockquote></div>Yep. Sorry for missing so manythings.<br /></div><div class="gmail_extra">-- <br /><div class="gmail_signature">Michael<br /></div></div></div>