Обсуждение: ECPG dynamic cursor fix for UPDATE/DELETE ... WHERE CURRENT OF :curname

Hi,

attached is a patch that adds the missing feature to use
"WHERE CURRENT OF :curname" in UPDATE and
DELETE statements via ECPG. I used the current CVS MAIN
but also applies almost cleanly to 9.0beta4. I certainly feel that
this should be applied to 9.0 as a bugfix.

The execute.c changes were required because

1. The statement

    UPDATE table SET fld1 = :input1
    WHERE CURRENT OF :curname
    RETURNING id + :input2;

    is transformed into

    UPDATE table SET fld1 = $1
    WHERE CURRENT OF $0
    RETURNING id + $2;

    and the $0 is past $1. The current code cannot deal with such
    a messed up order, and scanning the original query twice is
    needed, once for $0 substitution, once for mapping $1, etc. to
    the other input variables.

2. With such a statement and auto-prepare turned on, I always got

    SQL error: there is no parameter $0 on line X

    It turned out that the statement was prepared by the auto-prepare
machinery
    before the $0 substitution. PostgreSQL allows
        PREPARE mystmt AS UPDATE ... WHERE CURRENT OF mycur
    even if "mycur" is currently unknown to the system. It's resolved upon
    executing the prepared query, so we should allow it in ECPG even with
    dynamic cursorname.

The code survives "make check" and I also went through all the regression
tests manually to check them with valgrind to see that there's no leak.
As a result, another patch is attached that fixes two memory leaks in
PGTYPESnumeric_from_asc() and PGTYPESnumeric_to_long() and
quite some leaks in the regression tests themselves.

Best regards,
Zoltán Böszörményi

Boszormenyi Zoltan <zb@cybertec.at> wrote:
> attached is a patch that adds the missing feature
> I certainly feel that this should be applied to 9.0 as a bugfix.
Those two statements seem to contradict one another.  Is there some
bug manifestation beyond an unimplemented feature this fixes? 
Without this, is there some regression going from 8.4 to 9.0?
-Kevin

Kevin Grittner írta:
> Boszormenyi Zoltan <zb@cybertec.at> wrote:
>  
>   
>> attached is a patch that adds the missing feature
>>     
>  
>   
>> I certainly feel that this should be applied to 9.0 as a bugfix.
>>     
>  
> Those two statements seem to contradict one another.

PostgreSQL 8.3 or so added WHERE CURRENT OF <curname> at the SQL level.

9.0 added ECPG's dynamic cursorname (a char variable carries the actual
cursor name)
but the WHERE CURRENT OF part was not converted, which was definitely an
oversight. Whether this is a "missing feature" or a "bugfix", it's only
a difference in
points of view. I think this patch belongs to 9.0.

>   Is there some
> bug manifestation beyond an unimplemented feature this fixes? 
> Without this, is there some regression going from 8.4 to 9.0?
>   

I haven't looked at 8.4's regression tests but pgtypeslib/numeric.c in
8.4.4 has
the same problem, so the second patch (at least the numeric.c part) can be
applied there as well, maybe in even older branches.

Best regards,
Zoltán Böszörményi

Michael Meskes <michael@fam-meskes.de> wrote:
> I'd consider this a bug.
Could you explain why?  The assertions that people consider it a bug
without explanation of *why* is confusing for me.
It sounds more like a feature of the ECPG interface that people
would really like, and which has been technically possible since
PostgreSQL 8.3, but for which nobody submitted a patch until this
week.  There was some hint that a 9.0 ECPG patch added new features
which might make people expect this feature to have also been added.
If this patch isn't necessarily correct, and would be dangerous to
apply at this point, should the other patch be reverted as something
which shouldn't go out without this feature?
-Kevin

Michael Meskes <michael@fam-meskes.de> wrote:
> All prior ECPG versions were fine because dynamic cursor names
> were only added in 9.0.  Apparently only this one place was
> missed. So this is a bug in the new feature, however not such a
> major one that it warrants the complete removal IMO. I'd prefer to
> fix this in 9.0.1.
> 
> Hope this cleans it up a bit.
Thanks.  I think I get it now.
To restate from another angle, to confirm my understanding: UPDATE
WHERE CURRENT OF is working for cursors with the name hard-coded in
the embedded statement, which is the only way cursor names were
allowed to be specified prior to 9.0; 9.0 implements dynamic cursor
names, allowing you to use a variable for the cursor name; but this
one use of a cursor name isn't allowing a variable yet.  Do I have
it right?  (If so, I now see why it would be considered a bug.)
-Kevin

Kevin Grittner írta:
> Michael Meskes <michael@fam-meskes.de> wrote:
>   
>> All prior ECPG versions were fine because dynamic cursor names
>> were only added in 9.0.  Apparently only this one place was
>> missed. So this is a bug in the new feature, however not such a
>> major one that it warrants the complete removal IMO. I'd prefer to
>> fix this in 9.0.1.
>>     

