Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
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
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Patch applied. Thanks. --- [EMAIL PROTECTED] wrote: Bruce Momjian schrieb: This patch cannot be applied. 'active_simple_exprs' is referenced but not defined. I think the new variable name is 'simple_eval_estate', which used to be a structure member of 'active_simple_exprs'. Would you please update it against current CVS and resubmit? Thanks. Attached please find the patch against head of today, 2034 UTC Someone had removed active_simple_exprs. Actually I didn't use it but simply had to clean it up before calling freeplan. I removed the appropriate lines of code. Please let me know if anything appears to be wrong with this patch. Regards Titus *** ./doc/src/sgml/plpgsql.sgml.orig Fri Mar 10 20:10:48 2006 --- ./doc/src/sgml/plpgsql.sgml Mon May 8 23:40:12 2006 *** *** 865,870 --- 865,919 /para para + To obtain the values of the fields the record is made up of, + the record variable can be qualified with the column or field + name. This can be done either by literally using the column name + or the column name for indexing the record can be taken out of a scalar + variable. The syntax for this notation is Record_variable.(IndexVariable). + To get information about the column field names of the record, + a special expression exists that returns all column names as an array: + RecordVariable.(*) . + Thus, the RECORD can be viewed + as an associative array that allows for introspection of it's contents. + This feature is especially useful for writing generic triggers that + operate on records with unknown structure. + Here is an example procedure that shows column names and values + of the predefined record NEW in a trigger procedure: + programlisting + + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $$ + DECLARE + colname TEXT; + colcontent TEXT; + colnamesTEXT[]; + colnINT4; + coliINT4; + BEGIN + -- obtain an array with all field names of the record + colnames := NEW.(*); + RAISE NOTICE 'All column names of test record: %', colnames; + -- show field names and contents of record + coli := 1; + coln := array_upper(colnames,1); + RAISE NOTICE 'Number of columns in NEW: %', coln; + FOR coli IN 1 .. coln LOOP + colname := colnames[coli]; + colcontent := NEW.(colname); + RAISE NOTICE 'column % of NEW: %', quote_ident(colname), quote_literal(colcontent); + END LOOP; + -- Do it with a fixed field name: + -- will have to know the column name + RAISE NOTICE 'column someint of NEW: %', quote_literal(NEW.someint); + RETURN NULL; + END; + $$ LANGUAGE plpgsql; + --CREATE TABLE test_records (someint INT8, somestring TEXT); + --CREATE TRIGGER tr_test_record BEFORE INSERT ON test_records FOR EACH ROW EXECUTE PROCEDURE show_associative_records(); + + /programlisting +/para + +para Note that literalRECORD/ is not a true data type, only a placeholder. One should also realize that when a applicationPL/pgSQL/application function is declared to return type typerecord/, this is not quite the *** ./src/pl/plpgsql/src/pl_comp.c.orig Tue Mar 14 23:48:23 2006 --- ./src/pl/plpgsql/src/pl_comp.cMon May 8 22:49:50 2006 *** *** 870,876 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[1]); new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); --- 870,877 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldindex.fieldname = pstrdup(cp[1]); ! new-fieldindex_flag = RECFIELD_USE_FIELDNAME; new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); *** *** 976,982 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[2]); new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); --- 977,984 new = palloc(sizeof(PLpgSQL_recfield));
Re: [PATCHES] 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
OK, patch reverted, and attached. Would the author please revise? Thanks. It seems like a cool feature. --- Tom Lane wrote: 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 -- Bruce Momjian http://candle.pha.pa.us EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + *** ./doc/src/sgml/plpgsql.sgml.origFri Mar 10 20:10:48 2006 --- ./doc/src/sgml/plpgsql.sgml Mon May 8 23:40:12 2006 *** *** 865,870 --- 865,919 /para para + To obtain the values of the fields the record is made up of, + the record variable can be qualified with the column or field + name. This can be done either by literally using the column name + or the column name for indexing the record can be taken out of a scalar + variable. The syntax for this notation is Record_variable.(IndexVariable). + To get information about the column field names of the record, + a special expression exists that returns all column names as an array: + RecordVariable.(*) . + Thus, the RECORD can be viewed + as an associative array that allows for introspection of it's contents. + This feature is especially useful for writing generic triggers that + operate on records with unknown structure. + Here is an example procedure that shows column names and values + of the predefined record NEW in a trigger procedure: + programlisting + + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $$ + DECLARE + colname TEXT; + colcontent TEXT; + colnamesTEXT[]; + colnINT4; + coliINT4; + BEGIN + -- obtain an array with all field names of the record + colnames := NEW.(*); + RAISE NOTICE 'All column names of test record: %', colnames; + -- show field names and contents of record + coli := 1; + coln := array_upper(colnames,1); + RAISE NOTICE 'Number of columns in NEW: %', coln; + FOR coli IN 1 .. coln LOOP + colname := colnames[coli]; + colcontent := NEW.(colname); + RAISE NOTICE 'column % of NEW: %', quote_ident(colname), quote_literal(colcontent); + END LOOP; + -- Do it with a fixed field name: + -- will have to know the column name + RAISE NOTICE 'column someint of NEW: %', quote_literal(NEW.someint); + RETURN NULL; + END; + $$ LANGUAGE plpgsql; + --CREATE TABLE test_records (someint INT8, somestring TEXT); + --CREATE TRIGGER tr_test_record BEFORE INSERT ON test_records FOR EACH ROW EXECUTE PROCEDURE show_associative_records(); + + /programlisting +/para + +para Note that literalRECORD/ is not a true data type, only a placeholder. One should also realize that when a applicationPL/pgSQL/application function is declared to return type typerecord/, this is not quite the *** ./src/pl/plpgsql/src/pl_comp.c.orig Tue Mar 14 23:48:23 2006 --- ./src/pl/plpgsql/src/pl_comp.c Mon May 8 22:49:50 2006 *** *** 870,876
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
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. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
[EMAIL PROTECTED] said: The patch was now hanging around more than 9 months, and it was already accepted during the original discussion by Neil Conway. Nonsense. I provided some specific code cleanup suggestions and said that I didn't object to the general feature. That should not be taken as an endorsement of everything that I didn't specifically comment on. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
No, the author would not. Tom did participate in the discussion at the time when the original patch was developed. The patch was now hanging around more than 9 months, and it was already accepted during the original discussion by Neil Conway. Please figure out by yourselves who might be the one to really, really, finally accept the patch. A word about the discussion style in general: I personally don't like general objections on the code *after* I - again - spent some hours of work to comply to Bruce's wish to rewrite the patch for HEAD. After all, Tom, you don't pay me? And I don't like quoting my comments while at the same time complaining about poorly coded software. This simply is nonsense. The other attributes you gave to the patch are simply bad discussion style and at the same time nonsense as well: The patch tries to *minimize* changes to plpgsql and does not deal with fundamental problems of the interpreter. For this reason, of course the patch is more or less a hack. Apparently, the RECORD construct without that patch does not really make much sense; ROWTYPE would suffice. This is why the patch is a cool feature. If now Tom or the community (being identical?) in general have the feeling to start some kind of complete or otherwise fundamental rewrite of plpgsql to handle constructs like the one implemented by me, I suggest someone else should do that. I do not have the time to rewrite plpgsql or to participate in this kind of discussions. Regards Titus Bruce Momjian wrote: OK, patch reverted, and attached. Would the author please revise? Thanks. It seems like a cool feature. --- Tom Lane wrote: 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Bruce Momjian wrote: Your patch has been added to the PostgreSQL unapplied patches list It was agreed earlier that this should be saved for 8.2, I believe. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
This has been saved for the 8.2 release: http://momjian.postgresql.org/cgi-bin/pgpatches_hold --- Titus von Boxberg wrote: Tom Lane schrieb: Titus von Boxberg [EMAIL PROTECTED] writes: It works for me if we want to have an NFIELDS construct. Personally I'm still not convinced that we need one --- what's the use-case? I have removed the NFIELDS construct I'd prefer arbitrary expression, but I suppose there's no harm in doing the simple case first and generalizing if there's demand. I took the no harm way. Attached please find the updated patch. The patch is against HEAD of 050721. I switched the syntax to your proposal, renamed the functions in pl_comp.c and updated the sgml doc source and regression test files accordingly. Regards Titus *** ./doc/src/sgml/plpgsql.sgml.orig Sat Jul 2 08:59:47 2005 --- ./doc/src/sgml/plpgsql.sgml Sat Jul 23 17:24:54 2005 *** *** 867,872 --- 867,921 /para para + To obtain the values of the fields the record is made up of, + the record variable can be qualified with the column or field + name. This can be done either by literally using the column name + or the column name for indexing the record can be taken out of a scalar + variable. The syntax for this notation is Record_variable.(IndexVariable). + To get information about the column field names of the record, + a special expression exists that returns all column names as an array: + RecordVariable.(*) . + Thus, the RECORD can be viewed + as an associative array that allows for introspection of it's contents. + This feature is especially useful for writing generic triggers that + operate on records with unknown structure. + Here is an example procedure that shows column names and values + of the predefined record NEW in a trigger procedure: + programlisting + + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $$ + DECLARE + colname TEXT; + colcontent TEXT; + colnamesTEXT[]; + colnINT4; + coliINT4; + BEGIN + -- obtain an array with all field names of the record + colnames := NEW.(*); + RAISE NOTICE 'All column names of test record: %', colnames; + -- show field names and contents of record + coli := 1; + coln := array_upper(colnames,1); + RAISE NOTICE 'Number of columns in NEW: %', coln; + FOR coli IN 1 .. coln LOOP + colname := colnames[coli]; + colcontent := NEW.(colname); + RAISE NOTICE 'column % of NEW: %', quote_ident(colname), quote_literal(colcontent); + END LOOP; + -- Do it with a fixed field name: + -- will have to know the column name + RAISE NOTICE 'column someint of NEW: %', quote_literal(NEW.someint); + RETURN NULL; + END; + $$ LANGUAGE plpgsql; + --CREATE TABLE test_records (someint INT8, somestring TEXT); + --CREATE TRIGGER tr_test_record BEFORE INSERT ON test_records FOR EACH ROW EXECUTE PROCEDURE show_associative_records(); + + /programlisting +/para + +para Note that literalRECORD/ is not a true data type, only a placeholder. One should also realize that when a applicationPL/pgSQL/application function is declared to return type typerecord/, this is not quite the *** ./src/pl/plpgsql/src/pl_comp.c.orig Wed Jul 6 16:42:10 2005 --- ./src/pl/plpgsql/src/pl_comp.cThu Jul 21 21:28:15 2005 *** *** 995,1001 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[1]); new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); --- 995,1002 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldindex.fieldname = pstrdup(cp[1]); ! new-fieldindex_flag = RECFIELD_USE_FIELDNAME; new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); *** *** 1101,1107 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[2]); new-recparentno = ns-itemno; plpgsql_adddatum((PLpgSQL_datum *) new); ---
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
-Ursprüngliche Nachricht- Von: Tom Lane An: Neil Conway Cc: Titus von Boxberg Betreff: Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection --- There's a worse objection, which is that % is a perfectly valid operator OK. I did not recognize that. Another small problem with the patch is the untenable assumption that neither FIELDNAMES nor NFIELDS is ever used by anyone as an actual field name. No. You cannot name a variable NFIELDS or FIELDNAMES that you want to use in this expression. The record field names themselves are unaffected. As for NFIELDS, I don't think we need it -- you can always measure the length of the FIELDNAMES array. I would like to leave it there. As far as I can see it's much faster than other builtins for evaluating array dimensions and I think it does not hurt. What about recordvalue.(#) staying with your syntax? But the real $64 question for plpgsql stuff is what does Oracle do? I don't have much experience with Oracle. From what I know, Oracle PL/SQL lacks the concept of records with dynamically assigned structure. This would make this construct meaningless. Same for MS SQL. Don't know anything about DB2. - Questions: - could anyone who knows Oracle better than me confirm that with Oracle there are no RECORD variables of varying dynamically assigned type? And thus there is no construct dyamically acessing the fields of a RECORD type? - is the syntax RECORD.(identifier), RECORD.(*), RECORD.(#) still acceptable? I don't have any objections, but as I already overlooked the modulo stuff I'd assume my jugdement will not be authoritative :-) - do you agree with my approach that identifier is restricted to be a variable and cannot be an arbitrary expression evaluating to a string? - do you accept the NFIELDS construct, whatever it's syntax finally would look like? Or do you want it finally to be removed. I'd then modify the code to the new syntax. Regards Titus ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Titus von Boxberg [EMAIL PROTECTED] writes: Questions: - could anyone who knows Oracle better than me confirm that with Oracle there are no RECORD variables of varying dynamically assigned type? Anyone? - is the syntax RECORD.(identifier), RECORD.(*), RECORD.(#) still acceptable? It works for me if we want to have an NFIELDS construct. Personally I'm still not convinced that we need one --- what's the use-case? - do you agree with my approach that identifier is restricted to be a variable and cannot be an arbitrary expression evaluating to a string? I'd prefer arbitrary expression, but I suppose there's no harm in doing the simple case first and generalizing if there's demand. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
On 7/19/05 8:11 AM, Titus von Boxberg [EMAIL PROTECTED] wrote: As for NFIELDS, I don't think we need it -- you can always measure the length of the FIELDNAMES array. I would like to leave it there. As far as I can see it's much faster than other builtins for evaluating array dimensions and I think it does not hurt. Is performance a concern here? (Considering that you needn't repeatedly get the length of the array as you can store it in a local variable.) I agree with Tom, we can probably do without this. -Neil ---(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
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Neil Conway [EMAIL PROTECTED] writes: Titus von Boxberg wrote: What do you mean by right syntax. There are hundreds of examples in programming languages where a small syntactic difference leads to totally different semantics. Well, I'm just not happy that foo.bar means something completely different than foo%bar -- foo%bar isn't even fetching something called bar from foo. There's a worse objection, which is that % is a perfectly valid operator name (typically defined as modulo). The patch is therefore usurping syntax of an existing feature, which is surely Right Out. Another small problem with the patch is the untenable assumption that neither FIELDNAMES nor NFIELDS is ever used by anyone as an actual field name. Assuming that we're inventing in a green field, I'd suggest a syntax along this line: recordvalue.(stringexpression) which is not confusable with any currently-valid syntax. I'm not sure what to do about FIELDNAMES --- perhaps this would work: recordvalue.(*) As for NFIELDS, I don't think we need it -- you can always measure the length of the FIELDNAMES array. But the real $64 question for plpgsql stuff is what does Oracle do? Is there any remotely comparable functionality in PL/SQL, and if so what does it look like? regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Titus von Boxberg wrote: What do you mean by right syntax. There are hundreds of examples in programming languages where a small syntactic difference leads to totally different semantics. Well, I'm just not happy that foo.bar means something completely different than foo%bar -- foo%bar isn't even fetching something called bar from foo. Anyway, I can't see a better syntax, so I suppose this will do. Are there any general objections to implement this feature? Not from me. You missed the 8.1 freeze by about two weeks, so I'm not sure if this is a candidate for 8.1 though. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Neil Conway [EMAIL PROTECTED] writes: Not from me. You missed the 8.1 freeze by about two weeks, so I'm not sure if this is a candidate for 8.1 though. Not a chance. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
-Ursprüngliche Nachricht- Von: Tom Lane [mailto:[EMAIL PROTECTED] Neil Conway [EMAIL PROTECTED] writes: Not from me. You missed the 8.1 freeze by about two weeks, so I'm not sure if this is a candidate for 8.1 though. Not a chance. regards, tom lane I'd like to participate in postgresql development on a time to time basis as I continue using it and as my knowledge (hopefully) grows. Having read the FAQ, it's not clear to me, where the patches go (and when) that I would send into this list. These are my open questions: Are the patches applied to HEAD? And only Very Important Patches are merged to the current or even an older release branch? Main background questions are: What else (besides creating and testing the patch myself) can I do to alleviate applying (or deciding to apply) the patches? Could I expect (and if so in what time frame) the patches to appear in HEAD? So at some time I would find my code in a cvs up? How do I know that? Thanks for answering newbie questions. Regards Titus ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Titus von Boxberg wrote: Having read the FAQ, it's not clear to me, where the patches go (and when) that I would send into this list. These are my open questions: Are the patches applied to HEAD? It depends on the patch. New features are almost exclusively only applied to HEAD, whereas patches that fix bugs (with a low chance of introducing additional regressions) or fix typos in the documentation are applied to back branches. And only Very Important Patches are merged to the current or even an older release branch? Right. Main background questions are: What else (besides creating and testing the patch myself) can I do to alleviate applying (or deciding to apply) the patches? Could I expect (and if so in what time frame) the patches to appear in HEAD? So at some time I would find my code in a cvs up? Assuming the patch is accepted, it will eventually be reviewed and applied by one of the committers. The time frame for doing that depends on various factors: the size and complexity of the patch, the amount of review / fixing it needs, the workload of the committers, and the current stage of the release cycle (we just passed feature freeze for 8.1, so I think most committers are busy with work for the upcoming 8.1 beta release). Unfortunately we can be a bit tardy in applying patches at times, IMHO. How do I know that? It is standard practice to send email to patch submitters when their patch has been applied (or alternatively rejected, or a resubmission of the patch has been requested). You can also read the pgsql-committers mailing list, which logs all the CVS commits. -Neil ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Titus von Boxberg wrote:
With the following patch it's possible to
- extract all field names of a record into an array
- extract field count of a record
- address a single field of a record with a variable
containing the field name (additional to the usual record.fieldname
notation where the fieldname is hardcoded).
I wonder if this is the right syntax. record%identifier is doing
something fundamentally different from record.identifier, but the syntax
doesn't make that clear. I don't have any concrete suggestions for
improvement, mind you... :)
Test code for the patch can be extracted from an example I put into
plpgsql.sgml
Can you supply some proper regression tests, please? i.e. patch
sql/plpgsql.sql and expected/plpgsql.out in src/test/regress
A few minor comments from skimming the patch:
***
*** 995,1001
new = palloc(sizeof(PLpgSQL_recfield));
new-dtype = PLPGSQL_DTYPE_RECFIELD;
! new-fieldname = pstrdup(cp[1]);
new-recparentno = ns-itemno;
plpgsql_adddatum((PLpgSQL_datum *) new);
--- 995,1002
new = palloc(sizeof(PLpgSQL_recfield));
new-dtype = PLPGSQL_DTYPE_RECFIELD;
! new-fieldindex.fieldname = strdup(cp[1]);
! new-fieldindex_flag = RECFIELD_USE_FIELDNAME;
new-recparentno = ns-itemno;
plpgsql_adddatum((PLpgSQL_datum *) new);
Should be pstrdup().
***
*** 1101,1107
new = palloc(sizeof(PLpgSQL_recfield));
new-dtype = PLPGSQL_DTYPE_RECFIELD;
! new-fieldname = pstrdup(cp[2]);
new-recparentno = ns-itemno;
plpgsql_adddatum((PLpgSQL_datum *) new);
--- 1102,1109
new = palloc(sizeof(PLpgSQL_recfield));
new-dtype = PLPGSQL_DTYPE_RECFIELD;
! new-fieldindex.fieldname = strdup(cp[2]);
! new-fieldindex_flag = RECFIELD_USE_FIELDNAME;
new-recparentno = ns-itemno;
plpgsql_adddatum((PLpgSQL_datum *) new);
Ibid.
+ switch (ns1-itemtype)
+ {
+ case PLPGSQL_NSTYPE_REC:
+ {
+ PLpgSQL_recfieldproperties *new;
+
+ new = malloc(sizeof(PLpgSQL_recfieldproperties));
+ new-dtype = PLPGSQL_DTYPE_NRECFIELD;
+ new-recparentno = ns1-itemno;
+ plpgsql_adddatum((PLpgSQL_datum *) new);
+ plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
+ ret = T_SCALAR;/* ??? */
+ break;
Should be palloc().
+ return ret;
+ } /* plpgsql_parse_wordnfields */
The Postgres convention is not to include comments like this.
+ case PLPGSQL_NSTYPE_REC:
+ {
+ PLpgSQL_recfieldproperties *new;
+
+ new = malloc(sizeof(PLpgSQL_recfieldproperties));
+ new-dtype = PLPGSQL_DTYPE_RECFIELDNAMES;
+ new-recparentno = ns1-itemno;
+ plpgsql_adddatum((PLpgSQL_datum *) new);
+ plpgsql_yylval.scalar = (PLpgSQL_datum *) new;
+ ret = T_SCALAR;/* ??? */
+ break;
+ }
Should be palloc().
! /* construct_array copies data; free temp elem
array */
! #if 0
! for ( fc = 0; fc tfc; ++fc )
! pfree(DatumGetPointer(arrayelems[fc]);
! pfree(arrayelems);
! #endif
Unexplained #if 0 blocks aren't good.
-Neil
---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
good idea. it's what can eliminate not neccessery using plperl. I would to see it in plpgsql. regards Pavel On Thu, 14 Jul 2005, Titus von Boxberg wrote: Hi all, I needed introspection capabilities for record types to write more generic trigger procedures in PL/PGSQL. With the following patch it's possible to - extract all field names of a record into an array - extract field count of a record - address a single field of a record with a variable containing the field name (additional to the usual record.fieldname notation where the fieldname is hardcoded). The syntax is - record%NFIELDS gives the number of fields in the record - record%FIELDNAMES gives the array of the field names - record%scalarvariable extracts the field whose name is equal to the contents of scalarvariable The patch is nonintrusive in the sense that it only adds things with one exception: In function exec_eval_datum(), file pl_exec.c, line 3557 (after the patch) I chose to convert the record field values to TEXT if the caller does not require a certain type (expectedtypeid == InvalidOid). Additionally, I cast the value from the record field type if the destination Datum is of different type. As far as I can see, this does no harm because in most cases the expectedtypeid is correctly set. I just wanted to avoid that if it is not set, the returned datum is of a more restrictive type than TEXT. The patch is against a HEAD checkout from 07/12/05 The output comes from difforig. Test code for the patch can be extracted from an example I put into plpgsql.sgml Here is a summary of things that get patched by the file: - add three new special parsing functions to pl_comp.c (plpgsql_parse_wordpercentword, plpgsql_parse_wordnfields, plpgsql_parse_wordfieldnames). - modify PLpgSQL_recfield in plpgsql.h to either hold a conventional field name (record.fieldname) or a dno for the variable (record%variable). - add two PLPGSQL_DTYPEs for the two new % notations - modify case PLPGSQL_DTYPE_RECFIELD: in exec_eval_datum() and exec_assign_value() to deal with index strings from a variable - add case PLPGSQL_DTYPE_RECFIELDNAMES and case PLPGSQL_DTYPE_NRECFIELD to exec_eval_datum() to evaluate %FIELDNAMES and %NFIELDS expressions. - update plpgsql.sgml in the docs directory Please notify me if I can be of further assistance. Regards Titus ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Neil Conway schrieb: Titus von Boxberg wrote: Can you supply some proper regression tests, please? i.e. patch sql/plpgsql.sql and expected/plpgsql.out in src/test/regress In sql/plpgsql.sql I have added a function testing the new features and altered expected/plpgsql.out A few minor comments from skimming the patch: Done. -- The modifications to the first one are the following: - restored type evaluation in RECFIELD value evaluation in exec_eval_datum to the previous semantics. The addition should now be completely invisible to existing code. - instead do a check in exec_eval_expr() if the expression contains datums of the type record%variable. For these the type cannot be stored in a plan because the type might change between evaluations of this expr. -- free and reprepare the plan. - storage allocated by exec_eval_expr() for the expression record%FIELDNAMES (the array containing the names) is freed in exec_eval_cleanup -- regression tests are ok on my system. Do you agree with the mods and the patch? Regards Titus *** ./doc/src/sgml/plpgsql.sgml.origThu Jul 14 00:49:33 2005 --- ./doc/src/sgml/plpgsql.sgml Thu Jul 14 01:15:41 2005 *** *** 867,872 --- 867,941 /para para + To obtain the values of the fields the record is made up of, + the record variable can be qualified with the column or field + name. This can be done either literally with the usual dot notation + or the column name for indexing the record can be taken out of a scalar + variable. The syntax for this notation is Record_variable%IndexVariable. + To get information about the column fields, two special + notations exist that return all column names as an array (RecordVariable%FIELDNAMES) + and the count of fields stored in a record (RecordVariable%NFIELDS). + Thus, the RECORD can be viewed + as an associative array that allows for introspection of it's contents. + This feature is especially useful for writing generic triggers that + operate on records with unknown structure. + Here is an example that operates on the predefined record NEW in + a trigger procedure: + programlisting + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $ztt_u$ + DECLARE + wmsgTEXT; + column TEXT; + colcontent TEXT; + colnamesTEXT[]; + colnINT4; + coliINT4; + testint8INT8; + BEGIN + -- get the number of fields + coln := NEW%NFIELDS; + RAISE NOTICE 'Record type has % fields', coln; + + -- obtain an array with all field names of the record + colnames := NEW%FIELDNAMES; + RAISE NOTICE 'All column names of test record: %', colnames; + + -- show field names and contents of record + coli := 1; + LOOP + column := colnames[coli]; + colcontent := NEW%column; + IF colcontent IS NULL + THEN + colcontent := 'NULL'; + END IF; + RAISE NOTICE 'column name % of NEW: value %', column, colcontent; + coli := coli + 1; + EXIT WHEN coli coln; + END LOOP; + + -- get a single record field value indexed by the contents of the variable column + -- raises an error if the record does not have the field name from column + column := 'SomeFieldNameInYourRecord'; + -- testint8 will contain NULL if the value of this column + -- has a different type and cannot be casted to int8! + -- use a TEXT scalar variable to avoid this problem. + testint8 := NEW%column; + RAISE WARNING 'Column name = %, Column content = %', column, testint8; + + -- the normal way with fixed field name with dot notation: + wmsg := NEW.SomeFieldNameInYourRecord; + RAISE NOTICE 'Column content NEW.SomeFieldNameInYourRecord = %', wmsg; + + RETURN NULL; + END; + $ztt_u$ LANGUAGE plpgsql; + + + /programlisting +/para + +para Note that literalRECORD/ is not a true data type, only a placeholder. One should also realize that when a applicationPL/pgSQL/application function is declared to return type typerecord/, this is not quite the *** ./src/pl/plpgsql/src/pl_comp.c.orig Wed Jul 13 19:22:43 2005 --- ./src/pl/plpgsql/src/pl_comp.c Thu Jul 14 23:54:26 2005 *** *** 995,1001 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[1]); new-recparentno =
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Neil Conway schrieb: Titus von Boxberg wrote: Can you supply some proper regression tests, please? i.e. patch sql/plpgsql.sql and expected/plpgsql.out in src/test/regress In sql/plpgsql.sql I have added a function testing the new features and altered expected/plpgsql.out A few minor comments from skimming the patch: Done. -- The modifications to the first one are the following: - restored type evaluation in RECFIELD value evaluation in exec_eval_datum to the previous semantics. The addition should now be completely invisible to existing code. - instead do a check in exec_eval_expr() if the expression contains datums of the type record%variable. For these the type cannot be stored in a plan because the type might change between evaluations of this expr. -- free and reprepare the plan. - storage allocated by exec_eval_expr() for the expression record%FIELDNAMES (the array containing the names) is freed in exec_eval_cleanup -- regression tests are ok on my system. Do you agree with the mods and the patch? Regards Titus *** ./doc/src/sgml/plpgsql.sgml.origThu Jul 14 00:49:33 2005 --- ./doc/src/sgml/plpgsql.sgml Thu Jul 14 01:15:41 2005 *** *** 867,872 --- 867,941 /para para + To obtain the values of the fields the record is made up of, + the record variable can be qualified with the column or field + name. This can be done either literally with the usual dot notation + or the column name for indexing the record can be taken out of a scalar + variable. The syntax for this notation is Record_variable%IndexVariable. + To get information about the column fields, two special + notations exist that return all column names as an array (RecordVariable%FIELDNAMES) + and the count of fields stored in a record (RecordVariable%NFIELDS). + Thus, the RECORD can be viewed + as an associative array that allows for introspection of it's contents. + This feature is especially useful for writing generic triggers that + operate on records with unknown structure. + Here is an example that operates on the predefined record NEW in + a trigger procedure: + programlisting + CREATE OR REPLACE FUNCTION show_associative_records() RETURNS TRIGGER AS $ztt_u$ + DECLARE + wmsgTEXT; + column TEXT; + colcontent TEXT; + colnamesTEXT[]; + colnINT4; + coliINT4; + testint8INT8; + BEGIN + -- get the number of fields + coln := NEW%NFIELDS; + RAISE NOTICE 'Record type has % fields', coln; + + -- obtain an array with all field names of the record + colnames := NEW%FIELDNAMES; + RAISE NOTICE 'All column names of test record: %', colnames; + + -- show field names and contents of record + coli := 1; + LOOP + column := colnames[coli]; + colcontent := NEW%column; + IF colcontent IS NULL + THEN + colcontent := 'NULL'; + END IF; + RAISE NOTICE 'column name % of NEW: value %', column, colcontent; + coli := coli + 1; + EXIT WHEN coli coln; + END LOOP; + + -- get a single record field value indexed by the contents of the variable column + -- raises an error if the record does not have the field name from column + column := 'SomeFieldNameInYourRecord'; + -- testint8 will contain NULL if the value of this column + -- has a different type and cannot be casted to int8! + -- use a TEXT scalar variable to avoid this problem. + testint8 := NEW%column; + RAISE WARNING 'Column name = %, Column content = %', column, testint8; + + -- the normal way with fixed field name with dot notation: + wmsg := NEW.SomeFieldNameInYourRecord; + RAISE NOTICE 'Column content NEW.SomeFieldNameInYourRecord = %', wmsg; + + RETURN NULL; + END; + $ztt_u$ LANGUAGE plpgsql; + + + /programlisting +/para + +para Note that literalRECORD/ is not a true data type, only a placeholder. One should also realize that when a applicationPL/pgSQL/application function is declared to return type typerecord/, this is not quite the *** ./src/pl/plpgsql/src/pl_comp.c.orig Wed Jul 13 19:22:43 2005 --- ./src/pl/plpgsql/src/pl_comp.c Thu Jul 14 23:54:26 2005 *** *** 995,1001 new = palloc(sizeof(PLpgSQL_recfield)); new-dtype = PLPGSQL_DTYPE_RECFIELD; ! new-fieldname = pstrdup(cp[1]); new-recparentno =
Re: [PATCHES] PL/PGSQL: Dynamic Record Introspection
Neil Conway schrieb: I wonder if this is the right syntax. record%identifier is doing something fundamentally different from record.identifier, but the syntax doesn't make that clear. I don't have any concrete suggestions for improvement, mind you... :) What do you mean by right syntax. There are hundreds of examples in programming languages where a small syntactic difference leads to totally different semantics. I chose % because - it's already used in the var%TYPE notation. Here %xxx stands for a kind of extracting operator, too. In this sense, %TYPE's operation is similar to variables what %NFIELDS, %FIELDNAMES and %variable do to records. - I could not think of any other operator that would not be harmful for the parser (or cause much work) and also would not lead to other ambiguities in the sense not being right. - % strongly differs optically from . --- Can you supply some proper regression tests, please? i.e. patch sql/plpgsql.sql and expected/plpgsql.out in src/test/regress I'll do that. A few minor comments from skimming the patch: I'll implement your suggestions. I'll also remove the changes to the RECFIELD value evaluation semantics completely that I introduced. Are there any general objections to implement this feature? Personally, I would very much like to see it implemented because I do want to use plpgsql and without this feature (trigger) procedures cannot be held general if dealing with structural similarities between tables they are invoked upon. I'd send the patched patch when I'm done. Regards Titus ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
