Re: PL/PGSQL: Dynamic Record Introspection

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: uol(at)freenet(dot)de, Neil Conway <neilc(at)samurai(dot)com>, PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: PL/PGSQL: Dynamic Record Introspection
Date: 2006-05-30 13:39:18
Message-ID: 200605301339.k4UDdIo14119@candle.pha.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches


OK, patch reverted, and attached. Would the author please revise?
Thanks.

It seems like a cool feature.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Patch applied. Thanks.
>
> I wish to object to this patch; it's poorly designed, poorly coded, and
> badly documented. The philosophy seems to be "I want this feature
> and I don't care what I have to do to the semantics and performance
> of plpgsql to get it". In particular I don't like the bits that go
>
> /* Do not allow typeids to become "narrowed" by InvalidOids
> causing specialized typeids from the tuple restricting the destination */
>
> The incoherence of the comment is a good guide to how poorly thought out
> the code is. These bits are positioned so that they change the language
> semantics even when not using a dynamic field reference; what's more,
> they don't make any sense when you *are* using a dynamic field
> reference, because you need to keep the actual field type not try
> (probably vainly) to cast it to whatever the previous field's type was.
>
> I also dislike the loop added to exec_eval_cleanup(), which will impose
> a significant performance penalty on every single expression evaluation
> done by any plpgsql function, whether it's ever heard of dynamic field
> references or not. I doubt it's even necessary; I think the author
> doesn't understand plpgsql's memory allocation strategy very well.
>
> Lastly, and this is maybe a judgement call, I find the changes to
> PLpgSQL_recfield fairly ugly. It'd be better to make a separate
> datum type PLpgSQL_dynamic_recfield or some such.
>
> This needs to be bounced back for rework. It looks like a first draft
> that should have been rewritten once the author had learned enough to
> make it sort-of-work.
>
> regards, tom lane
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Attachment Content-Type Size
unknown_filename text/plain 23.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2006-05-30 13:41:59 Re: plperl's ppport.h out of date?
Previous Message Tom Lane 2006-05-30 13:35:38 Re: PL/PGSQL: Dynamic Record Introspection

Browse pgsql-patches by date

  From Date Subject
Next Message Bruce Momjian 2006-05-30 13:51:13 Re: [PATCH] Warning about configure args (weaker version)
Previous Message Tom Lane 2006-05-30 13:35:38 Re: PL/PGSQL: Dynamic Record Introspection