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
