On Thu, 05 Apr 2018 07:53:42 +0900 (JST)
Tatsuo Ishii <[email protected]> wrote:
I update the patch to fix the lockable view issues.
> >> > > +typedef struct
> >> > > +{
> >> > > + Oid root_reloid;
> >> > > + LOCKMODE lockmode;
> >> > > + bool nowait;
> >> > > + Oid viewowner;
> >> > > + Oid viewoid;
> >> > > +} LockViewRecurse_context;
> >> >
> >> > Probably wouldn't hurt to pgindent the larger changes in the patch.
>
> Yeah. Also, each struct member needs a comment.
I applied pgindent and added comments to struct members.
>
> >> > Hm, how can that happen? And if it can happen, why can it only happen
> >> > with the root relation?
> >>
> >> For example, the following queries cause the infinite recursion of views.
> >> This is detected and the error is raised.
> >>
> >> create table t (i int);
> >> create view v1 as select 1;
> >> create view v2 as select * from v1;
> >> create or replace view v1 as select * from v2;
> >> begin;
> >> lock v1;
> >> abort;
> >>
> >> However, I found that the previous patch could not handle the following
> >> situation in which the root relation itself doesn't have infinite
> >> recursion.
> >>
> >> create view v3 as select * from v1;
> >> begin;
> >> lock v3;
> >> abort;
>
> Shouldn't they be in the regression test?
Added tests for the infinite recursion detection.
Regards,
>
> It's shame that create_view test does not include the cases, but it's
> another story.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
--
Yugo Nagata <[email protected]>
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index 96d477c..a225cea 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,8 +46,8 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
</para>
<para>
- When a view is specified to be locked, all relations appearing in the view
- definition query are also locked recursively with the same lock mode.
+ When a view is locked, all relations appearing in the view definition
+ query are also locked recursively with the same lock mode.
</para>
<para>
@@ -174,6 +174,13 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
</para>
<para>
+ The user performing the lock on the view must have the corresponding privilege
+ on the view. In addition the view's owner must have the relevant privileges on
+ the underlying base relations, but the user performing the lock does
+ not need any permissions on the underlying base relations.
+ </para>
+
+ <para>
<command>LOCK TABLE</command> is useless outside a transaction block: the lock
would remain held only to the completion of the statement. Therefore
<productname>PostgreSQL</productname> reports an error if <command>LOCK</command>
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index b247c0f..5e0ef48 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -31,7 +31,7 @@ static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid use
static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
-static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, List *ancestor_views);
/*
* LOCK TABLE
@@ -67,7 +67,7 @@ LockTableCommand(LockStmt *lockstmt)
(void *) &lockstmt->mode);
if (get_rel_relkind(reloid) == RELKIND_VIEW)
- LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+ LockViewRecurse(reloid, lockstmt->mode, lockstmt->nowait, NIL);
else if (recurse)
LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
}
@@ -92,7 +92,6 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
return; /* woops, concurrently dropped; no permissions
* check */
-
/* Currently, we only allow plain tables or views to be locked */
if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
relkind != RELKIND_VIEW)
@@ -178,11 +177,11 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
typedef struct
{
- Oid root_reloid;
- LOCKMODE lockmode;
- bool nowait;
- Oid viewowner;
- Oid viewoid;
+ LOCKMODE lockmode; /* lock mode to use */
+ bool nowait; /* no wait mode */
+ Oid viewowner; /* view owner for checking the privilege */
+ Oid viewoid; /* OID of the view to be locked */
+ List *ancestor_views; /* OIDs of ancestor views */
} LockViewRecurse_context;
static bool
@@ -193,19 +192,22 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
if (IsA(node, Query))
{
- Query *query = (Query *) node;
- ListCell *rtable;
+ Query *query = (Query *) node;
+ ListCell *rtable;
foreach(rtable, query->rtable)
{
- RangeTblEntry *rte = lfirst(rtable);
- AclResult aclresult;
+ RangeTblEntry *rte = lfirst(rtable);
+ AclResult aclresult;
- Oid relid = rte->relid;
- char relkind = rte->relkind;
- char *relname = get_rel_name(relid);
+ Oid relid = rte->relid;
+ char relkind = rte->relkind;
+ char *relname = get_rel_name(relid);
- /* The OLD and NEW placeholder entries in the view's rtable are skipped. */
+ /*
+ * The OLD and NEW placeholder entries in the view's rtable are
+ * skipped.
+ */
if (relid == context->viewoid &&
(!strcmp(rte->eref->aliasname, "old") || !strcmp(rte->eref->aliasname, "new")))
continue;
@@ -216,11 +218,11 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
continue;
/* Check infinite recursion in the view definition. */
- if (relid == context->root_reloid)
+ if (list_member_oid(context->ancestor_views, relid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("infinite recursion detected in rules for relation \"%s\"",
- get_rel_name(context->root_reloid))));
+ errmsg("infinite recursion detected in rules for relation \"%s\"",
+ get_rel_name(relid))));
/* Check permissions with the view owner's privilege. */
aclresult = LockTableAclCheck(relid, context->lockmode, context->viewowner);
@@ -233,11 +235,11 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
else if (!ConditionalLockRelationOid(relid, context->lockmode))
ereport(ERROR,
(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
- errmsg("could not obtain lock on relation \"%s\"",
+ errmsg("could not obtain lock on relation \"%s\"",
relname)));
if (relkind == RELKIND_VIEW)
- LockViewRecurse(relid, context->root_reloid, context->lockmode, context->nowait);
+ LockViewRecurse(relid, context->lockmode, context->nowait, context->ancestor_views);
else if (rte->inh)
LockTableRecurse(relid, context->lockmode, context->nowait, context->viewowner);
}
@@ -254,24 +256,26 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
}
static void
-LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait)
+LockViewRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, List *ancestor_views)
{
LockViewRecurse_context context;
- Relation view;
- Query *viewquery;
+ Relation view;
+ Query *viewquery;
view = heap_open(reloid, NoLock);
viewquery = get_view_query(view);
- context.root_reloid = root_reloid;
context.lockmode = lockmode;
context.nowait = nowait;
context.viewowner = view->rd_rel->relowner;
context.viewoid = reloid;
+ context.ancestor_views = lcons_oid(reloid, ancestor_views);
LockViewRecurse_walker((Node *) viewquery, &context);
+ ancestor_views = list_delete_oid(ancestor_views, reloid);
+
heap_close(view, NoLock);
}
diff --git a/src/test/regress/expected/lock.out b/src/test/regress/expected/lock.out
index 964e6f2..185fd2f 100644
--- a/src/test/regress/expected/lock.out
+++ b/src/test/regress/expected/lock.out
@@ -121,6 +121,17 @@ select relname from pg_locks l, pg_class c
(2 rows)
ROLLBACK;
+-- detecting infinite recursions in view definitions
+CREATE OR REPLACE VIEW lock_view2 AS SELECT * from lock_view3;
+BEGIN TRANSACTION;
+LOCK TABLE lock_view2 IN EXCLUSIVE MODE;
+ERROR: infinite recursion detected in rules for relation "lock_view2"
+ROLLBACK;
+CREATE VIEW lock_view7 AS SELECT * from lock_view2;
+BEGIN TRANSACTION;
+LOCK TABLE lock_view7 IN EXCLUSIVE MODE;
+ERROR: infinite recursion detected in rules for relation "lock_view2"
+ROLLBACK;
-- Verify that we can lock a table with inheritance children.
CREATE TABLE lock_tbl2 (b BIGINT) INHERITS (lock_tbl1);
CREATE TABLE lock_tbl3 () INHERITS (lock_tbl2);
@@ -142,11 +153,12 @@ RESET ROLE;
--
-- Clean up
--
+DROP VIEW lock_view7;
DROP VIEW lock_view6;
DROP VIEW lock_view5;
DROP VIEW lock_view4;
-DROP VIEW lock_view3;
-DROP VIEW lock_view2;
+DROP VIEW lock_view3 CASCADE;
+NOTICE: drop cascades to view lock_view2
DROP VIEW lock_view1;
DROP TABLE lock_tbl3;
DROP TABLE lock_tbl2;
diff --git a/src/test/regress/sql/lock.sql b/src/test/regress/sql/lock.sql
index e22c9e2..26a7e59 100644
--- a/src/test/regress/sql/lock.sql
+++ b/src/test/regress/sql/lock.sql
@@ -84,6 +84,15 @@ select relname from pg_locks l, pg_class c
where l.relation = c.oid and relname like '%lock_%' and mode = 'ExclusiveLock'
order by relname;
ROLLBACK;
+-- detecting infinite recursions in view definitions
+CREATE OR REPLACE VIEW lock_view2 AS SELECT * from lock_view3;
+BEGIN TRANSACTION;
+LOCK TABLE lock_view2 IN EXCLUSIVE MODE;
+ROLLBACK;
+CREATE VIEW lock_view7 AS SELECT * from lock_view2;
+BEGIN TRANSACTION;
+LOCK TABLE lock_view7 IN EXCLUSIVE MODE;
+ROLLBACK;
-- Verify that we can lock a table with inheritance children.
CREATE TABLE lock_tbl2 (b BIGINT) INHERITS (lock_tbl1);
@@ -107,11 +116,11 @@ RESET ROLE;
--
-- Clean up
--
+DROP VIEW lock_view7;
DROP VIEW lock_view6;
DROP VIEW lock_view5;
DROP VIEW lock_view4;
-DROP VIEW lock_view3;
-DROP VIEW lock_view2;
+DROP VIEW lock_view3 CASCADE;
DROP VIEW lock_view1;
DROP TABLE lock_tbl3;
DROP TABLE lock_tbl2;
