Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

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 *) >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		 

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

>> > > +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

[Yugo Nagata]

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 

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

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 

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

>> 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

[Tom Lane]

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

[Tatsuo Ishii]

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

[Andres Freund]

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

[Tatsuo Ishii]

> 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

[Yugo Nagata]

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 *) >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 

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

>> 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

[Tatsuo Ishii]

> 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

[Yugo Nagata]

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 *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if 

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

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 *) >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 != 

Re: [HACKERS] [PATCH] Lockable views

[Robert Haas]

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

[Robert Haas]

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

[Tatsuo Ishii]

> 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

[Robert Haas]

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

[Tatsuo Ishii]

> 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

[Robert Haas]

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

[Tatsuo Ishii]

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

[Robert Haas]

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

[Tatsuo Ishii]

> 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

[Robert Haas]

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

[Yugo Nagata]

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

[Tatsuo Ishii]

> 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

[Thomas Munro]

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

[Yugo Nagata]

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 *) >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 

Re: [HACKERS] [PATCH] Lockable views

[Thomas Munro]

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

[Tatsuo Ishii]

>> 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

[Yugo Nagata]

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 *) >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);
+	

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

> 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

[Yugo Nagata]

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 *) >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 = 

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

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

[Thomas Munro]

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

[Yugo Nagata]

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 *) >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);
 	

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

>> >> > 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

[Yugo Nagata]

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 *) >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;
@@ 

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

> 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

[Yugo Nagata]

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 *) >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)
+{
+	

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

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

[Michael Paquier]

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

[Yugo Nagata]

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

[Michael Paquier]

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

[Yugo Nagata]

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

[Yugo Nagata]

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