On Wed, Nov 26, 2014 at 3:48 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <si...@2ndquadrant.com> wrote: >> On 23 October 2014 00:21, Alvaro Herrera <alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
