Tom Lane <[EMAIL PROTECTED]> writes:
> Please use names for the replacement routines that are more clear
> than "fooInternal".  You can get away with that kind of name for a
> static function, but I think globally visible ones should have more
> meaningful names.

The only function I named "fooInternal" was ExecTypeFromTLInternal,
which is static.

> For ExecTypeFromTLInternal, maybe use ExecTupDescFromTL, which is a
> more accurate name in the first place

What's the logic in having ExecTypeFromTL() and ExecCleanTypeFromTL()
implemented in terms of a function called ExecTupDescFromTL()? i.e. if
we're going to be renaming functions, wouldn't it make sense to rename
the public API functions, not the internal static functions?

> As for the Slot functions, I agree with getting rid of the macros,
> which seem to add little except obfuscation.  But I see no need to
> introduce an extra layer of calls.  Why not make them all go
> directly to ExecAllocTableSlot(estate->es_tupleTable)?

Yeah, I was considering that, both ways seemed about equal to me.

Attached is a revised version of the patch. I've adopted Tom's
suggestion for the slot functions. For renaming
ExecTypeFromTLInternal(), I haven't changed the name of the function
(see my comments above), but if you clarify what you're suggesting, I
can submit another version of the patch.

BTW, this code includes the comment:

 *              Currently there are about 4 different places where we create
 *              TupleDescriptors.  They should all be merged, or perhaps be
 *              rewritten to call BuildDesc().

Aside from the fact that BuildDesc() doesn't exist anymore AFAICS,
would this still be a reasonable reorganization to make?

-Neil
Index: src/backend/executor/execTuples.c
===================================================================
RCS file: /var/lib/cvs/pgsql-server/src/backend/executor/execTuples.c,v
retrieving revision 1.72
diff -c -r1.72 execTuples.c
*** src/backend/executor/execTuples.c	29 Sep 2003 18:22:48 -0000	1.72
--- src/backend/executor/execTuples.c	21 Nov 2003 01:25:14 -0000
***************
*** 112,117 ****
--- 112,119 ----
  #include "executor/executor.h"
  #include "utils/lsyscache.h"
  
+ static TupleDesc ExecTypeFromTLInternal(List *targetList,
+ 										bool hasoid, bool skipjunk);
  
  /* ----------------------------------------------------------------
   *				  tuple table create/delete functions
***************
*** 469,481 ****
   *		is used for initializing special-purpose slots.
   * --------------------------------
   */
