Re: Revised Patch to allow multiple table locks in "Unison"

I have backed out the entire patch.  I think it is best for the author
to make the suggested changes and resubmit.  Reversed patch attached.


> Bruce Momjian  writes:
> > Patch applied.
> 
> Idly looking this over again, I noticed a big OOPS:
> 
> >> ! 		freeList(lockstmt->rellist);
> 
> >> ! 		pfree(relname);
> 
> >> ! 			pfree(relname);
> 
> It is most definitely NOT the executor's business to release pieces of
> the querytree; this will certainly break plpgsql functions, for example,
> where the same querytree is executed repeatedly.
> Bruce, please remove those lines.
> 
> Another thing I am concerned about now that I look more closely is that
> the multi-rel case code opens the relations without any lock, and then
> assumes they'll stick around while it opens and access-checks the rest.
> This will fail if someone else drops one of the rels meanwhile.  I think
> the entire routine should be reduced to a simple loop that opens, locks,
> and closes the rels one at a time.  The extra code bulk to do it this
> way isn't buying us anything at all.
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
Tom Lane wrote:
> 
> Neil Padgett  writes:
> >> This seems fine to me (and in fact I thought we'd already agreed to it).
> 
> > Here's the patch then. =)
> 
> [ quick mental checklist... ]  You forgot the documentation updates.
> Otherwise it looks good.

Ok -- I made a go at the documentation. I'm no SGML wizard, so if
someone could check my changes that would be helpful.

Neil

-- 
Neil Padgett
Red Hat Canada Ltd.                       E-Mail:  npadgett@redhat.com
2323 Yonge Street, Suite #300, 
Toronto, ON  M4P 2C9

Index: doc/src/sgml/ref/lock.sgml
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/doc/src/sgml/ref/lock.sgml,v
retrieving revision 1.24
diff -c -p -r1.24 lock.sgml
*** doc/src/sgml/ref/lock.sgml	2001/07/09 22:18:33	1.24
--- doc/src/sgml/ref/lock.sgml	2001/08/02 21:39:42
*************** Postgres documentation
*** 15,21 ****
     LOCK
    
    
!    Explicitly lock a table inside a transaction
    
   
   
--- 15,21 ----
     LOCK
    
    
!    Explicitly lock a table / tables inside a transaction
    
   
   
*************** Postgres documentation
*** 23,30 ****
     2001-07-09
    
    
! LOCK [ TABLE ] name
! LOCK [ TABLE ] name IN lockmode MODE
  
  where lockmode is one of:
  
--- 23,30 ----
     2001-07-09
    
    
! LOCK [ TABLE ] name [,...]
! LOCK [ TABLE ] name [,...] IN lockmode MODE
  
  where lockmode is one of:
  
*************** ERROR nam
*** 373,378 ****
--- 373,379 ----
       An example for this rule was given previously when discussing the 
       use of SHARE ROW EXCLUSIVE mode rather than SHARE mode.
      
+ 
     
    
  
*************** ERROR nam
*** 383,388 ****
--- 384,395 ----
     
    
  
+   
+    When locking multiple tables, the command LOCK a, b; is equivalent to LOCK
+    a; LOCK b;. The tables are locked one-by-one in the order specified in the
+    LOCK command.
+   
+ 
    
     
      1999-06-08
*************** ERROR nam
*** 406,411 ****
--- 413,419 ----
     
      LOCK works only inside transactions.
     
+ 
    
   
    
Index: src/backend/commands/command.c
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/src/backend/commands/command.c,v
retrieving revision 1.136
diff -c -p -r1.136 command.c
*** src/backend/commands/command.c	2001/07/16 05:06:57	1.136
--- src/backend/commands/command.c	2001/08/02 21:39:43
*************** needs_toast_table(Relation rel)
*** 1984,1991 ****
  		MAXALIGN(data_length);
  	return (tuple_length > TOAST_TUPLE_THRESHOLD);
  }