As we are so late in the beta phase, we can live with that, hopefully
you will find time by then to review the patch which is actually not
that complex, only a bit large. The part of ECPGdo() that deals with
auto-preparing statements is moved closer to calling ecpg_execute(),
after the varargs are converted to stmt->inlist and ->outlist.

>> Hope this cleans it up a bit.
>>     
>  
> Thanks.  I think I get it now.
>  
> To restate from another angle, to confirm my understanding: UPDATE
> WHERE CURRENT OF is working for cursors with the name hard-coded in
> the embedded statement, which is the only way cursor names were
> allowed to be specified prior to 9.0; 9.0 implements dynamic cursor
> names, allowing you to use a variable for the cursor name; but this
> one use of a cursor name isn't allowing a variable yet.  Do I have
> it right?  (If so, I now see why it would be considered a bug.)
>   

Yes, you understand it correctly.

Best regards,
Zoltán Böszörményi

Sorry I thought Zoltan's explanation was clear enough. All prior ECPG versions were fine because dynamic cursor names
wereonly added in 9.0. Apparently only this one place was missed. So this is a bug in the new feature, however not such
amajor one that it warrants the complete removal IMO. I'd prefer to fix this in 9.0.1. 

Hope this cleans it up a bit.

Michael


"Kevin Grittner" <Kevin.Grittner@wicourts.gov> schrieb:

>Michael Meskes <michael@fam-meskes.de> wrote:
>
>> I'd consider this a bug.
>
>Could you explain why?  The assertions that people consider it a bug
>without explanation of *why* is confusing for me.
>
>It sounds more like a feature of the ECPG interface that people
>would really like, and which has been technically possible since
>PostgreSQL 8.3, but for which nobody submitted a patch until this
>week.  There was some hint that a 9.0 ECPG patch added new features
>which might make people expect this feature to have also been added.
>If this patch isn't necessarily correct, and would be dangerous to
>apply at this point, should the other patch be reverted as something
>which shouldn't go out without this feature?
>
>-Kevin

--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Excerpts from Michael Meskes's message of jue ago 05 05:39:46 -0400 2010:
> Sorry I thought Zoltan's explanation was clear enough. All prior ECPG versions were fine because dynamic cursor names
wereonly added in 9.0. Apparently only this one place was missed. So this is a bug in the new feature, however not such
amajor one that it warrants the complete removal IMO. I'd prefer to fix this in 9.0.1.
 

Since we're still in the beta phase, it makes sense to apply the fix
right now so that it appears in 9.0.  No point in waiting for 9.0.1.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera írta:
> Excerpts from Michael Meskes's message of jue ago 05 05:39:46 -0400 2010:
>   
>> Sorry I thought Zoltan's explanation was clear enough. All prior ECPG versions were fine because dynamic cursor
nameswere only added in 9.0. Apparently only this one place was missed. So this is a bug in the new feature, however
notsuch a major one that it warrants the complete removal IMO. I'd prefer to fix this in 9.0.1.
 
>>     
>
> Since we're still in the beta phase, it makes sense to apply the fix
> right now so that it appears in 9.0.  No point in waiting for 9.0.1.
>   

It boils down to the fact that Michael doesn't have too much time
and no one else knows ECPG in depth. So...

Boszormenyi Zoltan <zb@cybertec.at> writes:
> Alvaro Herrera írta:
>> Since we're still in the beta phase, it makes sense to apply the fix
>> right now so that it appears in 9.0.  No point in waiting for 9.0.1.

> It boils down to the fact that Michael doesn't have too much time
> and no one else knows ECPG in depth. So...

Yeah.  I think what Michael is saying is he doesn't have time to review
the patch now and doesn't want to hold up 9.0 until he does.  We can
delay 9.0 for him, or apply the patch unreviewed, or allow 9.0 to go out
with this as a known bug.  I don't much care for #2, given the size of
the patch.
        regards, tom lane

> 1. The statement
> 
>     UPDATE table SET fld1 = :input1
>     WHERE CURRENT OF :curname
>     RETURNING id + :input2;
> 
>     is transformed into
> 
>     UPDATE table SET fld1 = $1
>     WHERE CURRENT OF $0
>     RETURNING id + $2;
> 
>     and the $0 is past $1. The current code cannot deal with such
>     a messed up order, and scanning the original query twice is
>     needed, once for $0 substitution, once for mapping $1, etc. to
>     the other input variables.

I cannot seem to reproduce this bug. Could you please send me an example that
makes this reproducible? Yes, I know that I have to change preproc.y to allow
for variable cursor names but in my test case everything seems to work well and
$0 gets replaced by the cursor name.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Hi,

