View Single Post

   
  #2 (permalink)  
Old 04-18-2008, 01:40 AM
Tom Lane
 
Posts: n/a
Default Re: PL/PGSQL: Dynamic Record Introspection

Bruce Momjian <pgman@candle.pha.pa.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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Reply With Quote