- #define INIT_SLOT_DEFS \
- 	TupleTable	   tupleTable; \
- 	TupleTableSlot*   slot
- 
- #define INIT_SLOT_ALLOC \
- 	tupleTable = (TupleTable) estate->es_tupleTable; \
- 	slot =		 ExecAllocTableSlot(tupleTable);
  
  /* ----------------
   *		ExecInitResultTupleSlot
--- 471,476 ----
***************
*** 484,492 ****
  void
  ExecInitResultTupleSlot(EState *estate, PlanState *planstate)
  {
! 	INIT_SLOT_DEFS;
! 	INIT_SLOT_ALLOC;
! 	planstate->ps_ResultTupleSlot = slot;
  }
  
  /* ----------------
--- 479,485 ----
  void
  ExecInitResultTupleSlot(EState *estate, PlanState *planstate)
  {
! 	planstate->ps_ResultTupleSlot = ExecAllocTableSlot(estate->es_tupleTable);
  }
  
  /* ----------------
***************
*** 496,504 ****
  void
  ExecInitScanTupleSlot(EState *estate, ScanState *scanstate)
  {
! 	INIT_SLOT_DEFS;
! 	INIT_SLOT_ALLOC;
! 	scanstate->ss_ScanTupleSlot = slot;
  }
  
  /* ----------------
--- 489,495 ----
  void
  ExecInitScanTupleSlot(EState *estate, ScanState *scanstate)
  {
! 	scanstate->ss_ScanTupleSlot = ExecAllocTableSlot(estate->es_tupleTable);
  }
  
  /* ----------------
***************
*** 508,516 ****
  TupleTableSlot *
  ExecInitExtraTupleSlot(EState *estate)
  {
! 	INIT_SLOT_DEFS;
! 	INIT_SLOT_ALLOC;
! 	return slot;
  }
  
  /* ----------------
--- 499,505 ----
  TupleTableSlot *
  ExecInitExtraTupleSlot(EState *estate)
  {
! 	return ExecAllocTableSlot(estate->es_tupleTable);
  }
  
  /* ----------------
***************
*** 560,593 ****
  TupleDesc
  ExecTypeFromTL(List *targetList, bool hasoid)
  {
! 	TupleDesc	typeInfo;
! 	List	   *tlitem;
! 	int			len;
! 
! 	/*
! 	 * allocate a new typeInfo
! 	 */
! 	len = ExecTargetListLength(targetList);
! 	typeInfo = CreateTemplateTupleDesc(len, hasoid);
! 
! 	/*
! 	 * scan list, generate type info for each entry
! 	 */
! 	foreach(tlitem, targetList)
! 	{
! 		TargetEntry *tle = lfirst(tlitem);
! 		Resdom	   *resdom = tle->resdom;
! 
! 		TupleDescInitEntry(typeInfo,
! 						   resdom->resno,
! 						   resdom->resname,
! 						   resdom->restype,
! 						   resdom->restypmod,
! 						   0,
! 						   false);
! 	}
! 
! 	return typeInfo;
  }
  
  /* ----------------------------------------------------------------
--- 549,555 ----
  TupleDesc
  ExecTypeFromTL(List *targetList, bool hasoid)
  {
! 	return ExecTypeFromTLInternal(targetList, hasoid, false);
  }
  
  /* ----------------------------------------------------------------
***************
*** 599,628 ****
  TupleDesc
  ExecCleanTypeFromTL(List *targetList, bool hasoid)
  {
! 	TupleDesc	typeInfo;
! 	List	   *tlitem;
! 	int			len;
! 	int			cleanresno;
  
! 	/*
! 	 * allocate a new typeInfo
! 	 */
! 	len = ExecCleanTargetListLength(targetList);
  	typeInfo = CreateTemplateTupleDesc(len, hasoid);
  
! 	/*
! 	 * scan list, generate type info for each entry
! 	 */
! 	cleanresno = 1;
! 	foreach(tlitem, targetList)
  	{
! 		TargetEntry *tle = lfirst(tlitem);
! 		Resdom	   *resdom = tle->resdom;
  
! 		if (resdom->resjunk)
  			continue;
  		TupleDescInitEntry(typeInfo,
! 						   cleanresno++,
  						   resdom->resname,
  						   resdom->restype,
  						   resdom->restypmod,
--- 561,592 ----
  TupleDesc
  ExecCleanTypeFromTL(List *targetList, bool hasoid)
  {
! 	return ExecTypeFromTLInternal(targetList, hasoid, true);
! }
  
! static TupleDesc
! ExecTypeFromTLInternal(List *targetList, bool hasoid, bool skipjunk)
! {
! 	TupleDesc	 typeInfo;
! 	List		*l;
! 	int			 len;
! 	int			 cur_resno = 1;
! 
! 	if (skipjunk)
! 		len = ExecCleanTargetListLength(targetList);
! 	else
! 		len = ExecTargetListLength(targetList);
  	typeInfo = CreateTemplateTupleDesc(len, hasoid);
  
! 	foreach(l, targetList)
  	{
! 		TargetEntry	*tle = lfirst(l);
! 		Resdom		*resdom = tle->resdom;
  
! 		if (skipjunk && resdom->resjunk)
  			continue;
  		TupleDescInitEntry(typeInfo,
! 						   cur_resno++,
  						   resdom->resname,
  						   resdom->restype,
  						   resdom->restypmod,
---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faqs/FAQ.html

Reply via email to