Re: Proposal : REINDEX SCHEMA - Mailing list pgsql-hackers

From Sawada Masahiko
Subject Re: Proposal : REINDEX SCHEMA
Date
Msg-id CAD21AoBkcWjzbMew=dfKA6RwCRYcjkvZ7QUyntegi+gPoA2QRQ@mail.gmail.com
Whole thread Raw
In response to Re: Proposal : REINDEX SCHEMA  (Fabrízio de Royes Mello <fabriziomello@gmail.com>)
Responses Re: Proposal : REINDEX SCHEMA
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Heikki Linnakangas
Date:
Subject: Re: [9.4 bug] The database server hangs with write-heavy workload on Windows
Next
From: Petr Jelinek
Date:
Subject: Re: tracking commit timestamps