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