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 ] <replaceable >> class="parameter">name</replaceable> [ * ] >> </para> >> >> <para> >> + When a view is specified to be locked, all relations appearing in the >> view >> + definition query are also locked recursively with the same lock mode. >> + </para> > > 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); >> + AclResult aclresult; >> + >> + 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
