On Thu, Feb 13, 2020 at 8:39 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> On 2020/02/07 10:39, Amit Langote wrote:
> > On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao <masao.fu...@oss.nttdata.com>
> > wrote:
> >> Yes, so I will review your patch getting rid of
> >> LOCK TABLE exception.
> >
> > Attached updated patch.
>
> Thanks! This patch basically looks good to me except
> the following minor comment.
>
> ROLLBACK;
> -BEGIN;
> -LOCK TABLE ONLY lock_tbl1;
> -ROLLBACK;
> RESET ROLE;
>
> I think that there is no strong reason why these SQLs need to be
> removed. We can verify that even "LOCK TABLE ONLY" command works
> expectedly on the inherited tables by keeping those SQLs in the
> regression test. So what about not removing these SQLs?
Hmm, that test becomes meaningless with the behavior change we are
introducing, but I am okay with not removing it.
However, I added a test showing that locking child table directly doesn't work.
Attached updated patch.
Thanks,
Amit
From 71a2dcbb00928dc695831ceace5992d729436fcd Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangot...@gmail.com>
Date: Wed, 22 Jan 2020 15:43:41 +0900
Subject: [PATCH v3] Don't check child's LOCK privilege when locked recursively
---
src/backend/commands/lockcmds.c | 33 +++++++++++---------------------
src/test/regress/expected/lock.out | 10 +++++++---
src/test/regress/expected/privileges.out | 7 +++++++
src/test/regress/sql/lock.sql | 8 ++++++--
src/test/regress/sql/privileges.sql | 6 ++++++
5 files changed, 37 insertions(+), 27 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..2e54688dff 100644
--- a/src/test/regress/expected/lock.out
+++ b/src/test/regress/expected/lock.out
@@ -138,15 +138,19 @@ 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;
+-- fail when child locked directly
BEGIN;
-LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
+LOCK TABLE lock_tbl2;
ERROR: permission denied for table lock_tbl2
ROLLBACK;
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/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..e50cb6f064 100644
--- a/src/test/regress/sql/lock.sql
+++ b/src/test/regress/sql/lock.sql
@@ -101,10 +101,14 @@ 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;
+-- fail when child locked directly
+BEGIN;
+LOCK TABLE lock_tbl2;
+ROLLBACK;
BEGIN;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
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
