Re: cleanup execTuples.c

Tom Lane <tgl@sss.pgh.pa.us> 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