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
