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>:
Show quoted text
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:5143So, 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
Attachments:
return_query_fix.difftext/x-patch; charset=US-ASCII; name=return_query_fix.diffDownload+136-43
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>:
Show quoted text
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:5143So, 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
Attachments:
return_query_fix.difftext/x-patch; charset=US-ASCII; name=return_query_fix.diffDownload+136-43
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
Attachments:
return_query_fix-8.3.difftext/x-diff; charset=US-ASCII; name=return_query_fix-8.3.diffDownload+52-32
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. Surely you don't think that version
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)?
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría 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ó:
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.
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 = 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;
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría 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
Attachments:
return_query_fix.v2.patchtext/x-diff; charset=US-ASCII; name=return_query_fix.v2.patchDownload+205-53
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 himselfpatch attached
super, thanks
Pavel
Show quoted text
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría 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 himselfpatch 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...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría 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
Show quoted text
...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. 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.
ook I'll to try modify patch in this direction.
Pavel
Show quoted text
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
There is fixed patch. Please, Jaime, can you look on it?
Thank You
Pavel
2009/7/30 Tom Lane <tgl@sss.pgh.pa.us>:
Show quoted text
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
Attachments:
return_query_fix.v3.patchtext/x-patch; charset=US-ASCII; name=return_query_fix.v3.patchDownload+402-214
forward patch to pg_hackers
There is fixed patch. Please, Jaime, can you look on it?
Thank You
Pavel
2009/7/30 Tom Lane <tgl@sss.pgh.pa.us>:
Show quoted text
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
Attachments:
return_query_fix.v3.patchtext/x-patch; charset=US-ASCII; name=return_query_fix.v3.patchDownload+402-214
On Tue, Aug 4, 2009 at 4:30 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote:
forward patch to pg_hackers
There is fixed patch. Please, Jaime, can you look on it?
this one passed regression tests and my personal test script (of
course it passed the script the last time too)... i'm doing a lot of
alter table [add|drop] column...
it seems it's good enough and is implementing tom's suggestions... can
this be reviewed by a commiter?
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Pavel Stehule <pavel.stehule@gmail.com> writes:
There is fixed patch. Please, Jaime, can you look on it?
Applied with significant revisions. I really wanted this code factored
out, because we'd otherwise end up duplicating it in other PLs (and it
was already duplicative of execQual.c). So I pushed the support code
into a new file tupconvert.c.
regards, tom lane
2009/8/6 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
There is fixed patch. Please, Jaime, can you look on it?
Applied with significant revisions. I really wanted this code factored
out, because we'd otherwise end up duplicating it in other PLs (and it
was already duplicative of execQual.c). So I pushed the support code
into a new file tupconvert.c.regards, tom lane
thank you
Pavel