Re: cleanup execTuples.c

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: cleanup execTuples.c
Date: 2003-12-01 23:08:35
Message-ID: 200312012308.hB1N8Zm11127@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches


Patch applied. Thanks.

---------------------------------------------------------------------------

Neil Conway wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

In response to

Browse pgsql-patches by date

  From Date Subject
Next Message Bruce Momjian 2003-12-01 23:10:46 Re: [PATCH] Re: [pgsql-advocacy] Why READ ONLY transactions?
Previous Message Tom Lane 2003-12-01 23:08:06 Re: 7.4 shared memory error on 64-bit SPARC/Solaris 5.8