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
