On Tue, Sep 27, 2016 at 12:45 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:
> Hello Stephen,
>
> I am reviewing the latest patch in detail now and will post my review
> comments later.
>
Here are the review comments:
1. In documentation, we should put both permissive as well as restrictive in
the header like permissive|restrictive. Or may be a generic term, say,
policy
type (like we have command and then options mentioned like all, select
etc.),
followed by options in the description. Or like we have CASCADE and RESTRICT
in drop case.
2. "If the policy is a "permissive" or "restrictive" policy." seems broken
as
sentence starts with "If" and there is no other part to it. Will it be
better
to say "Specifies whether the policy is a "permissive" or "restrictive"
policy."?
3. " .. a policy can instead by "restrictive""
Do you mean "instead be" here?
4. It will be good if we have an example for this in section
"5.7. Row Security Policies"
5. AS ( PERMISSIVE | RESTRICTIVE )
should be '{' and '}' instead of parenthesis.
6. I think following changes are irrelevant for this patch.
Should be submitted as separate patch if required.
@@ -2133,6 +2139,25 @@ psql_completion(const char *text, int start, int end)
/* Complete "CREATE POLICY <name> ON <table> USING (" */
else if (Matches6("CREATE", "POLICY", MatchAny, "ON", MatchAny,
"USING"))
COMPLETE_WITH_CONST("(");
+ /* CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR
ALL|SELECT|INSERT|UPDATE|DELETE */
+ else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR"))
+ COMPLETE_WITH_LIST5("ALL", "SELECT", "INSERT", "UPDATE", "DELETE");
+ /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE
FOR INSERT TO|WITH CHECK" */
+ else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "INSERT"))
+ COMPLETE_WITH_LIST2("TO", "WITH CHECK (");
+ /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE
FOR SELECT|DELETE TO|USING" */
+ else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "SELECT|DELETE"))
+ COMPLETE_WITH_LIST2("TO", "USING (");
+ /* CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE FOR
ALL|UPDATE TO|USING|WITH CHECK */
+ else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "ALL|UPDATE"))
+ COMPLETE_WITH_LIST3("TO", "USING (", "WITH CHECK (");
+ /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE
TO <role>" */
+ else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "TO"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);
+ /* Complete "CREATE POLICY <name> ON <table> AS PERMISSIVE|RESTRICTIVE
USING (" */
+ else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "USING"))
+ COMPLETE_WITH_CONST("(");
7. Natts_pg_policy should be updated to 7 now.
8. In following error, $2 and @2 should be used to correctly display the
option and location.
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("unrecognized row security option
\"%s\"", $1),
errhint("Only PERMISSIVE or RESTRICTIVE
policies are supported currently."),
parser_errposition(@1)));
You will see following result otherwise:
postgres=# CREATE POLICY my_policy ON foo AS a123;
ERROR: unrecognized row security option "as"
LINE 1: CREATE POLICY my_policy ON foo AS a123;
^
HINT: Only PERMISSIVE or RESTRICTIVE policies are supported currently.
I think adding negative test to test this error should be added in
regression.
9. Need to update following comments in gram.y to reflect new changes.
* QUERIES:
* CREATE POLICY name ON table [FOR cmd] [TO role, ...]
* [USING (qual)] [WITH CHECK (with_check)]
10. ALTER POLICY has no changes for this. Any reason why that is not
considered here.
11. Will it be better to use boolean for polpermissive in _policyInfo?
And then set that appropriately while getting the policies. So that later we
only need to test the boolean avoiding string comparison.
12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
appropriate, like other default cases.
strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"
13. Since PERMISSIVE is default, do we need changes like below?
- \QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
+ \QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
PUBLIC \E
I am not familiar or used TAP framework. So no idea about these changes.
14. While displaying policy details in permissionsList, per syntax, we
should
display (RESTRICT) before the command option. Also will it be better to use
(RESTRICTIVE) instead of (RESTRICT)?
15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
after
policy name and before command option ?
If we do that then changes related to adding "POLICY" followed by
"RESTRICTIVE"
will be straight forward.
16. It be good to have test-coverage for permissionsList,
describeOneTableDetails and dump-restore changes. Please add those.
17. In pg_policies view, we need to add details related to PERMISSIVE and
RESTRICTIVE. Please do so. Also add test for it.
18. Fix typos pointed earlier by Alvera.
Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
