Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection

I am not sure where to go on this patch. I agree there was a long delay
in getting you feedback on it, and some of the feedback was harsh.

However, the patch does need more work to get into PostgreSQL because it
has to work in all cases. It is a neat feature, for sure.

At this point, I have put the three email threads on the TODO list so if
you or someone else wants to work on it, we have references to all the
relevant code and comments.

---------------------------------------------------------------------------

uol(at)freenet(dot)de wrote:
> Tom Lane schrieb:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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
>

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +