Re: PL/PGSQL: Dynamic Record Introspection - Mailing list pgsql-patches
From | uol@freenet.de |
---|---|
Subject | Re: PL/PGSQL: Dynamic Record Introspection |
Date | |
Msg-id | 447CCD1B.7070508@freenet.de Whole thread Raw |
In response to | Re: PL/PGSQL: Dynamic Record Introspection (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-patches |
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
pgsql-patches by date: