Re: PL/PGSQL: Dynamic Record Introspection

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bruce Momjian <pgman(at)candle(dot)pha(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:35:38
Message-ID: 23092.1148996138@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2006-05-30 13:39:18 Re: PL/PGSQL: Dynamic Record Introspection
Previous Message Andy Shellam 2006-05-30 13:35:27 Re: Problem building initdb on sparc10

Browse pgsql-patches by date

  From Date Subject
Next Message Bruce Momjian 2006-05-30 13:39:18 Re: PL/PGSQL: Dynamic Record Introspection
Previous Message Bruce Momjian 2006-05-30 13:30:13 Re: archiver.pid