Thread: fix: plpgsql: return query and dropped columns problem
Hello there is fix for bug Re: [BUGS] BUG #4907: stored procedures and changed tables regards Pavel Stehule 2009/7/10 Sergey Burladyan <eshkinkot@gmail.com>: > Sergey Burladyan <eshkinkot@gmail.com> writes: > >> Alvaro Herrera <alvherre@commandprompt.com> writes: >> >> > Michael Tenenbaum wrote: >> > >> > > If I have a stored procedure that returns a set of records of a table, I get >> > > an error message that the procedure's record is the wrong type after I >> > > change some columns in the table. >> > > >> > > Deleting the procedure then rewriting the procedure does not help. The only >> > > thing that works is deleting both the stored procedure and the table and >> > > starting over again. >> > >> > Does it work if you disconnect and connect again? >> >> No, example: > > More simple: > > PostgreSQL 8.4.0 on i486-pc-linux-gnu, compiled by GCC gcc-4.3.real (Debian 4.3.3-13) 4.3.3, 32-bit > > create table t (i int); > alter table t add v text; alter table t drop i; > create function foo() returns setof t language plpgsql as $$begin return query select * from t; end$$; > select foo(); > ERROR: 42804: structure of query does not match function result type > ПОДРОБНО: Number of returned columns (1) does not match expected column count (2). > КОНТЕКСТ: PL/pgSQL function "foo" line 1 at RETURN QUERY > РАСПОЛОЖЕНИЕ: validate_tupdesc_compat, pl_exec.c:5143 > > So, function with RETURNS SETOF tbl does not work if it created after ALTER TABLE > > 8.3.7 too: > > PostgreSQL 8.3.7 on i486-pc-linux-gnu, compiled by GCC gcc-4.3.real (Debian 4.3.3-5) 4.3.3 > > create table t (i int); > alter table t add v text; alter table t drop i; > create function foo() returns setof t language plpgsql as $$begin return query select * from t; end$$; > select * from foo(); > ERROR: 42804: structure of query does not match function result type > КОНТЕКСТ: PL/pgSQL function "foo" line 1 at RETURN QUERY > РАСПОЛОЖЕНИЕ: exec_stmt_return_query, pl_exec.c:2173 > > > -- > Sergey Burladyan > > -- > Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-bugs >
Attachment
On Sun, Jul 12, 2009 at 10:28 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote: > Hello > > there is fix for bug Re: [BUGS] BUG #4907: stored procedures and changed tables > this one applies, compiles and it actually fixes the bug... it should be backpatched until 8.3... but applies cleanly only until 8.4 (what a surprise), attached is an adapted version for 8.3 the only thing that is bothered me is that the same solution is used again and again... seems like we can use a function like make_tuple_from_row (maybe something like make_tuple_from_datum or eliminate_dropped_cols_from_tuple or something like that) for avoiding duplicate code... but that's another patch, is worth the trouble? -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Attachment
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> this one applies, compiles and it actually fixes the bug...
And introduces a bunch of new ones. Surely you don't think that version
of compatible_tupdesc() is good enough.
regards, tom lane
On Mon, Jul 20, 2009 at 9:55 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > Jaime Casanova <jcasanov@systemguards.com.ec> writes: >> this one applies, compiles and it actually fixes the bug... > > And introduces a bunch of new ones. =C2=A0Surely you don't think that ver= sion > of compatible_tupdesc() is good enough. > i guess you're talking about my adapted version for backpatching in 8.3, right? i just was trying to move as less code as possible for 8.3... or there is something wrong in pavel's patch (the one that applies to head)? --=20 Atentamente, Jaime Casanova Soporte y capacitaci=C3=B3n de PostgreSQL Asesor=C3=ADa y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Tom Lane escribió: > Jaime Casanova <jcasanov@systemguards.com.ec> writes: > > this one applies, compiles and it actually fixes the bug... > > And introduces a bunch of new ones. Surely you don't think that version > of compatible_tupdesc() is good enough. Getting rid of the check on natts was "ungood" ... it needs to compare the number of undropped columns of both tupdescs. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Mon, Jul 20, 2009 at 10:09 AM, Alvaro Herrera<alvherre@commandprompt.com> wrote: > Tom Lane escribi=C3=B3: >> Jaime Casanova <jcasanov@systemguards.com.ec> writes: >> > this one applies, compiles and it actually fixes the bug... >> >> And introduces a bunch of new ones. =C2=A0Surely you don't think that ve= rsion >> of compatible_tupdesc() is good enough. > > Getting rid of the check on natts was "ungood" ... it needs to compare > the number of undropped columns of both tupdescs. > ah! ok, i see... i will mark the patch as "waiting on author" and then will try to fix it myself unless pavel wants to do it himself create table test_tbl (a int, b date, c int); create function trg_ins_test_tbl() returns trigger as $$ begin new.c =3D 100; return new; end; $$ language plpgsql; create trigger trg_test_tbl before insert on test_tbl for each row execute procedure trg_ins_test_tbl(); insert into test_tbl(a, b) select i, current_date + i from generate_series(7, 9) as i; alter table test_tbl add column z text; alter table test_tbl drop column z; alter table test_tbl add column z text; insert into test_tbl(a, b) select i, current_date + i from generate_series(7, 9) as i; ERROR: returned row structure does not match the structure of the triggering table DETAIL: Returned type text does not match expected type text in column "z". CONTEXT: PL/pgSQL function "trg_ins_test_tbl" during function exit STATEMENT: insert into test_tbl(a, b) select i, current_date + i from generate_series(7, 9) as i; --=20 Atentamente, Jaime Casanova Soporte y capacitaci=C3=B3n de PostgreSQL Asesor=C3=ADa y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
On Mon, Jul 20, 2009 at 11:34 AM, Jaime Casanova<jcasanov@systemguards.com.ec> wrote: > On Mon, Jul 20, 2009 at 10:09 AM, Alvaro >> >> Getting rid of the check on natts was "ungood" ... it needs to compare >> the number of undropped columns of both tupdescs. >> > > ah! ok, i see... i will mark the patch as "waiting on author" and then > will try to fix it myself unless pavel wants to do it himself > patch attached -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Attachment
2009/7/21 Jaime Casanova <jcasanov@systemguards.com.ec>: > On Mon, Jul 20, 2009 at 11:34 AM, Jaime > Casanova<jcasanov@systemguards.com.ec> wrote: >> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro >>> >>> Getting rid of the check on natts was "ungood" ... it needs to compare >>> the number of undropped columns of both tupdescs. >>> >> >> ah! ok, i see... i will mark the patch as "waiting on author" and then >> will try to fix it myself unless pavel wants to do it himself >> > > patch attached > super, thanks Pavel > > -- > Atentamente, > Jaime Casanova > Soporte y capacitaci=C3=B3n de PostgreSQL > Asesor=C3=ADa y desarrollo de sistemas > Guayaquil - Ecuador > Cel. +59387171157 >
On Tue, Jul 21, 2009 at 2:13 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote: > 2009/7/21 Jaime Casanova <jcasanov@systemguards.com.ec>: >> On Mon, Jul 20, 2009 at 11:34 AM, Jaime >> Casanova<jcasanov@systemguards.com.ec> wrote: >>> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro >>>> >>>> Getting rid of the check on natts was "ungood" ... it needs to compare >>>> the number of undropped columns of both tupdescs. >>>> >>> >>> ah! ok, i see... i will mark the patch as "waiting on author" and then >>> will try to fix it myself unless pavel wants to do it himself >>> >> >> patch attached >> > super, thanks > > Pavel So is this Ready for Committer? ...Robert
On Tue, Jul 28, 2009 at 12:12 PM, Robert Haas<robertmhaas@gmail.com> wrote: >>> >>> patch attached >>> >> super, thanks >> >> Pavel > > So is this Ready for Committer? > i think so... but being me who added the last bit of code i didn't felt confident to mark it as such... --=20 Atentamente, Jaime Casanova Soporte y capacitaci=C3=B3n de PostgreSQL Asesor=C3=ADa y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
On Tue, Jul 28, 2009 at 1:19 PM, Jaime Casanova<jcasanov@systemguards.com.ec> wrote: > On Tue, Jul 28, 2009 at 12:12 PM, Robert Haas<robertmhaas@gmail.com> wrote: >>>> >>>> patch attached >>>> >>> super, thanks >>> >>> Pavel >> >> So is this Ready for Committer? >> > > i think so... but being me who added the last bit of code i didn't > felt confident to mark it as such... Well, perhaps Pavel could do so then, if he has looked it over. ...Robert
2009/7/28 Robert Haas <robertmhaas@gmail.com>: > On Tue, Jul 28, 2009 at 1:19 PM, Jaime > Casanova<jcasanov@systemguards.com.ec> wrote: >> On Tue, Jul 28, 2009 at 12:12 PM, Robert Haas<robertmhaas@gmail.com> wrote: >>>>> >>>>> patch attached >>>>> >>>> super, thanks >>>> >>>> Pavel >>> >>> So is this Ready for Committer? >>> >> >> i think so... but being me who added the last bit of code i didn't >> felt confident to mark it as such... > > Well, perhaps Pavel could do so then, if he has looked it over. > I hope, so last Jaime's patch is final - I didn't find any bug there Pavel > ...Robert >
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
>> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro
>>> Getting rid of the check on natts was "ungood" ... it needs to compare
>>> the number of undropped columns of both tupdescs.
> patch attached
This patch is *still* introducing more bugs than it fixes. The reason
is that it has modified validate_tupdesc_compat to allow the tupdescs to
be somewhat different, but has fixed only one of the several call sites
to deal with the consequences of that. The others will now become crash
risks if we apply it as-is.
What I would suggest as a suitable plan for a fix is to modify
validate_tupdesc_compat so that it returns a flag indicating whether the
tupdesc compatibility is exact or requires translation. If translation
is required, provide another routine that does that --- probably using a
mapping data structure set up by validate_tupdesc_compat, since in some
of these cases we'll be processing many tuples. Then the callers just
have to know enough to call the tuple-translation function when
validate_tupdesc_compat tells them to.
There are a number of other places in the system with similar
requirements, although none of them seem to be exact matches.
In particular ExecEvalConvertRowtype() provides a good template for
efficient translation logic, but it's using column name matching
rather than positional matching so you couldn't share the setup logic.
I'm not sure if it's worth moving all this code into the core so that
it can be shared with other future uses (maybe in other PLs), but it's
worth considering that. access/common/heaptuple.c or tupdesc.c might
be a good place for it if we decide to do that.
The executor's junkfilter code is pretty nearly related as well, and
perhaps the Right Thing is to make all of this stuff into applications
of junkfilters with different setup routines for the different
requirements. However the junkfilter code is designed to work with
tuples that are in TupleTableSlots, which isn't particularly helpful for
plpgsql's uses, so maybe trying to unify with that code is more trouble
than it's worth.
I'm marking this patch as Waiting on Author again, although perhaps
Returned with Feedback would be better since my suggestions amount
to wholesale rewrites.
regards, tom lane
2009/7/30 Tom Lane <tgl@sss.pgh.pa.us>: > Jaime Casanova <jcasanov@systemguards.com.ec> writes: >>> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro >>>> Getting rid of the check on natts was "ungood" ... it needs to compare >>>> the number of undropped columns of both tupdescs. > >> patch attached > > This patch is *still* introducing more bugs than it fixes. =C2=A0The reas= on > is that it has modified validate_tupdesc_compat to allow the tupdescs to > be somewhat different, but has fixed only one of the several call sites > to deal with the consequences of that. =C2=A0The others will now become c= rash > risks if we apply it as-is. > > What I would suggest as a suitable plan for a fix is to modify > validate_tupdesc_compat so that it returns a flag indicating whether the > tupdesc compatibility is exact or requires translation. =C2=A0If translat= ion > is required, provide another routine that does that --- probably using a > mapping data structure set up by validate_tupdesc_compat, since in some > of these cases we'll be processing many tuples. =C2=A0Then the callers ju= st > have to know enough to call the tuple-translation function when > validate_tupdesc_compat tells them to. > ook I'll to try modify patch in this direction. Pavel > There are a number of other places in the system with similar > requirements, although none of them seem to be exact matches. > In particular ExecEvalConvertRowtype() provides a good template for > efficient translation logic, but it's using column name matching > rather than positional matching so you couldn't share the setup logic. > I'm not sure if it's worth moving all this code into the core so that > it can be shared with other future uses (maybe in other PLs), but it's > worth considering that. =C2=A0access/common/heaptuple.c or tupdesc.c might > be a good place for it if we decide to do that. > > The executor's junkfilter code is pretty nearly related as well, and > perhaps the Right Thing is to make all of this stuff into applications > of junkfilters with different setup routines for the different > requirements. =C2=A0However the junkfilter code is designed to work with > tuples that are in TupleTableSlots, which isn't particularly helpful for > plpgsql's uses, so maybe trying to unify with that code is more trouble > than it's worth. > > I'm marking this patch as Waiting on Author again, although perhaps > Returned with Feedback would be better since my suggestions amount > to wholesale rewrites. > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0regards, tom lane >
There is fixed patch. Please, Jaime, can you look on it? Thank You Pavel 2009/7/30 Tom Lane <tgl@sss.pgh.pa.us>: > Jaime Casanova <jcasanov@systemguards.com.ec> writes: >>> On Mon, Jul 20, 2009 at 10:09 AM, Alvaro >>>> Getting rid of the check on natts was "ungood" ... it needs to compare >>>> the number of undropped columns of both tupdescs. > >> patch attached > > This patch is *still* introducing more bugs than it fixes. The reason > is that it has modified validate_tupdesc_compat to allow the tupdescs to > be somewhat different, but has fixed only one of the several call sites > to deal with the consequences of that. The others will now become crash > risks if we apply it as-is. > > What I would suggest as a suitable plan for a fix is to modify > validate_tupdesc_compat so that it returns a flag indicating whether the > tupdesc compatibility is exact or requires translation. If translation > is required, provide another routine that does that --- probably using a > mapping data structure set up by validate_tupdesc_compat, since in some > of these cases we'll be processing many tuples. Then the callers just > have to know enough to call the tuple-translation function when > validate_tupdesc_compat tells them to. > > There are a number of other places in the system with similar > requirements, although none of them seem to be exact matches. > In particular ExecEvalConvertRowtype() provides a good template for > efficient translation logic, but it's using column name matching > rather than positional matching so you couldn't share the setup logic. > I'm not sure if it's worth moving all this code into the core so that > it can be shared with other future uses (maybe in other PLs), but it's > worth considering that. access/common/heaptuple.c or tupdesc.c might > be a good place for it if we decide to do that. > > The executor's junkfilter code is pretty nearly related as well, and > perhaps the Right Thing is to make all of this stuff into applications > of junkfilters with different setup routines for the different > requirements. However the junkfilter code is designed to work with > tuples that are in TupleTableSlots, which isn't particularly helpful for > plpgsql's uses, so maybe trying to unify with that code is more trouble > than it's worth. > > I'm marking this patch as Waiting on Author again, although perhaps > Returned with Feedback would be better since my suggestions amount > to wholesale rewrites. > > regards, tom lane >