Re: Proposal : REINDEX SCHEMA

On Wed, Nov 26, 2014 at 3:48 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 23 October 2014 00:21, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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