! 
! 
  /*
   *
   * LOCK TABLE
--- 1984,1990 ----
  		MAXALIGN(data_length);
  	return (tuple_length > TOAST_TUPLE_THRESHOLD);
  }
! 	
  /*
   *
   * LOCK TABLE
*************** needs_toast_table(Relation rel)
*** 1994,2019 ****
  void
  LockTableCommand(LockStmt *lockstmt)
  {
! 	Relation	rel;
! 	int			aclresult;
  
! 	rel = heap_openr(lockstmt->relname, NoLock);
  
! 	if (rel->rd_rel->relkind != RELKIND_RELATION)
! 		elog(ERROR, "LOCK TABLE: %s is not a table", lockstmt->relname);
  
! 	if (lockstmt->mode == AccessShareLock)
! 		aclresult = pg_aclcheck(lockstmt->relname, GetUserId(), ACL_SELECT);
! 	else
! 		aclresult = pg_aclcheck(lockstmt->relname, GetUserId(),
! 								ACL_UPDATE | ACL_DELETE);
  
! 	if (aclresult != ACLCHECK_OK)
! 		elog(ERROR, "LOCK TABLE: permission denied");
  
! 	LockRelation(rel, lockstmt->mode);
  
! 	heap_close(rel, NoLock);	/* close rel, keep lock */
  }
  
  