Michael Meskes írta:
>> 1. The statement
>>
>>     UPDATE table SET fld1 = :input1
>>     WHERE CURRENT OF :curname
>>     RETURNING id + :input2;
>>
>>     is transformed into
>>
>>     UPDATE table SET fld1 = $1
>>     WHERE CURRENT OF $0
>>     RETURNING id + $2;
>>
>>     and the $0 is past $1. The current code cannot deal with such
>>     a messed up order, and scanning the original query twice is
>>     needed, once for $0 substitution, once for mapping $1, etc. to
>>     the other input variables.
>>
>
> I cannot seem to reproduce this bug. Could you please send me an example that
> makes this reproducible? Yes, I know that I have to change preproc.y to allow
> for variable cursor names but in my test case everything seems to work well and
> $0 gets replaced by the cursor name.
>
> Michael
>

sorry for the late answer. Here is a minimal patch against the
current GIT tree, so the WHERE CURRENT OF accepts
dynamic cursornames,  plus the test case that shows the problem.
The problem is caused by line 25 in the attached source, the
UPDATE statement is processed into this code:

                if (sqlca.sqlcode != 0)
                        break;
                { ECPGdo(__LINE__, 1, 1, NULL, 0, ECPGst_prepnormal,
"update t1 set t = $1  where current of $0 returning id + $2 ",
        ECPGt_char,&(new_t),(long)0,(long)1,(1)*sizeof(char),
        ECPGt_NO_INDICATOR, NULL , 0L, 0L, 0L,
        ECPGt_char,&(curname),(long)0,(long)1,(1)*sizeof(char),
        ECPGt_NO_INDICATOR, NULL , 0L, 0L, 0L,
        ECPGt_int,&(one),(long)1,(long)1,sizeof(int),
        ECPGt_NO_INDICATOR, NULL , 0L, 0L, 0L, ECPGt_EOIT,
        ECPGt_int,&(id1),(long)1,(long)1,sizeof(int),
        ECPGt_NO_INDICATOR, NULL , 0L, 0L, 0L, ECPGt_EORT);
#line 25 "where-current-of.pgc"

if (sqlca.sqlcode < 0) sqlprint();}
#line 25 "where-current-of.pgc"

Running the program needs this table:

create table t1 (id serial primary key, t text);

and a few records in it. Result of running it:

$ ./where-current-of
SQL error: there is no parameter $0 on line 25
SQL error: current transaction is aborted, commands ignored until end of
transaction block on line 27
2 0 0 'x'
SQL error: current transaction is aborted, commands ignored until end of
transaction block on line 32

In the above code, the $1, $0, $2 order is correctly mirrored
in the order of the actual parameters.

The DELETE ... WHERE CURRENT OF ... RETURNING ...
grammar wouldn't cause such problem, $0 would be the first
in this case but what do you suggest solving for UPDATE?

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
     http://www.postgresql.at/

Boszormenyi Zoltan wrote:
> Kevin Grittner ?rta:
> > Michael Meskes <michael@fam-meskes.de> wrote:
> >   
> >> All prior ECPG versions were fine because dynamic cursor names
> >> were only added in 9.0.  Apparently only this one place was
> >> missed. So this is a bug in the new feature, however not such a
> >> major one that it warrants the complete removal IMO. I'd prefer to
> >> fix this in 9.0.1.
> >>     
> 
> As we are so late in the beta phase, we can live with that, hopefully
> you will find time by then to review the patch which is actually not
> that complex, only a bit large. The part of ECPGdo() that deals with
> auto-preparing statements is moved closer to calling ecpg_execute(),
> after the varargs are converted to stmt->inlist and ->outlist.
> 
> >> Hope this cleans it up a bit.
> >>     
> >  
> > Thanks.  I think I get it now.
> >  
> > To restate from another angle, to confirm my understanding: UPDATE
> > WHERE CURRENT OF is working for cursors with the name hard-coded in
> > the embedded statement, which is the only way cursor names were
> > allowed to be specified prior to 9.0; 9.0 implements dynamic cursor
> > names, allowing you to use a variable for the cursor name; but this
> > one use of a cursor name isn't allowing a variable yet.  Do I have
> > it right?  (If so, I now see why it would be considered a bug.)
> >   
> 
> Yes, you understand it correctly.

Did this ever get applied?  If so, I can't find it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

> Did this ever get applied?  If so, I can't find it.

No, my bad, I simply forgot about it, sorry.

Will work on this now.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

> sorry for the late answer. Here is a minimal patch against the

My answer is way later, so I have to apologize. 

> current GIT tree, so the WHERE CURRENT OF accepts
> dynamic cursornames,  plus the test case that shows the problem.

It doesn't on my test system. I just committed the minimal patch, so everyone
can test it, but your test case works just nicely on my system.

No idea why your results are different.

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL