Re: [HACKERS] [PATCH] Lockable views
On Thu, 05 Apr 2018 07:53:42 +0900 (JST) Tatsuo Ishii 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 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 ] name [ * ] - 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. @@ -174,6 +174,13 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] +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. + + + LOCK TABLE is useless outside a transaction block: the lock would remain held only to the completion of the statement. Therefore PostgreSQL reports an error if LOCK 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
Re: [HACKERS] [PATCH] Lockable views
>> > > +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. >> > 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? 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
Re: [HACKERS] [PATCH] Lockable views
On Mon, 2 Apr 2018 18:32:53 +0900 Yugo Nagata wrote: > On Thu, 29 Mar 2018 17:26:36 -0700 > Andres Freund wrote: > > Thank you for your comments. I attach a patch to fix issues > you've pointed out. I found a typo in the documentation and attach the updated patch. Regards, > > > Hi, > > > > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote: > > > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml > > > index b2c7203..96d477c 100644 > > > --- a/doc/src/sgml/ref/lock.sgml > > > +++ b/doc/src/sgml/ref/lock.sgml > > > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] > > class="parameter">name [ * ] > > > > > > > > > > > > + 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. > > > + > > > > Trailing space added. I'd remove "specified to be" from the sentence. > > Fixed. > > > > > I think mentioning how this interacts with permissions would be a good > > idea too. Given how operations use the view's owner to recurse, that's > > not obvious. Should also explain what permissions are required to do the > > operation on the view. > > Added a description about permissions into the documentation. > > > > > > > > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid > > > relid, Oid oldrelid, > > > return; /* woops, concurrently > > > dropped; no permissions > > >* check */ > > > > > > - /* Currently, we only allow plain tables to be locked */ > > > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) > > > + > > > > This newline looks spurious to me. > > Removed. > > > > > > > > /* > > > + * Apply LOCK TABLE recursively over a view > > > + * > > > + * All tables and views appearing in the view definition query are locked > > > + * recursively with the same lock mode. > > > + */ > > > + > > > +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. > > > > > > > +static bool > > > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) > > > +{ > > > + if (node == NULL) > > > + return false; > > > + > > > + if (IsA(node, Query)) > > > + { > > > + Query *query = (Query *) node; > > > + ListCell*rtable; > > > + > > > + foreach(rtable, query->rtable) > > > + { > > > + RangeTblEntry *rte = lfirst(rtable); > > > + AclResultaclresult; > > > + > > > + 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. */ > > > + if (relid == context->viewoid && > > > + (!strcmp(rte->eref->aliasname, "old") || > > > !strcmp(rte->eref->aliasname, "new"))) > > > + continue; > > > + > > > + /* Currently, we only allow plain tables or views to be > > > locked. */ > > > + if (relkind != RELKIND_RELATION && relkind != > > > RELKIND_PARTITIONED_TABLE && > > > + relkind != RELKIND_VIEW) > > > + continue; > > > + > > > + /* Check infinite recursion in the view definition. */ > > > + if (relid == context->root_reloid) > > > + ereport(ERROR, > > > + > > > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > > > + errmsg("infinite recursion > > > detected in rules for relation \"%s\"", > > > + > > > get_rel_name(context->root_reloid; > > > > 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; > > This fixed in the attached patch. > > Regards, > > > > > Greetings, > > > > Andres Freund > > > -- > Yugo Nagata -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 96d477c..a225cea
Re: [HACKERS] [PATCH] Lockable views
On Thu, 29 Mar 2018 17:26:36 -0700 Andres Freund wrote: Thank you for your comments. I attach a patch to fix issues you've pointed out. > Hi, > > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote: > > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml > > index b2c7203..96d477c 100644 > > --- a/doc/src/sgml/ref/lock.sgml > > +++ b/doc/src/sgml/ref/lock.sgml > > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] > class="parameter">name [ * ] > > > > > > > > + 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. > > + > > Trailing space added. I'd remove "specified to be" from the sentence. Fixed. > > I think mentioning how this interacts with permissions would be a good > idea too. Given how operations use the view's owner to recurse, that's > not obvious. Should also explain what permissions are required to do the > operation on the view. Added a description about permissions into the documentation. > > > > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid > > relid, Oid oldrelid, > > return; /* woops, concurrently > > dropped; no permissions > > * check */ > > > > - /* Currently, we only allow plain tables to be locked */ > > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) > > + > > This newline looks spurious to me. Removed. > > > > /* > > + * Apply LOCK TABLE recursively over a view > > + * > > + * All tables and views appearing in the view definition query are locked > > + * recursively with the same lock mode. > > + */ > > + > > +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. > > > > +static bool > > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) > > +{ > > + if (node == NULL) > > + return false; > > + > > + if (IsA(node, Query)) > > + { > > + Query *query = (Query *) node; > > + ListCell*rtable; > > + > > + foreach(rtable, query->rtable) > > + { > > + RangeTblEntry *rte = lfirst(rtable); > > + AclResultaclresult; > > + > > + 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. */ > > + if (relid == context->viewoid && > > + (!strcmp(rte->eref->aliasname, "old") || > > !strcmp(rte->eref->aliasname, "new"))) > > + continue; > > + > > + /* Currently, we only allow plain tables or views to be > > locked. */ > > + if (relkind != RELKIND_RELATION && relkind != > > RELKIND_PARTITIONED_TABLE && > > + relkind != RELKIND_VIEW) > > + continue; > > + > > + /* Check infinite recursion in the view definition. */ > > + if (relid == context->root_reloid) > > + ereport(ERROR, > > + > > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > > + errmsg("infinite recursion > > detected in rules for relation \"%s\"", > > + > > get_rel_name(context->root_reloid; > > 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; This fixed in the attached patch. Regards, > > Greetings, > > Andres Freund -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index 96d477c..12303fe 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,8 +46,8 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] - 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 a
Re: [HACKERS] [PATCH] Lockable views
>> The buildfarm is fairly unhappy, and I think it's because of this patch. > > Thanks for the info. Yes, at least prion is unhappy because of the > patch. I will look into this. Done. See if the buildarm becomes happy. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
>> I have just pushed the v10 patch. > > The buildfarm is fairly unhappy, and I think it's because of this patch. Thanks for the info. Yes, at least prion is unhappy because of the patch. I will look into this. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
Tatsuo Ishii writes: > I have just pushed the v10 patch. The buildfarm is fairly unhappy, and I think it's because of this patch. regards, tom lane
Re: [HACKERS] [PATCH] Lockable views
Andres, I have just pushed the v10 patch. Yugo will reply back to your point but I will look into your review as well. Thanks. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp > Hi, > > On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote: >> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml >> index b2c7203..96d477c 100644 >> --- a/doc/src/sgml/ref/lock.sgml >> +++ b/doc/src/sgml/ref/lock.sgml >> @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] > class="parameter">name [ * ] >> >> >> >> + 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. >> + > > Trailing space added. I'd remove "specified to be" from the sentence. > > I think mentioning how this interacts with permissions would be a good > idea too. Given how operations use the view's owner to recurse, that's > not obvious. Should also explain what permissions are required to do the > operation on the view. > > >> @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid >> relid, Oid oldrelid, >> return; /* woops, concurrently >> dropped; no permissions >> * check */ >> >> -/* Currently, we only allow plain tables to be locked */ >> -if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) >> + > > This newline looks spurious to me. > > >> /* >> + * Apply LOCK TABLE recursively over a view >> + * >> + * All tables and views appearing in the view definition query are locked >> + * recursively with the same lock mode. >> + */ >> + >> +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. > > >> +static bool >> +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) >> +{ >> +if (node == NULL) >> +return false; >> + >> +if (IsA(node, Query)) >> +{ >> +Query *query = (Query *) node; >> +ListCell*rtable; >> + >> +foreach(rtable, query->rtable) >> +{ >> +RangeTblEntry *rte = lfirst(rtable); >> +AclResultaclresult; >> + >> +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. */ >> +if (relid == context->viewoid && >> +(!strcmp(rte->eref->aliasname, "old") || >> !strcmp(rte->eref->aliasname, "new"))) >> +continue; >> + >> +/* Currently, we only allow plain tables or views to be >> locked. */ >> +if (relkind != RELKIND_RELATION && relkind != >> RELKIND_PARTITIONED_TABLE && >> +relkind != RELKIND_VIEW) >> +continue; >> + >> +/* Check infinite recursion in the view definition. */ >> +if (relid == context->root_reloid) >> +ereport(ERROR, >> + >> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), >> +errmsg("infinite recursion >> detected in rules for relation \"%s\"", >> + >> get_rel_name(context->root_reloid; > > Hm, how can that happen? And if it can happen, why can it only happen > with the root relation? > > Greetings, > > Andres Freund
Re: [HACKERS] [PATCH] Lockable views
Hi, On 2018-03-28 20:26:48 +0900, Yugo Nagata wrote: > diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml > index b2c7203..96d477c 100644 > --- a/doc/src/sgml/ref/lock.sgml > +++ b/doc/src/sgml/ref/lock.sgml > @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] class="parameter">name [ * ] > > > > + 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. > + Trailing space added. I'd remove "specified to be" from the sentence. I think mentioning how this interacts with permissions would be a good idea too. Given how operations use the view's owner to recurse, that's not obvious. Should also explain what permissions are required to do the operation on the view. > @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid > relid, Oid oldrelid, > return; /* woops, concurrently > dropped; no permissions >* check */ > > - /* Currently, we only allow plain tables to be locked */ > - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) > + This newline looks spurious to me. > /* > + * Apply LOCK TABLE recursively over a view > + * > + * All tables and views appearing in the view definition query are locked > + * recursively with the same lock mode. > + */ > + > +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. > +static bool > +LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) > +{ > + if (node == NULL) > + return false; > + > + if (IsA(node, Query)) > + { > + Query *query = (Query *) node; > + ListCell*rtable; > + > + foreach(rtable, query->rtable) > + { > + RangeTblEntry *rte = lfirst(rtable); > + AclResultaclresult; > + > + 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. */ > + if (relid == context->viewoid && > + (!strcmp(rte->eref->aliasname, "old") || > !strcmp(rte->eref->aliasname, "new"))) > + continue; > + > + /* Currently, we only allow plain tables or views to be > locked. */ > + if (relkind != RELKIND_RELATION && relkind != > RELKIND_PARTITIONED_TABLE && > + relkind != RELKIND_VIEW) > + continue; > + > + /* Check infinite recursion in the view definition. */ > + if (relid == context->root_reloid) > + ereport(ERROR, > + > (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > + errmsg("infinite recursion > detected in rules for relation \"%s\"", > + > get_rel_name(context->root_reloid; Hm, how can that happen? And if it can happen, why can it only happen with the root relation? Greetings, Andres Freund
Re: [HACKERS] [PATCH] Lockable views
> On Wed, 28 Mar 2018 15:45:09 +0900 (JST) > Tatsuo Ishii wrote: > >> >> I found the previous patch was broken and this can't handle >> >> views that has subqueries as bellow; >> >> >> >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; >> >> >> >> I fixed this and attached the updated version including additional tests. >> > >> > This patch gives a warning while compiling: >> > >> > lockcmds.c:186:1: warning: no semicolon at end of struct or union >> > } LockViewRecurse_context; >> > ^ >> >> Also I get a regression test failure: > > Thank you for your reviewing my patch. > I attached the updated patch, v10. Thanks. Looks good to me. I marked the patch as "Ready for Committer". Unless there's an objection, especially from Robert or Thomas Munro, I am going to commit/push it. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Wed, 28 Mar 2018 15:45:09 +0900 (JST) Tatsuo Ishii wrote: > >> I found the previous patch was broken and this can't handle > >> views that has subqueries as bellow; > >> > >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; > >> > >> I fixed this and attached the updated version including additional tests. > > > > This patch gives a warning while compiling: > > > > lockcmds.c:186:1: warning: no semicolon at end of struct or union > > } LockViewRecurse_context; > > ^ > > Also I get a regression test failure: Thank you for your reviewing my patch. I attached the updated patch, v10. Regards, > > *** > /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out > 2018-03-28 15:24:13.805314577 +0900 > --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out > 2018-03-28 15:42:07.975592594 +0900 > *** > *** 118,124 > >lock_tbl1 >lock_view6 > ! (2 rows) > > ROLLBACK; > -- Verify that we can lock a table with inheritance children. > --- 118,125 > >lock_tbl1 >lock_view6 > ! mvtest_tm > ! (3 rows) > > 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 diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..96d477c 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + 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 acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..e8a8877 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -23,11 +23,15 @@ #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" +#include "nodes/nodeFuncs.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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); /* * LOCK TABLE @@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); } @@ -107,7 +115,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +128,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) AclResult aclresult; /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode); + aclresult = LockTableAclCheck(childreloid, lockmode, userid); if (aclresult != ACLCHECK_OK) { char *relname = get_rel_name(childreloid); @@ -157,15 +165,120 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) continue; } -
Re: [HACKERS] [PATCH] Lockable views
>> I found the previous patch was broken and this can't handle >> views that has subqueries as bellow; >> >> CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; >> >> I fixed this and attached the updated version including additional tests. > > This patch gives a warning while compiling: > > lockcmds.c:186:1: warning: no semicolon at end of struct or union > } LockViewRecurse_context; > ^ Also I get a regression test failure: *** /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/lock.out 2018-03-28 15:24:13.805314577 +0900 --- /usr/local/src/pgsql/current/postgresql/src/test/regress/results/lock.out 2018-03-28 15:42:07.975592594 +0900 *** *** 118,124 lock_tbl1 lock_view6 ! (2 rows) ROLLBACK; -- Verify that we can lock a table with inheritance children. --- 118,125 lock_tbl1 lock_view6 ! mvtest_tm ! (3 rows) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
> On Tue, 27 Mar 2018 23:28:04 +0900 > Yugo Nagata wrote: > > I found the previous patch was broken and this can't handle > views that has subqueries as bellow; > > CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; > > I fixed this and attached the updated version including additional tests. This patch gives a warning while compiling: lockcmds.c:186:1: warning: no semicolon at end of struct or union } LockViewRecurse_context; ^ Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Tue, 27 Mar 2018 23:28:04 +0900 Yugo Nagata wrote: I found the previous patch was broken and this can't handle views that has subqueries as bellow; CREATE VIEW lock_view6 AS SELECT * from (select * from lock_tbl1) sub; I fixed this and attached the updated version including additional tests. Regards, > On Tue, 6 Feb 2018 11:12:37 -0500 > Robert Haas wrote: > > > On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii wrote: > > >> But what does that have to do with locking? > > > > > > Well, if the view is not updatable, I think there will be less point > > > to allow to lock the base tables in the view because locking is > > > typically used in a case when updates are required. > > > > > > Of course we could add special triggers to allow to update views that > > > are not automatically updatable but that kind of views are tend to > > > complex and IMO there's less need the automatic view locking feature. > > > > Hmm. Well, I see now why you've designed the feature in the way that > > you have, but I guess it still seems somewhat arbitrary to me. If you > > ignore the deadlock consideration, then there's no reason not to > > define the feature as locking every table mentioned anywhere in the > > query, including subqueries, and it can work for all views whether > > updatable or not. If the deadlock consideration is controlling, then > > I guess we can't do better than what you have, but I'm not sure how > > future-proof it is. If in the future somebody makes views updateable > > that involve a join, say from the primary key of one table to a unique > > key of another so that no duplicate rows can be introduced, then > > they'll either have to write code to make this feature identify and > > lock the "main" table, which I'm not sure would be strong enough in > > all cases, or lock them all, which reintroduces the deadlock problem. > > > > Personally, I would be inclined to view the deadlock problem as not > > very important. I just don't see how that is going to come up very > > I agree that the deadlock won't occur very often and this is not > so important. > > I have updated the lockable-view patch to v8. > > This new version doen't consider the deadlock problem, and all tables > or views appearing in the view definition query are locked recursively. > Also, this allows all kinds of views to be locked even if it is not > auto-updatable view. > > > > often. What I do think will be an issue is that if you start locking > > lots of tables, you might prevent the system from getting much work > > done, whether or not you also cause any deadlocks. But I don't see > > what we can do about that, really. If users want full control over > > which tables get locked, then they have to name them explicitly. Or > > alternatively, maybe they should avoid the need for full-table locks > > by using SSI, gaining the benefits of (1) optimistic rather than > > pessimistic concurrency control, (2) finer-grained locking, and (3) > > not needing to issue explicit LOCK commands. > > > > > -- > > Robert Haas > > EnterpriseDB: http://www.enterprisedb.com > > The Enterprise PostgreSQL Company > > > -- > Yugo Nagata -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..96d477c 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + 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 acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..e15d87d 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -23,11 +23,15 @@ #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" +#include "nodes/nodeFuncs.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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); /* * LOCK TABLE @@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); +
Re: [HACKERS] [PATCH] Lockable views
On Tue, 6 Feb 2018 11:12:37 -0500 Robert Haas wrote: > On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii wrote: > >> But what does that have to do with locking? > > > > Well, if the view is not updatable, I think there will be less point > > to allow to lock the base tables in the view because locking is > > typically used in a case when updates are required. > > > > Of course we could add special triggers to allow to update views that > > are not automatically updatable but that kind of views are tend to > > complex and IMO there's less need the automatic view locking feature. > > Hmm. Well, I see now why you've designed the feature in the way that > you have, but I guess it still seems somewhat arbitrary to me. If you > ignore the deadlock consideration, then there's no reason not to > define the feature as locking every table mentioned anywhere in the > query, including subqueries, and it can work for all views whether > updatable or not. If the deadlock consideration is controlling, then > I guess we can't do better than what you have, but I'm not sure how > future-proof it is. If in the future somebody makes views updateable > that involve a join, say from the primary key of one table to a unique > key of another so that no duplicate rows can be introduced, then > they'll either have to write code to make this feature identify and > lock the "main" table, which I'm not sure would be strong enough in > all cases, or lock them all, which reintroduces the deadlock problem. > > Personally, I would be inclined to view the deadlock problem as not > very important. I just don't see how that is going to come up very I agree that the deadlock won't occur very often and this is not so important. I have updated the lockable-view patch to v8. This new version doen't consider the deadlock problem, and all tables or views appearing in the view definition query are locked recursively. Also, this allows all kinds of views to be locked even if it is not auto-updatable view. > often. What I do think will be an issue is that if you start locking > lots of tables, you might prevent the system from getting much work > done, whether or not you also cause any deadlocks. But I don't see > what we can do about that, really. If users want full control over > which tables get locked, then they have to name them explicitly. Or > alternatively, maybe they should avoid the need for full-table locks > by using SSI, gaining the benefits of (1) optimistic rather than > pessimistic concurrency control, (2) finer-grained locking, and (3) > not needing to issue explicit LOCK commands. > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..96d477c 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,11 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + 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 acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..daf3d81 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -23,11 +23,15 @@ #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" +#include "nodes/nodeFuncs.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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); /* * LOCK TABLE @@ -62,8 +66,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +92,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain
Re: [HACKERS] [PATCH] Lockable views
On Tue, Feb 6, 2018 at 1:28 AM, Tatsuo Ishii wrote: >> But what does that have to do with locking? > > Well, if the view is not updatable, I think there will be less point > to allow to lock the base tables in the view because locking is > typically used in a case when updates are required. > > Of course we could add special triggers to allow to update views that > are not automatically updatable but that kind of views are tend to > complex and IMO there's less need the automatic view locking feature. Hmm. Well, I see now why you've designed the feature in the way that you have, but I guess it still seems somewhat arbitrary to me. If you ignore the deadlock consideration, then there's no reason not to define the feature as locking every table mentioned anywhere in the query, including subqueries, and it can work for all views whether updatable or not. If the deadlock consideration is controlling, then I guess we can't do better than what you have, but I'm not sure how future-proof it is. If in the future somebody makes views updateable that involve a join, say from the primary key of one table to a unique key of another so that no duplicate rows can be introduced, then they'll either have to write code to make this feature identify and lock the "main" table, which I'm not sure would be strong enough in all cases, or lock them all, which reintroduces the deadlock problem. Personally, I would be inclined to view the deadlock problem as not very important. I just don't see how that is going to come up very often. What I do think will be an issue is that if you start locking lots of tables, you might prevent the system from getting much work done, whether or not you also cause any deadlocks. But I don't see what we can do about that, really. If users want full control over which tables get locked, then they have to name them explicitly. Or alternatively, maybe they should avoid the need for full-table locks by using SSI, gaining the benefits of (1) optimistic rather than pessimistic concurrency control, (2) finer-grained locking, and (3) not needing to issue explicit LOCK commands. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
> But what does that have to do with locking? Well, if the view is not updatable, I think there will be less point to allow to lock the base tables in the view because locking is typically used in a case when updates are required. Of course we could add special triggers to allow to update views that are not automatically updatable but that kind of views are tend to complex and IMO there's less need the automatic view locking feature. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Mon, Feb 5, 2018 at 10:49 PM, Tatsuo Ishii wrote: >> Hmm, true. Why exactly are we imposing the restriction to updateable >> views, anyway? > > In my understanding, because of ambiguity to determine which rows in > which base tables needs to be modified by just looking at the DML > against a view. There could be multiple ways to modify the base > tables. But what does that have to do with locking? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
> Hmm, true. Why exactly are we imposing the restriction to updateable > views, anyway? In my understanding, because of ambiguity to determine which rows in which base tables needs to be modified by just looking at the DML against a view. There could be multiple ways to modify the base tables. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Mon, Feb 5, 2018 at 10:26 PM, Tatsuo Ishii wrote: >> True. But the same exact analysis also applies to this definition, >> which contains no subquery: >> >> CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; > > That's not an updatable view, thus cannot be locked according to the > proposed implementation. Hmm, true. Why exactly are we imposing the restriction to updateable views, anyway? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
> True. But the same exact analysis also applies to this definition, > which contains no subquery: > > CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; That's not an updatable view, thus cannot be locked according to the proposed implementation. Anyway do you want to allow to lock all base tables in a view definition if the view is updatable? Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Mon, Feb 5, 2018 at 8:18 PM, Tatsuo Ishii wrote: > We have: CREATE VIEW v1 AS SELECT * FROM t1 WHERE i = (SELECT i FROM t2); > > 1. Session A tries to lock v1 (I suppose it tries to acquire lock in > the order of t1, then t2). A acquires lock on t1 but yet on t2. > > 2. Another session B acquires lock on t2. > > 3. A continues to try to acquire lock on t2 (blocked). > > 4. B tries to acquire lock on t1. Deadlock occurs. True. But the same exact analysis also applies to this definition, which contains no subquery: CREATE VIEW v1 AS SELECT t1.* FROM t1, t2 WHERE t1.i = t2.i; -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
Robert, > I just reread those discussions but I don't see that they really make > any argument for the behavior the patch implements. I see no > explanation on the thread for why locking a table inside of a subquery > is more or less likely to cause deadlock than locking one outside of a > subquery. If we allow to lock a table in a subquery, following could happen: We have: CREATE VIEW v1 AS SELECT * FROM t1 WHERE i = (SELECT i FROM t2); 1. Session A tries to lock v1 (I suppose it tries to acquire lock in the order of t1, then t2). A acquires lock on t1 but yet on t2. 2. Another session B acquires lock on t2. 3. A continues to try to acquire lock on t2 (blocked). 4. B tries to acquire lock on t1. Deadlock occurs. So if a user mixes locking a view and a underlying base table, there's a possibility of deadlocks. If we regard that it is user's responsibility not to mix them or to be careful about the consequence the mixing of locks so that dealocks do not happen, then I would agree that we should lock a table inside a subquery. What do you think? >>> I think that if we >>> change the rules for which subqueries get flattened in a future >>> release, then the behavior will also change. That seems bad. >> >> I doubt it could happen in the future but if that happend we should >> disallow locking on such views. > > That doesn't make any sense to me. When someone migrates from > PostgreSQL 11 to, say, PostgreSQL 14, the view definition is going to > be recreated from an SQL query. Neither the user nor the database > will know whether the query was optimized the same way on both > databases, so how could we disallow locking only those views where > there was a difference on the two releases? Even if we could, how > does that help anything? Throwing an error is just as much a > backward-incompatibility in the command as silently changing what gets > locked. > > But my complaint may have been a little off base all the same -- I > guess we're doing this based on the rewriter output, rather than the > optimizer output, which makes it a lot less likely that we would > decide to change anything here. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Thu, Feb 1, 2018 at 8:09 PM, Tatsuo Ishii wrote: > Initially I thought all base tables including ones in a subquery also > should be locked like you. But after some discussions with Yugo, I > agree that locking table in a subquery is less valuable for users (and > I am afraid it may introduce more deadlock chances). See upthead > discussions. I just reread those discussions but I don't see that they really make any argument for the behavior the patch implements. I see no explanation on the thread for why locking a table inside of a subquery is more or less likely to cause deadlock than locking one outside of a subquery. >> I think that if we >> change the rules for which subqueries get flattened in a future >> release, then the behavior will also change. That seems bad. > > I doubt it could happen in the future but if that happend we should > disallow locking on such views. That doesn't make any sense to me. When someone migrates from PostgreSQL 11 to, say, PostgreSQL 14, the view definition is going to be recreated from an SQL query. Neither the user nor the database will know whether the query was optimized the same way on both databases, so how could we disallow locking only those views where there was a difference on the two releases? Even if we could, how does that help anything? Throwing an error is just as much a backward-incompatibility in the command as silently changing what gets locked. But my complaint may have been a little off base all the same -- I guess we're doing this based on the rewriter output, rather than the optimizer output, which makes it a lot less likely that we would decide to change anything here. >> I also think that this is a bad idea for another reason, which is that >> it leaves us with no syntax to say that you want to lock the view >> itself, and pg_dump wants do that if only we had syntax for it. > > I agree with Yugo and Alvaro. It's better to have a separate syntax > for locking views itself. > > https://www.postgresql.org/message-id/20171226143407.6wjzjn42pt54qskm@alvherre.pgsql Hmm, Alvaro's argument makes sense, I guess. I withdraw this complaint. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
> On Tue, Jan 30, 2018 at 1:21 AM, Thomas Munro > wrote: >> About the idea: it makes some kind of sense to me that we should lock >> the underlying table, in all the same cases that you could do DML on >> the view automatically. I wonder if this is a problem for the >> soundness: "Tables appearing in a subquery are ignored and not >> locked." > > Yeah, that seems like a pretty bad idea. It's exposing what is > basically an implementation detail to users. Initially I thought all base tables including ones in a subquery also should be locked like you. But after some discussions with Yugo, I agree that locking table in a subquery is less valuable for users (and I am afraid it may introduce more deadlock chances). See upthead discussions. > I think that if we > change the rules for which subqueries get flattened in a future > release, then the behavior will also change. That seems bad. I doubt it could happen in the future but if that happend we should disallow locking on such views. > I also think that this is a bad idea for another reason, which is that > it leaves us with no syntax to say that you want to lock the view > itself, and pg_dump wants do that if only we had syntax for it. I agree with Yugo and Alvaro. It's better to have a separate syntax for locking views itself. https://www.postgresql.org/message-id/20171226143407.6wjzjn42pt54qskm@alvherre.pgsql Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Tue, Jan 30, 2018 at 1:21 AM, Thomas Munro wrote: > About the idea: it makes some kind of sense to me that we should lock > the underlying table, in all the same cases that you could do DML on > the view automatically. I wonder if this is a problem for the > soundness: "Tables appearing in a subquery are ignored and not > locked." Yeah, that seems like a pretty bad idea. It's exposing what is basically an implementation detail to users. I think that if we change the rules for which subqueries get flattened in a future release, then the behavior will also change. That seems bad. I also think that this is a bad idea for another reason, which is that it leaves us with no syntax to say that you want to lock the view itself, and pg_dump wants do that if only we had syntax for it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Lockable views
On Thu, 01 Feb 2018 09:48:49 +0900 (JST) Tatsuo Ishii wrote: > > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii wrote: > >> Looks good to me. If there's no objection, especially from Thomas > >> Munro, I will mark this as "ready for committer". > > > > No objection from me. > > I marked this as "Ready for Committer". Thank you for reviewing the patch! Regards, > > 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
Re: [HACKERS] [PATCH] Lockable views
> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii wrote: >> Looks good to me. If there's no objection, especially from Thomas >> Munro, I will mark this as "ready for committer". > > No objection from me. I marked this as "Ready for Committer". Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii wrote: > Looks good to me. If there's no objection, especially from Thomas > Munro, I will mark this as "ready for committer". No objection from me. -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] [PATCH] Lockable views
On Tue, 30 Jan 2018 19:21:04 +1300 Thomas Munro wrote: > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii wrote: > >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression > >>> test as well. > >> > >> Thank you for reviewing the patch. > >> > >> I fixed this and attached a updated patch v6. > > > > Looks good to me. If there's no objection, especially from Thomas > > Munro, I will mark this as "ready for committer". > > About the idea: it makes some kind of sense to me that we should lock > the underlying table, in all the same cases that you could do DML on > the view automatically. I wonder if this is a problem for the > soundness: "Tables appearing in a subquery are ignored and not > locked." I can imagine using this for making backwards-compatible > schema changes, in which case the LOCK-based transaction isolation > techniques might benefit from this behaviour. I'd be interested to > hear about the ideal use case you have in mind. I think the use case is almost similar to that of auto-updatable views. There are some purpose to use views, for example 1) preventing from modifying application due to schema changes, 2) protecting the underlying table from users without proper privileges, 3) making a shorthand of a query with complex WHERE condition. When these are updatable views and users need transaction isolation during updating them, I think the lockable views feature is benefitical because users don't need to refer to the underlying table. Users might don't know the underlying table, or even might not have the privilege to lock this. > About the patch: I didn't study it in detail. It builds, has > documentation and passes all tests. Would it be a good idea to add an > isolation test to show that the underlying relation is actually > locked? Whether the underlying relation is actually locked or not is confirmed in the regression test using pg_locks, so I don't believe that we need to add an isolation test. > Typo: > > + /* Check permissions with the view owner's priviledge. */ > > s/priviledge/privilege/ > > Grammar: > > +/* > + * Check whether the view is lockable. > + * > + * Currently, only auto-updatable views can be locked, that is, > + * views whose definition are simple and that doesn't have > + * instead of rules or triggers are lockable. > > s/definition are simple and that doesn't/definition is simple and that don't/ > s/instead of/INSTEAD OF/ ? Thank you for pointing out these. I attached the fixed patch. Regards, > -- > Thomas Munro > http://www.enterprisedb.com -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..6ddd128 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or + INSTEAD rules are also lockable. When a view is + locked, its base relations are also locked recursively with the same + lock mode. Tables appearing in a subquery are ignored and not locked. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..f6b5962 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we
Re: [HACKERS] [PATCH] Lockable views
On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii wrote: >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression >>> test as well. >> >> Thank you for reviewing the patch. >> >> I fixed this and attached a updated patch v6. > > Looks good to me. If there's no objection, especially from Thomas > Munro, I will mark this as "ready for committer". About the idea: it makes some kind of sense to me that we should lock the underlying table, in all the same cases that you could do DML on the view automatically. I wonder if this is a problem for the soundness: "Tables appearing in a subquery are ignored and not locked." I can imagine using this for making backwards-compatible schema changes, in which case the LOCK-based transaction isolation techniques might benefit from this behaviour. I'd be interested to hear about the ideal use case you have in mind. About the patch: I didn't study it in detail. It builds, has documentation and passes all tests. Would it be a good idea to add an isolation test to show that the underlying relation is actually locked? Typo: + /* Check permissions with the view owner's priviledge. */ s/priviledge/privilege/ Grammar: +/* + * Check whether the view is lockable. + * + * Currently, only auto-updatable views can be locked, that is, + * views whose definition are simple and that doesn't have + * instead of rules or triggers are lockable. s/definition are simple and that doesn't/definition is simple and that don't/ s/instead of/INSTEAD OF/ ? -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] [PATCH] Lockable views
>> You need to DROP VIEW lock_view4 and lock_view5 in the regression >> test as well. > > Thank you for reviewing the patch. > > I fixed this and attached a updated patch v6. Looks good to me. If there's no objection, especially from Thomas Munro, I will mark this as "ready for committer". Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Tue, 30 Jan 2018 13:58:30 +0900 (JST) Tatsuo Ishii wrote: > > Attached is the updated patch v5 including fixing SGML and rebase to HEAD. > > You need to DROP VIEW lock_view4 and lock_view5 in the regression > test as well. Thank you for reviewing the patch. I fixed this and attached a updated patch v6. Regards, > > 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 diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..6ddd128 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or + INSTEAD rules are also lockable. When a view is + locked, its base relations are also locked recursively with the same + lock mode. Tables appearing in a subquery are ignored and not locked. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..7ae3a84 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or auto-updatable views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); } @@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) AclResult aclresult; /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode); + aclresult = LockTableAclCheck(childreloid, lockmode, userid); if (aclresult != ACLCHECK_OK) { char *relname = get_rel_name(childreloid); @@ -157,15 +166,82 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) continue; } - LockTableRecurse(childreloid, lockmode, nowait); + LockTableRecurse(childreloid, lockmode, nowait, userid); } } /* + * Apply LOCK TABLE recursively over a view + */ +static void +LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait) +{ + Relation view; + Query *viewquery; + RangeTblRef *rtr; + RangeTblEntry *base_rte; + Oid baseoid; + AclResult aclresult; + char *relname; + char relkind; + + view = heap_open(reloid, NoLock); + viewquery = get_view_query(view); + + /*
Re: [HACKERS] [PATCH] Lockable views
> Attached is the updated patch v5 including fixing SGML and rebase to HEAD. You need to DROP VIEW lock_view4 and lock_view5 in the regression test as well. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Fri, 26 Jan 2018 21:30:49 +0900 Yugo Nagata wrote: > On Thu, 25 Jan 2018 20:51:41 +1300 > Thomas Munro wrote: > > > On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata wrote: > > > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > > > Tatsuo Ishii wrote: > > >> Your addition to the doc: > > >> + Automatically updatable views (see ) > > >> + that do not have INSTEAD OF triggers or > > >> INSTEAD > > >> + rules are also lockable. When a view is locked, its base relations > > >> are > > >> + also locked recursively with the same lock mode. > > > > > > I added this point to the documentation. > > > > + Automatically updatable views (see ) > > + that do not have INSTEAD OF triggers or INSTEAD > > Thank you for pointing out this. > > Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix > them together and update the patch. Attached is the updated patch v5 including fixing SGML and rebase to HEAD. Regards, > > > > > Tthe documentation doesn't build: you now need to say > > instead of , and you need to say . > > > > -- > > Thomas Munro > > http://www.enterprisedb.com > > > > > -- > Yugo Nagata > -- Yugo Nagata diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..6ddd128 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or + INSTEAD rules are also lockable. When a view is + locked, its base relations are also locked recursively with the same + lock mode. Tables appearing in a subquery are ignored and not locked. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 6479dcb..7ae3a84 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or auto-updatable views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relid)), rv->relname); } @@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) AclResult aclresult; /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode); + aclresult = LockTableAclCheck(childreloid, lockmode, userid); if (aclresult != ACLCHEC
Re: [HACKERS] [PATCH] Lockable views
On Thu, 25 Jan 2018 20:51:41 +1300 Thomas Munro wrote: > On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata wrote: > > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > > Tatsuo Ishii wrote: > >> Your addition to the doc: > >> + Automatically updatable views (see ) > >> + that do not have INSTEAD OF triggers or INSTEAD > >> + rules are also lockable. When a view is locked, its base relations are > >> + also locked recursively with the same lock mode. > > > > I added this point to the documentation. > > + Automatically updatable views (see ) > + that do not have INSTEAD OF triggers or INSTEAD Thank you for pointing out this. Also, I've noticed this patch cannot be applied to the HEAD, so I'll fix them together and update the patch. > > Tthe documentation doesn't build: you now need to say > instead of , and you need to say . > > -- > Thomas Munro > http://www.enterprisedb.com > -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
On Sun, Dec 31, 2017 at 11:57 PM, Yugo Nagata wrote: > On Fri, 29 Dec 2017 23:39:39 +0900 (JST) > Tatsuo Ishii wrote: >> Your addition to the doc: >> + Automatically updatable views (see ) >> + that do not have INSTEAD OF triggers or INSTEAD >> + rules are also lockable. When a view is locked, its base relations are >> + also locked recursively with the same lock mode. > > I added this point to the documentation. + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or INSTEAD Tthe documentation doesn't build: you now need to say instead of , and you need to say . -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] [PATCH] Lockable views
Hi, The updated patch is attached. On Fri, 29 Dec 2017 23:39:39 +0900 (JST) Tatsuo Ishii wrote: > The patch produces a warning. > > /home/t-ishii/lock_view-v3.patch:542: trailing whitespace. > -- Verify that we can lock a auto-updatable views > warning: 1 line adds whitespace errors. Fixed. > > Your addition to the doc: > + Automatically updatable views (see ) > + that do not have INSTEAD OF triggers or INSTEAD > + rules are also lockable. When a view is locked, its base relations are > + also locked recursively with the same lock mode. > > does not mention about the point: > > >> >> > 1) Leave as it is (ignore tables appearing in a subquery) I added this point to the documentation. Regards, > > 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 diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..c2ed481 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or INSTEAD + rules are also lockable. When a view is locked, its base relations are + also locked recursively with the same lock mode. Tables appearing in a + subquery are ignored and not locked. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 9fe9e02..80e00f7 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or auto-updatable views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname); } @@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) AclResult aclresult; /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode); + aclresult = LockTableAclCheck(childreloid, lockmode, userid); if (aclresult != ACLCHECK_OK) { char *relname = get_rel_name(childreloid); @@ -157,15 +166,82 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) continue; } - LockTableRecurse(childreloid, lockmode, nowait); + LockTableRecurse(childreloid, lockmode, nowait, userid); } } /* + * Apply LOCK TABLE
Re: [HACKERS] [PATCH] Lockable views
>> >> > 1) Leave as it is (ignore tables appearing in a subquery) >> >> > >> >> > 2) Lock all tables including in a subquery >> >> > >> >> > 3) Check subquery in the view >> > >> >> > So it seem #1 is the most reasonable way to deal with the problem >> >> > assuming that it's user's responsibility to take appropriate locks on >> >> > the tables in the subquery. >> > >> > I adopted #1 and I didn't change anything about this. >> >> Looks good to me except that the patch lacks documents and the >> regression test needs more cases. For example, it needs a test for the >> case #1 above: probably using pg_locks to make sure that the tables >> appearing in the subquery do not hold locks. > > Attached is the update patch, v3. I add some regression test and > the documentation. The patch produces a warning. /home/t-ishii/lock_view-v3.patch:542: trailing whitespace. -- Verify that we can lock a auto-updatable views warning: 1 line adds whitespace errors. Your addition to the doc: + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or INSTEAD + rules are also lockable. When a view is locked, its base relations are + also locked recursively with the same lock mode. does not mention about the point: >> >> > 1) Leave as it is (ignore tables appearing in a subquery) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
On Thu, 28 Dec 2017 09:29:11 +0900 (JST) Tatsuo Ishii wrote: > > I didn't want to change the interface of view_query_is_auto_updatable() > > because this might be called from other third-patry software, so I renamed > > this function to view_query_is_auto_updatable_or_lockable() and added the > > flag > > to this. I created view_query_is_auto_updatable() as a wrapper of this > > function. > > I also made view_query_is_lockable() that returns a other message than > > view_query_is_auto_updatable(). > > > >> On Tue, 17 Oct 2017 11:59:05 +0900 (JST) > >> Tatsuo Ishii wrote: > >> > 1) Leave as it is (ignore tables appearing in a subquery) > >> > > >> > 2) Lock all tables including in a subquery > >> > > >> > 3) Check subquery in the view > > > >> > So it seem #1 is the most reasonable way to deal with the problem > >> > assuming that it's user's responsibility to take appropriate locks on > >> > the tables in the subquery. > > > > I adopted #1 and I didn't change anything about this. > > Looks good to me except that the patch lacks documents and the > regression test needs more cases. For example, it needs a test for the > case #1 above: probably using pg_locks to make sure that the tables > appearing in the subquery do not hold locks. Attached is the update patch, v3. I add some regression test and the documentation. > > 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 diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b2c7203..986ae71 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,13 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or INSTEAD + rules are also lockable. When a view is locked, its base relations are + also locked recursively with the same lock mode. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 9fe9e02..80e00f7 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or auto-updatable views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname); } @@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE l
Re: [HACKERS] [PATCH] Lockable views
> I didn't want to change the interface of view_query_is_auto_updatable() > because this might be called from other third-patry software, so I renamed > this function to view_query_is_auto_updatable_or_lockable() and added the flag > to this. I created view_query_is_auto_updatable() as a wrapper of this > function. > I also made view_query_is_lockable() that returns a other message than > view_query_is_auto_updatable(). > >> On Tue, 17 Oct 2017 11:59:05 +0900 (JST) >> Tatsuo Ishii wrote: >> > 1) Leave as it is (ignore tables appearing in a subquery) >> > >> > 2) Lock all tables including in a subquery >> > >> > 3) Check subquery in the view > >> > So it seem #1 is the most reasonable way to deal with the problem >> > assuming that it's user's responsibility to take appropriate locks on >> > the tables in the subquery. > > I adopted #1 and I didn't change anything about this. Looks good to me except that the patch lacks documents and the regression test needs more cases. For example, it needs a test for the case #1 above: probably using pg_locks to make sure that the tables appearing in the subquery do not hold locks. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: [HACKERS] [PATCH] Lockable views
Hi, Attached is the updated patch. On Mon, 16 Oct 2017 10:07:48 +0900 (JST) Tatsuo Ishii wrote: > >> > It would be nice if the message would be something like: > >> > > >> > DETAIL: Views that return aggregate functions are not lockable > You could add a flag to view_query_is_auto_updatable() to switch the > message between > > DETAIL: Views that return aggregate functions are not automatically > updatable. > > and > > DETAIL: Views that return aggregate functions are not lockable I didn't want to change the interface of view_query_is_auto_updatable() because this might be called from other third-patry software, so I renamed this function to view_query_is_auto_updatable_or_lockable() and added the flag to this. I created view_query_is_auto_updatable() as a wrapper of this function. I also made view_query_is_lockable() that returns a other message than view_query_is_auto_updatable(). > On Tue, 17 Oct 2017 11:59:05 +0900 (JST) > Tatsuo Ishii wrote: > > 1) Leave as it is (ignore tables appearing in a subquery) > > > > 2) Lock all tables including in a subquery > > > > 3) Check subquery in the view > > So it seem #1 is the most reasonable way to deal with the problem > > assuming that it's user's responsibility to take appropriate locks on > > the tables in the subquery. I adopted #1 and I didn't change anything about this. Regards, -- Yugo Nagata diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 9fe9e02..80e00f7 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +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 LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) &lockstmt->mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, return; /* woops, concurrently dropped; no permissions * check */ - /* Currently, we only allow plain tables to be locked */ - if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE) + + /* Currently, we only allow plain tables or auto-updatable views to be locked */ + if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE && + relkind != RELKIND_VIEW) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a table", + errmsg("\"%s\" is not a table or view", rv->relname))); /* Check permissions. */ - aclresult = LockTableAclCheck(relid, lockmode); + aclresult = LockTableAclCheck(relid, lockmode, GetUserId()); if (aclresult != ACLCHECK_OK) aclcheck_error(aclresult, ACL_KIND_CLASS, rv->relname); } @@ -107,7 +116,7 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, * multiply-inheriting children more than once, but that's no problem. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) { List *children; ListCell *lc; @@ -120,7 +129,7 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) AclResult aclresult; /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode); + aclresult = LockTableAclCheck(childreloid, lockmode, userid); if (aclresult != ACLCHECK_OK) { char *relname = get_rel_name(childreloid); @@ -157,15 +166,82 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) continue; } - LockTableRecurse(childreloid, lockmode, nowait); + LockTableRecurse(childreloid, lockmode, nowait, userid); } } /* + * Apply LOCK TABLE recursively over a view + */ +static void +LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait) +{ + Relation view; + Query *viewquery; + RangeTblRef
Re: [HACKERS] [PATCH] Lockable views
On Tue, 26 Dec 2017 22:22:33 +0900 Michael Paquier wrote: > On Tue, Dec 26, 2017 at 06:37:06PM +0900, Yugo Nagata wrote: > > I have created a new entry in CF-2017-1 and registered this thread again. > > Fine for me. Thanks for the update. And I guess that you are planning to > send a new version before the beginning of the next commit fest using > the feedback provided? Yes. I'll update the patch according to Ishii-san's comments. > -- > Michael -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
Yugo Nagata wrote: > On Fri, 27 Oct 2017 07:11:14 +0200 > Robert Haas wrote: > > > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata wrote: > > > In the attached patch, only automatically-updatable views that do not have > > > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > > > those views definition have only one base-relation. When an auto-updatable > > > view is locked, its base relation is also locked. If the base relation is > > > a > > > view again, base relations are processed recursively. For locking a view, > > > the view owner have to have he priviledge to lock the base relation. > > > > Why is this the right behavior? > > > > I would have expected LOCK TABLE v to lock the view and nothing else. > This discussion is one about 7 years ago when automatically-updatable views > are not supported. Since 9.3, simple views can be updated as well as tables, > so now I think it is reasonable that LOCK TABLE for views locks their base > tables. I agree with Yugo Nagata -- LOCK TABLE is in some cases necessary to provide the right isolation so that an operation can be carried out without interference from other processes that want to process the same data -- and if a view is provided on top of existing tables, preventing concurrent changes to the data returned by the view is done by locking the view and recursively the tables that the view are built on, as if the view were a table. This is why LOCK TABLE is the right command to do it. Also, if an application is designed using a table and concurrent changes are prevented via LOCK TABLE, then when the underlying schema is changed and the table is replaced by a view, the application continues to work unchanged; not only syntactically (no error because of table-locking a view) but also semantically because new application code that modifies data in underlying tables from paths other than the view will need to compete with those through the view, which is correct. > > See > > http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com > > for previous discussion of this topic. > If we want to lock only the view, it seems to me that LOCK VIEW syntax is > good. > However, to realize this, changing the syntax to avoid a shift/reduce > conflict will be needed as disucussed in the "LOCK for non-tables" thread. +1 on making TABLE mandatory in LOCK [TABLE], since that will support this new LOCK VIEW thing as well as locking other object types. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] Lockable views
On Tue, Dec 26, 2017 at 06:37:06PM +0900, Yugo Nagata wrote: > I have created a new entry in CF-2017-1 and registered this thread again. Fine for me. Thanks for the update. And I guess that you are planning to send a new version before the beginning of the next commit fest using the feedback provided? -- Michael signature.asc Description: PGP signature
Re: [HACKERS] [PATCH] Lockable views
On Sat, 23 Dec 2017 09:44:30 +0900 Michael Paquier wrote: > On Fri, Dec 22, 2017 at 04:19:46PM +0900, Yugo Nagata wrote: > > I was busy for and I could not work on this patch. After reading the > > previous discussion, I still think the behavior of this patch would > > be right. So, I would like to reregister to CF 2018-1. Do I need to > > create a new entry on CF? or should I change the status to > > "Moved to next CF"? > > This is entirely up to you. It may make sense as well to spawn a new thread > and mark the new patch set as v2, based on the feedback received for v1, as > well as it could make sense to continue with this thread. If the move involves > a complete patch rewrite with a rather different concept, a new thread is more > adapted in my opinion. Thank you for your comment. I have created a new entry in CF-2017-1 and registered this thread again. > -- > Michael -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
On Fri, Dec 22, 2017 at 04:19:46PM +0900, Yugo Nagata wrote: > I was busy for and I could not work on this patch. After reading the > previous discussion, I still think the behavior of this patch would > be right. So, I would like to reregister to CF 2018-1. Do I need to > create a new entry on CF? or should I change the status to > "Moved to next CF"? This is entirely up to you. It may make sense as well to spawn a new thread and mark the new patch set as v2, based on the feedback received for v1, as well as it could make sense to continue with this thread. If the move involves a complete patch rewrite with a rather different concept, a new thread is more adapted in my opinion. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] [PATCH] Lockable views
On Wed, 29 Nov 2017 11:29:36 +0900 Michael Paquier wrote: > On Fri, Oct 27, 2017 at 2:11 PM, Robert Haas wrote: > > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata wrote: > >> In the attached patch, only automatically-updatable views that do not have > >> INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > >> those views definition have only one base-relation. When an auto-updatable > >> view is locked, its base relation is also locked. If the base relation is a > >> view again, base relations are processed recursively. For locking a view, > >> the view owner have to have he priviledge to lock the base relation. > > > > Why is this the right behavior? > > > > I would have expected LOCK TABLE v to lock the view and nothing else. > > > > See > > http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com > > for previous discussion of this topic. > > That's what I would expect as well.. But I may be missing something. I > am marking the patch as returned with feedback as this has not been > replied in one month. I was busy for and I could not work on this patch. After reading the previous discussion, I still think the behavior of this patch would be right. So, I would like to reregister to CF 2018-1. Do I need to create a new entry on CF? or should I change the status to "Moved to next CF"? > -- > Michael > -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
On Fri, 27 Oct 2017 07:11:14 +0200 Robert Haas wrote: > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata wrote: > > In the attached patch, only automatically-updatable views that do not have > > INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that > > those views definition have only one base-relation. When an auto-updatable > > view is locked, its base relation is also locked. If the base relation is a > > view again, base relations are processed recursively. For locking a view, > > the view owner have to have he priviledge to lock the base relation. > > Why is this the right behavior? > > I would have expected LOCK TABLE v to lock the view and nothing else. > > See > http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com > for previous discussion of this topic. This discussion is one about 7 years ago when automatically-updatable views are not supported. Since 9.3, simple views can be updated as well as tables, so now I think it is reasonable that LOCK TABLE for views locks their base tables. If we want to lock only the view, it seems to me that LOCK VIEW syntax is good. However, to realize this, changing the syntax to avoid a shift/reduce conflict will be needed as disucussed in the "LOCK for non-tables" thread. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
On Tue, 17 Oct 2017 11:59:05 +0900 (JST) Tatsuo Ishii wrote: > > I'm a bit confused. What is difference between tables and functions > > in a subquery with regard to view locking? I think also none view queries > > using a subquery do not care about the changes of tables in the > > subquery while executing the query. I might be misnderstanding > > the problem you mentioned. > > The difference is in the function cases we concern the function > definition. While the table cases need to care about table > definitions *and* contents of the table. > > If we are changing the table definition, AccessExclusiveLock will be > held for the table and the updation will be blocked anyway. So we > don't need to care about the table definition changes. > > On the other hand, table contents changes need to be cared because no > automatic locking are held in some cases. I think whether tables in > the subquery need locking or not is depending on use cases. > > So I dug into the previous candidates a little bit more: > > 1) Leave as it is (ignore tables appearing in a subquery) > > 2) Lock all tables including in a subquery > > 3) Check subquery in the view definition. If there are some tables >involved, emit an error and abort. > > I think one of the problems with #2 is, we will lock tables involved > by the view in random order, which could cause unwanted dead > locks. This is not good and I cannot see any easy way to avoid > this. Also some tables may not need to be locked. > > Problem with #3 is, it does not help a user who wants to control > lockings by himself/herself. > > So it seem #1 is the most reasonable way to deal with the problem > assuming that it's user's responsibility to take appropriate locks on > the tables in the subquery. Thank you for your response. I agree to adopt #1. > > > BTW, I found that if we have to handle subqueries in where clause, we would > > also have to care about subqueries in target list... The view defined as > > below is also updatable. > > > > =# create view v7 as select (select * from tbl2 limit 1) from tbl; > > The view is not updatable. You will get something like if you try to update > v7: > > DETAIL: Views that have no updatable columns are not automatically updatable. Although you can not insert into or update v7, you can delete tuples from v7 since it just delete tuples from table tbl regardless with any column. However, as disussed above, if it is user's responsibility to take appropriate locks on the tables in subqueries in the target list, we don't need to care about these. > > On the other hand this: > > create view v7 as select i, (select j from tbl2 limit 1) from tbl; > > will be updatable. In this case column j of v7 will never be > updatable. And you should do something like: > > insert into v7(i) values... > > In short, you don't need to care about a subquery appearing in the TLE > as far as the view locking concerns. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp > > > -- > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata
Re: [HACKERS] [PATCH] Lockable views
On Fri, Oct 27, 2017 at 2:11 PM, Robert Haas wrote: > On Wed, Oct 11, 2017 at 11:36 AM, Yugo Nagata wrote: >> In the attached patch, only automatically-updatable views that do not have >> INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that >> those views definition have only one base-relation. When an auto-updatable >> view is locked, its base relation is also locked. If the base relation is a >> view again, base relations are processed recursively. For locking a view, >> the view owner have to have he priviledge to lock the base relation. > > Why is this the right behavior? > > I would have expected LOCK TABLE v to lock the view and nothing else. > > See > http://postgr.es/m/AANLkTi=kupesjhrdevgfbt30au_iyro6zwk+fwwy_...@mail.gmail.com > for previous discussion of this topic. That's what I would expect as well.. But I may be missing something. I am marking the patch as returned with feedback as this has not been replied in one month. -- Michael