Re: Proposal : REINDEX SCHEMA

On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>
>> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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