Stephen Frost wrote: > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> BTW, I raised a few issues. Do you have any opinions? > > Certainly, though they're my opinions and I don't know if the committers > will agree, but I suspect they will.
Thanks for your comments. >> * deployment of the source code >> >> The current patch implements all the access control abstractions at the >> src/backend/security/access_control.c. Its size is about 4,500 lines >> which includs source comments. >> It is an approach to sort out a series of functionalities into a single >> big file, such as aclchk.c. One other approach is to put these codes in >> the short many files deployed in a directory, such as backend/catalog/*. >> Which is the preferable in PostgreSQL? > > A single, larger file, as implemented, is preferable, I believe. OK, >> * pg_class_ownercheck() in EnableDisableRule() >> >> As I mentioned in the another message, pg_class_ownercheck() in the >> EnableDisableRule() is redundant. >> >> The EnableDisableRule() is called from ATExecEnableDisableRule() only, >> and it is also called from the ATExecCmd() with AT_EnableRule, >> AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule. >> In this path, ATPrepCmd() already calls ATSimplePermissions() which >> also calls pg_class_ownercheck() for the target. >> >> I don't think it is necessary to check ownership of the relation twice. >> My opinion is to remove the checks from the EnableDisableRule() and >> the ac_rule_toggle() is also removed from the patch. >> It does not have any compatibility issue. >> >> Any comments? > > I agree that we don't need to check the ownership twice. You might > check if there was some history to having both checks (perhaps there was > another code path before which didn't check before calling > EnableDisableRule()?). I'd feel alot more comfortable removing the > check if we can show why it was there originally as well as why it's not > needed now. I checked history of the repository, and this commit adds EnableDisableRule(). * Changes pg_trigger and extend pg_rewrite in order to allow triggers and Jan Wieck [Mon, 19 Mar 2007 23:38:32 +0000 (23:38 +0000)] http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=31e6bde46c0594c6f8bb82606c49f93295f1195c#patch8 The corresponding AT_xxxx command calls ATSimplePermissions() from ATPrepCmd(), and ATExecCmd() is the only caller from the begining. It seems to me this redundant check does not have any explicit reason. I think it is harmless to remove this pg_class_ownercheck() from here. > I'm working on a more comprehensive review, but wanted to answer these > questions first. Thanks for your efforts. I'm looking forward to see rest of the comments. -- OSS Platform Development Division, NEC KaiGai Kohei <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