--- 1993,2096 ----
  void
  LockTableCommand(LockStmt *lockstmt)
  {
! 	int         relCnt;
! 
! 	relCnt = length(lockstmt -> rellist);
! 
! 	/* Handle a single relation lock specially to avoid overhead on likely the
! 	   most common case */
! 
! 	if(relCnt == 1)
! 	{
! 
! 		/* Locking a single table */
! 
! 		Relation	rel;
! 		int			aclresult;
! 		char *relname;
! 
! 		relname = strVal(lfirst(lockstmt->rellist));
! 
! 		freeList(lockstmt->rellist);
! 
! 		rel = heap_openr(relname, NoLock);
! 
! 		if (rel->rd_rel->relkind != RELKIND_RELATION)
! 			elog(ERROR, "LOCK TABLE: %s is not a table", relname);
! 
! 		if (lockstmt->mode == AccessShareLock)
! 			aclresult = pg_aclcheck(relname, GetUserId(),
! 									ACL_SELECT);
! 		else
! 			aclresult = pg_aclcheck(relname, GetUserId(),
! 									ACL_UPDATE | ACL_DELETE);
! 
! 		if (aclresult != ACLCHECK_OK)
! 			elog(ERROR, "LOCK TABLE: permission denied");
! 
! 		LockRelation(rel, lockstmt->mode);
! 
! 		pfree(relname);
! 
! 		heap_close(rel, NoLock);	/* close rel, keep lock */
! 	} 
! 	else 
! 	{
! 		List *p;
! 		Relation *RelationArray;
! 		Relation *pRel;
! 
! 		/* Locking multiple tables */
! 
! 		/* Create an array of relations */
! 
! 		RelationArray = palloc(relCnt * sizeof(Relation));
! 		pRel = RelationArray;
! 
! 		/* Iterate over the list and populate the relation array */
! 
! 		foreach(p, lockstmt->rellist)
! 		{
! 			char* relname = strVal(lfirst(p));
! 			int			aclresult;
! 
! 			*pRel = heap_openr(relname, NoLock);
! 
! 			if ((*pRel)->rd_rel->relkind != RELKIND_RELATION)
! 				elog(ERROR, "LOCK TABLE: %s is not a table", 
! 					 relname);
! 
! 			if (lockstmt->mode == AccessShareLock)
! 				aclresult = pg_aclcheck(relname, GetUserId(),
! 										ACL_SELECT);
! 			else
! 				aclresult = pg_aclcheck(relname, GetUserId(),
! 										ACL_UPDATE | ACL_DELETE);
! 
! 			if (aclresult != ACLCHECK_OK)
! 				elog(ERROR, "LOCK TABLE: permission denied");
  
! 			pRel++;
! 			pfree(relname);
! 		}
  
! 		/* Now, lock all the relations, closing each after it is locked
! 		   (Keeping the locks)
! 		 */
  
! 		for(pRel = RelationArray;
! 			pRel < RelationArray + relCnt;
! 			pRel++)
! 			{
! 				LockRelation(*pRel, lockstmt->mode);
  
! 				heap_close(*pRel, NoLock);
! 			}
  
! 		/* Free the relation array */
  
! 		pfree(RelationArray);
! 	}
  }
  
  
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.148
diff -c -p -r1.148 copyfuncs.c
*** src/backend/nodes/copyfuncs.c	2001/07/16 19:07:37	1.148
--- src/backend/nodes/copyfuncs.c	2001/08/02 21:39:44
*************** _copyLockStmt(LockStmt *from)
*** 2425,2432 ****
  {
  	LockStmt   *newnode = makeNode(LockStmt);
  
! 	if (from->relname)
! 		newnode->relname = pstrdup(from->relname);
  	newnode->mode = from->mode;
  
  	return newnode;
--- 2425,2432 ----
  {
  	LockStmt   *newnode = makeNode(LockStmt);
  
! 	Node_Copy(from, newnode, rellist);
! 	
  	newnode->mode = from->mode;
  
  	return newnode;
Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.96
diff -c -p -r1.96 equalfuncs.c
*** src/backend/nodes/equalfuncs.c	2001/07/16 19:07:38	1.96
--- src/backend/nodes/equalfuncs.c	2001/08/02 21:39:44
*************** _equalDropUserStmt(DropUserStmt *a, Drop
*** 1283,1289 ****
  static bool
  _equalLockStmt(LockStmt *a, LockStmt *b)
  {
! 	if (!equalstr(a->relname, b->relname))
  		return false;
  	if (a->mode != b->mode)
  		return false;
--- 1283,1289 ----
  static bool
  _equalLockStmt(LockStmt *a, LockStmt *b)
  {
! 	if (!equal(a->rellist, b->rellist))
  		return false;
  	if (a->mode != b->mode)
  		return false;
Index: src/backend/parser/gram.y
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.238
diff -c -p -r2.238 gram.y
*** src/backend/parser/gram.y	2001/07/16 19:07:40	2.238
--- src/backend/parser/gram.y	2001/08/02 21:39:46
*************** DeleteStmt:  DELETE FROM relation_expr w
*** 3280,3290 ****
  				}
  		;
  
! LockStmt:	LOCK_P opt_table relation_name opt_lock
  				{
  					LockStmt *n = makeNode(LockStmt);
  
! 					n->relname = $3;
  					n->mode = $4;
  					$$ = (Node *)n;
  				}
--- 3280,3290 ----
  				}
  		;
  
! LockStmt:	LOCK_P opt_table relation_name_list opt_lock
  				{
  					LockStmt *n = makeNode(LockStmt);
  
! 					n->rellist = $3;
  					n->mode = $4;
  					$$ = (Node *)n;
  				}
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.136
diff -c -p -r1.136 parsenodes.h
*** src/include/nodes/parsenodes.h	2001/07/16 19:07:40	1.136
--- src/include/nodes/parsenodes.h	2001/08/02 21:39:46
*************** typedef struct VariableResetStmt
*** 760,766 ****
  typedef struct LockStmt
  {
  	NodeTag		type;
! 	char	   *relname;		/* relation to lock */
  	int			mode;			/* lock mode */
  } LockStmt;
  
--- 760,766 ----
  typedef struct LockStmt
  {
  	NodeTag		type;
! 	List	   *rellist;		/* relations to lock */
  	int			mode;			/* lock mode */
  } LockStmt;
  
Index: src/interfaces/ecpg/preproc/preproc.y
===================================================================
RCS file:
/home/projects/pgsql/cvsroot/pgsql/src/interfaces/ecpg/preproc/preproc.y,v
retrieving revision 1.146
diff -c -p -r1.146 preproc.y
*** src/interfaces/ecpg/preproc/preproc.y	2001/07/16 05:07:00	1.146
--- src/interfaces/ecpg/preproc/preproc.y	2001/08/02 21:39:49
*************** DeleteStmt:  DELETE FROM relation_expr w
*** 2421,2427 ****
  				}
  		;
  
! LockStmt:  LOCK_P opt_table relation_name opt_lock
  				{
  					$$ = cat_str(4, make_str("lock"), $2, $3, $4);
  				}
--- 2421,2427 ----
  				}
  		;
  
! LockStmt:  LOCK_P opt_table relation_name_list opt_lock
  				{
  					$$ = cat_str(4, make_str("lock"), $2, $3, $4);
  				}