Re: [HACKERS] [PATCH] Lockable views

On Mon, 2 Apr 2018 18:32:53 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> On Thu, 29 Mar 2018 17:26:36 -0700
> Andres Freund <andres@anarazel.de> 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 ] <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.
> 
> 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);
> > > +			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?
> 
> 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 <nagata@sraoss.co.jp>


-- 
Yugo Nagata <nagata@sraoss.co.jp>