Re: 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(at)gmail(dot)com>:
> Sergey Burladyan <eshkinkot(at)gmail(dot)com> writes:
>
>> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

Hello

there is fix for bug Re: [BUGS] BUG #4907: stored procedures and changed tables

regards
Pavel Stehule

2009/7/10 Sergey Burladyan <eshkinkot(at)gmail(dot)com>:
> Sergey Burladyan <eshkinkot(at)gmail(dot)com> writes:
>
>> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>

On Sun, Jul 12, 2009 at 10:28 AM, Pavel Stehule<pavel(dot)stehule(at)gmail(dot)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

Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)systemguards(dot)com(dot)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(at)commandprompt(dot)com> wrote:
> Tom Lane escribió:
>> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)systemguards(dot)com(dot)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

2009/7/21 Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>:
> On Mon, Jul 20, 2009 at 11:34 AM, Jaime
> Casanova<jcasanov(at)systemguards(dot)com(dot)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ó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(dot)stehule(at)gmail(dot)com> wrote:
> 2009/7/21 Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>:
>> On Mon, Jul 20, 2009 at 11:34 AM, Jaime
>> Casanova<jcasanov(at)systemguards(dot)com(dot)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(at)gmail(dot)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(at)systemguards(dot)com(dot)ec> wrote:
> On Tue, Jul 28, 2009 at 12:12 PM, Robert Haas<robertmhaas(at)gmail(dot)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(at)gmail(dot)com>:
> On Tue, Jul 28, 2009 at 1:19 PM, Jaime
> Casanova<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>> On Tue, Jul 28, 2009 at 12:12 PM, Robert Haas<robertmhaas(at)gmail(dot)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(at)systemguards(dot)com(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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
> 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(at)sss(dot)pgh(dot)pa(dot)us>:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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
>

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(at)sss(dot)pgh(dot)pa(dot)us>:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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
>

On Tue, Aug 4, 2009 at 4:30 AM, Pavel Stehule<pavel(dot)stehule(at)gmail(dot)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(dot)stehule(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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