Tom Lane schrieb:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
It seems like a cool feature.
The fundamental problem with it is that plpgsql isn't designed to deal
with dynamically changing data types. The patch as submitted contained
some hacks that sort of dealt with this in some cases (I don't think it
covered them all), but really some serious thought is going to have to
go into making that work. It'd be good to do, because the existing
RECORD-variable feature already creates cases where it's needed; but
AFAICS it's not something that can be fixed off-the-cuff.
Tom,
I took some time to reinvestigate my patch.
At a closer look on the code you will find the
free-reprepare statements for the plan being the
only relevant part where changing
types are dealt with besides the cited comment with english of
- I have to admit it - questionable quality.
The "fundamental problem" stated above as well as the patch
code to free and reprepare the plan when encountering
expressions with types changing after stmt preparation
(up to now the newly introduced construct RECORD.(variable))
leads to the fact that the if part where the cited bogus comment is
placed should never get executed, thus no however vainly
casting is ever necessary. Maybe you meant this with your mail?
You can safely change back that if to the previous
version and leave the else part as it is. The complete
if clause as sent in by me is superfluous:
I cannot see any cases when this if would ever get executed
besides usage of a RECORD variable indexed by "hard coded"
column name in a loop where you fill
the RECORD with different row types that happen to have the
same column name. Personally speaking, I'd consider
this as dangerous nonsense code.
My change simply would replace the original error message of
different types between preparation and execution time
with a different error message when trying to assign incompatible
types, thus causing the cast to fail.
The change of the if clause was introduced before I
was fully aware of the "fundamental problem" and I forgot to
remove it afterwards.
However, I apologize for the inconvenience caused by this if clause,
it's bogus comment, and for my insolence to send in a patch with 10
lines of unimportant bogus code before having learned enough.
If the freeing of the array with record field names is unnecessary
and causes any irritation to you, simply remove it.
I'm used to free memory after usage; if postgres or plpgsql
handles that automatically or otherwise differently in that case,
change the patch.
I'd rather get accused for slowing down things than
silently eating up memory. And I'm sure *you* know if it's safe
to drop that pointer or whatever else to do.
Being ugly or not, the changes to PLpgSQL_recfield were deliberatly
chosen over a separate struct to be sure that records are
always dealt with in the same way regardless of the field indexing
method, to avoid code duplication between these two indexing cases
(considering that the code dealing with the actual record field is more
important to hold in common than to distinguish the two indexing
cases with separate structures),
and to make sure to catch all cases where this structure is dealt with
by simply provoking a compilation error because of the moved struct
member.
So the bits do not "change language semantics", just because a
bad comment and a never executed if clause is left in the code.
Rest assured: the code (except the if clause) is in use on several
servers for a year now and the rest of our now quite substantial amount
of plpgsql code not using the newly introduced RECORD indexing construct
remains very compatible with the interpreter without my patch,
and the usual regression test executes without problems
on an interpreter with that patch.
Thus I dare to say that this patch more than "sort-of" works,
even if - as I always would be the first to admit -
my position on the apparently steep learning curve
of pgsql internals is still quite near the origin of the coordinate
system.
I would suggest you change back the bogus if clause and
- if you, as I presume, know better than me what happens -
drop or change the freeing of the memory of the record field names array
and apply the patch. For other changes I'd suggest you should apologize
for your mail in case you would expect me to implement them
or please find someone else to bear with you.
Titus
---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
