On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> On 2020/02/03 14:26, Amit Langote wrote:
> > On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao <masao.fu...@oss.nttdata.com>
> > wrote:
> >> On 2020/02/03 11:05, Amit Langote wrote:
> >>> Okay. How about the attached?
> >>
> >> Thanks for the patches! You added the note just after the description
> >> about row level security on inherited table, but isn't it better to
> >> add it before that? Attached patch does that. Thought?
> >
> > Yeah, that might be a better flow for that paragraph.
>
> Pushed! Thanks!
Thank you.
> >>> Maybe, we should also note the LOCK TABLE exception?
> >>
> >> Yes.
> >
> > Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too,
> > but maybe you're aware of that.
>
> Yes, so I will review your patch getting rid of
> LOCK TABLE exception.
Attached updated patch.
Regards,
Amit
From 77bf8ac02734b75f8a1e7c61bfc84e1b37189893 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangot...@gmail.com>
Date: Wed, 22 Jan 2020 15:43:41 +0900
Subject: [PATCH v2] Don't check child's LOCK privilege when locked recursively
---
src/backend/commands/lockcmds.c | 33 +++++++++++---------------------
src/test/regress/expected/lock.out | 8 ++------
src/test/regress/expected/privileges.out | 7 +++++++
src/test/regress/sql/lock.sql | 7 ++-----
src/test/regress/sql/privileges.sql | 6 ++++++
5 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 329ab849c0..d8cafc42bb 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -28,7 +28,7 @@
#include "utils/lsyscache.h"
#include "utils/syscache.h"
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid
userid);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
@@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt)
if (get_rel_relkind(reloid) == RELKIND_VIEW)
LockViewRecurse(reloid, lockstmt->mode,
lockstmt->nowait, NIL);
else if (recurse)
- LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait, GetUserId());
+ LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait);
}
}
@@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid
relid, Oid oldrelid,
/*
* Apply LOCK TABLE recursively over an inheritance tree
*
- * We use find_inheritance_children not find_all_inheritors to avoid taking
- * locks far in advance of checking privileges. This means we'll visit
- * multiply-inheriting children more than once, but that's no problem.
+ * This doesn't check permission to perform LOCK TABLE on the child tables,
+ * because getting here means that the user has permission to lock the
+ * parent which is enough.
*/
static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
{
List *children;
ListCell *lc;
- children = find_inheritance_children(reloid, NoLock);
+ children = find_all_inheritors(reloid, NoLock, NULL);
foreach(lc, children)
{
Oid childreloid = lfirst_oid(lc);
- AclResult aclresult;
- /* Check permissions before acquiring the lock. */
- aclresult = LockTableAclCheck(childreloid, lockmode, userid);
- if (aclresult != ACLCHECK_OK)
- {
- char *relname = get_rel_name(childreloid);
-
- if (!relname)
- continue; /* child concurrently
dropped, just skip it */
- aclcheck_error(aclresult,
get_relkind_objtype(get_rel_relkind(childreloid)), relname);
- }
+ /* Parent already locked. */
+ if (childreloid == reloid)
+ continue;
- /* We have enough rights to lock the relation; do so. */
if (!nowait)
LockRelationOid(childreloid, lockmode);
else if (!ConditionalLockRelationOid(childreloid, lockmode))
@@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool
nowait, Oid userid)
UnlockRelationOid(childreloid, lockmode);
continue;
}
-
- LockTableRecurse(childreloid, lockmode, nowait, userid);
}
}
@@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context
*context)
if (relkind == RELKIND_VIEW)
LockViewRecurse(relid, context->lockmode,
context->nowait, context->ancestor_views);
else if (rte->inh)
- LockTableRecurse(relid, context->lockmode,
context->nowait, context->viewowner);
+ LockTableRecurse(relid, context->lockmode,
context->nowait);
}
return query_tree_walker(query,
diff --git a/src/test/regress/expected/lock.out
b/src/test/regress/expected/lock.out
index 185fd2f879..898fd6accd 100644
--- a/src/test/regress/expected/lock.out
+++ b/src/test/regress/expected/lock.out
@@ -138,16 +138,12 @@ CREATE TABLE lock_tbl3 () INHERITS (lock_tbl2);
BEGIN TRANSACTION;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Child tables are locked without granting explicit permission to do so as
+-- long as we have permission to lock the parent.
GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
SET ROLE regress_rol_lock1;
BEGIN;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
-ERROR: permission denied for table lock_tbl2
-ROLLBACK;
-BEGIN;
-LOCK TABLE ONLY lock_tbl1;
ROLLBACK;
RESET ROLE;
--
diff --git a/src/test/regress/expected/privileges.out
b/src/test/regress/expected/privileges.out
index 297cedbacf..c2d037b614 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -716,6 +716,13 @@ ERROR: permission denied for table atestc
TRUNCATE atestp1; -- ok
TRUNCATE atestc; -- fail
ERROR: permission denied for table atestc
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+ERROR: permission denied for table atestc
+END;
-- privileges on functions, languages
-- switch to superuser
\c -
diff --git a/src/test/regress/sql/lock.sql b/src/test/regress/sql/lock.sql
index 26a7e59a13..8947d8e52e 100644
--- a/src/test/regress/sql/lock.sql
+++ b/src/test/regress/sql/lock.sql
@@ -101,16 +101,13 @@ BEGIN TRANSACTION;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Child tables are locked without granting explicit permission to do so as
+-- long as we have permission to lock the parent.
GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
SET ROLE regress_rol_lock1;
BEGIN;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
-BEGIN;
-LOCK TABLE ONLY lock_tbl1;
-ROLLBACK;
RESET ROLE;
--
diff --git a/src/test/regress/sql/privileges.sql
b/src/test/regress/sql/privileges.sql
index dfe2603fe2..2ba69617dc 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -459,6 +459,12 @@ UPDATE atestp1 SET f1 = 1; -- ok
UPDATE atestc SET f1 = 1; -- fail
TRUNCATE atestp1; -- ok
TRUNCATE atestc; -- fail
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+END;
-- privileges on functions, languages
--
2.16.5
