Thread: cleanup execTuples.c
This patch refactors execTuples.c in two ways:
(1) ExecInitXXXResultTupleSlot() used a macro to avoid some
duplicated code, whereas calling ExecInitExtraTupleSlot() would
make the code more clear.
(2) ExecTypeFromTL() and ExecCleanTypeFromTL() duplicated a bunch
of code; I added a new function ExecTypeFromTLInternal() and
re-implemented these functions in terms of calls to it.
As a result, ExecInitScanTupleSlot(), ExecInitResultTupleSlot(),
ExecTypeFromTL(), and ExecCleanTypeFromTL() are now all trivial
(1 line) functions. I could have replaced these with macros, but I
didn't: does anyone thinks that would be worth doing?
-Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> As a result, ExecInitScanTupleSlot(), ExecInitResultTupleSlot(),
> ExecTypeFromTL(), and ExecCleanTypeFromTL() are now all trivial
> (1 line) functions. I could have replaced these with macros, but I
> didn't: does anyone thinks that would be worth doing?
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.
For ExecTypeFromTLInternal, maybe use ExecTupDescFromTL, which is a more
accurate name in the first place ...
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)? I don't see that
planstate->ps_ResultTupleSlot = ExecInitExtraTupleSlot(estate);
is better than
planstate->ps_ResultTupleSlot = ExecAllocTableSlot(estate->es_tupleTable);
regards, tom lane
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
Attachment
Neil Conway <neilc@samurai.com> writes:
>> 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?
My point was that you intended to export ExecTypeFromTLInternal in order
to convert the other names to macros, and I didn't want an exported name
like that.
The number of call sites seems small enough that altering the API isn't
out of the question either, if you like that better.
regards, tom lane
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> 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
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faqs/FAQ.html
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > 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 [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073