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 |
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 |
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 |