Re: [PATCHES] too many variants of relation_open

[Heikki Linnakangas]

Jaime Casanova wrote:
 I can understand why we have relation_openrv and try_relation_open,
 but relation_open_nowait can be merged with relation_open.

Well, yes it could, but why? Keeping them separate looks slightly more
readable to me, and the change could break a lot of external modules for
no good reason.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

[PATCHES] too many variants of relation_open

[Jaime Casanova]

Hi,

I can understand why we have relation_openrv and try_relation_open,
but relation_open_nowait can be merged with relation_open.

Or there is something i'm missing? attached is a patch that do the merge.

-- 
regards,
Jaime Casanova

Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning.
   Richard Cook
Index: contrib/dblink/dblink.c
===
RCS file: /home/postgres/cvsrepo/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.65
diff -c -r1.65 dblink.c
*** contrib/dblink/dblink.c	27 Aug 2007 01:24:50 -	1.65
--- contrib/dblink/dblink.c	28 Sep 2007 02:26:18 -
***
*** 1690,1696 
  	AclResult	aclresult;
  
  	/* open relation using relid, check permissions, get tupdesc */
! 	rel = relation_open(relid, AccessShareLock);
  
  	aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
    ACL_SELECT);
--- 1690,1696 
  	AclResult	aclresult;
  
  	/* open relation using relid, check permissions, get tupdesc */
! 	rel = relation_open(relid, AccessShareLock, true);
  
  	aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
    ACL_SELECT);
***
*** 1819,1825 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
--- 1819,1825 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock, true);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
***
*** 1902,1908 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
--- 1902,1908 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock, true);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
***
*** 1954,1960 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
--- 1954,1960 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock, true);
  	tupdesc = rel-rd_att;
  	natts = tupdesc-natts;
  
***
*** 2098,2104 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock);
  	tupdesc = CreateTupleDescCopy(rel-rd_att);
  	relation_close(rel, AccessShareLock);
  
--- 2098,2104 
  	/*
  	 * Open relation using relid
  	 */
! 	rel = relation_open(relid, AccessShareLock, true);
  	tupdesc = CreateTupleDescCopy(rel-rd_att);
  	relation_close(rel, AccessShareLock);
  
Index: contrib/pgstattuple/pgstattuple.c
===
RCS file: /home/postgres/cvsrepo/pgsql/contrib/pgstattuple/pgstattuple.c,v
retrieving revision 1.30
diff -c -r1.30 pgstattuple.c
*** contrib/pgstattuple/pgstattuple.c	20 Sep 2007 17:56:30 -	1.30
--- contrib/pgstattuple/pgstattuple.c	28 Sep 2007 02:22:00 -
***
*** 183,189 
   (errmsg(must be superuser to use pgstattuple functions;
  
  	/* open relation */
! 	rel = relation_open(relid, AccessShareLock);
  
  	PG_RETURN_DATUM(pgstat_relation(rel, fcinfo));
  }
--- 183,189 
   (errmsg(must be superuser to use pgstattuple functions;
  
  	/* open relation */
! 	rel = relation_open(relid, AccessShareLock, true);
  
  	PG_RETURN_DATUM(pgstat_relation(rel, fcinfo));
  }
Index: src/backend/access/heap/heapam.c
===
RCS file: /home/postgres/cvsrepo/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.242
diff -c -r1.242 heapam.c
*** src/backend/access/heap/heapam.c	21 Sep 2007 21:25:42 -	1.242
--- src/backend/access/heap/heapam.c	28 Sep 2007 02:51:24 -
***
*** 819,824 
--- 819,827 
   *		obtained on the relation.  (Generally, NoLock should only be
   *		used if the caller knows it has some appropriate lock on the
   *		relation already.)
+  *		
+  *		if wait is false open but don't wait for lock, instead throw an error 
+  *		when the requested lock is not immediately obtainable.
   *
   *		An error is raised if the relation does not exist.
   *
***
*** 827,833 
   * 
   */
  Relation
! relation_open(Oid relationId, LOCKMODE lockmode)
  {
  	Relation	r;
  
--- 830,836 
   * 
   */
  Relation
! relation_open(Oid relationId, LOCKMODE lockmode, bool wait)
  {
  	Relation	r;
  
***
*** 835,842 
  
  	/* Get the lock before trying to open the relcache entry */
  	if (lockmode != NoLock)
! 		LockRelationOid(relationId, lockmode);
! 
  	/* The relcache does all the real work... */
  	r = 

Re: [PATCHES] too many variants of relation_open

[Neil Conway]

On Fri, 2007-09-28 at 10:02 +0100, Heikki Linnakangas wrote:
 Well, yes it could, but why? Keeping them separate looks slightly more
 readable to me, and the change could break a lot of external modules for
 no good reason.

I agree: it also complicates the common case (calling relation_open()
and waiting to acquire for a lock).

If anything, you could perhaps refactor relation_open_nowait() to be
implemented in terms of relation_open() (first try to get the lock, then
do a relation_open(rel, NoLock)), but since you'd only be saving a
handful of duplicated lines, it hardly seems worth it.

-Neil



---(end of broadcast)---
TIP 6: explain analyze is your friend