Re: Split-up ECPG patches

Hi,

as asked by Michael Meskes, I have split up our ECPG patchset:
1. dynamic cursorname (DECLARE :cursorname ..., etc)
2. SQLDA support in Informix compat mode (C structure used for
descriptor and data query)
3. DESCRIBE OUTPUT support for named and sqlda descriptors
4. "string" pseudo-type in Informix compat mode

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

On Wed, Jul 15, 2009 at 07:17:17PM +0200, Böszörményi Zoltán wrote:
> as asked by Michael Meskes, I have split up our ECPG patchset:

Just a couple quick comments.

It appears to me (without much testing) that the patches still partly rely on
each other. But I cannot see a reason for this.

> 1. dynamic cursorname (DECLARE :cursorname ..., etc)
> 2. SQLDA support in Informix compat mode (C structure used for
> descriptor and data query)

One file has this:

* (C) 2009 Cybertec GmbH
* Zolt??n B??sz??rm??nyi <zb(at)cybertec(dot)at>
* Hans-J??rgen Sch??nig <hs(at)cybertec(dot)at>

Shouldn't this also list a license? In general I wonder whether we need some
statement for every patch submitted? Anyone more into licensing might comment
here.

> 3. DESCRIBE OUTPUT support for named and sqlda descriptors

I don't think we have to add ECPGdescribe2 to keep the old API. The old
ECPGdescribe function does nothing, so it's not worth being kept.

> 4. "string" pseudo-type in Informix compat mode

There is still a lot of stuff being done when not in compatibility mode. I
thought you wanted to change that?

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Wed, Jul 15, 2009 at 07:17:17PM +0200, Böszörményi Zoltán wrote:
>
>> as asked by Michael Meskes, I have split up our ECPG patchset:
>>
>
> Just a couple quick comments.
>
> It appears to me (without much testing) that the patches still partly rely on
> each other. But I cannot see a reason for this.
>

There are three reasons:

1. sqlda and string type support both add one constant in ecptypes.h
2. dynamic cursorname and DESCRIBE support both modify the
FetchStmt rule.
3. DESCRIBE support partially builds on sqlda support

I saw no point creating patches that are applicable standalone
when they would conflict each other. The point would be to have
all patches upstreamed, reviewed and applied in the order
indicated by the patch filenames.

Another point was that where to split features?
SQLDA and DESCRIBE [OUTPUT] features overlap.

>> 1. dynamic cursorname (DECLARE :cursorname ..., etc)
>> 2. SQLDA support in Informix compat mode (C structure used for
>> descriptor and data query)
>>
>
> One file has this:
>
> * (C) 2009 Cybertec GmbH
> * Zolt??n B??sz??rm??nyi <zb(at)cybertec(dot)at>
> * Hans-J??rgen Sch??nig <hs(at)cybertec(dot)at>
>
> Shouldn't this also list a license? In general I wonder whether we need some
> statement for every patch submitted? Anyone more into licensing might comment
> here.
>

What is the correct way to indicate that the licence is the same
as the PostgreSQL licence but we are the authors? We aren't
license experts. :-)

>> 3. DESCRIBE OUTPUT support for named and sqlda descriptors
>>
>
> I don't think we have to add ECPGdescribe2 to keep the old API. The old
> ECPGdescribe function does nothing, so it's not worth being kept.
>

I thought about easing transition and letting old binaries work as is.
IIRC a common wisdom is that if you add API calls, you only need to
increase the minor library version. But if you modify an existing
call you create an incompatibility (even when the usage of said call
was unlikely) and the major library version need to be increased.

>> 4. "string" pseudo-type in Informix compat mode
>>
>
> There is still a lot of stuff being done when not in compatibility mode. I
> thought you wanted to change that?
>

The things is that in Informix mode, the patch refuses
"typedef ... string;", in native mode it lets "string"
typename through. "make check" under ecpg passes. Isn't that
enough? Is there a particular place you didn't like?

Thanks for the review so far.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Hi,

new versions attached, updated to apply to the current CVS cleanly.

Changes:
- we fixed the case in the dynamic cursor support when
the cursor was declared in a way that the query contained
a WHERE clause, using host variables as parameters.
(leftover bug from our first, non-public, non-parser-only attempt)
- sqlda support:
- sqlda.c now indicates license
- #defines inside #if 0 ... #endif are now omitted from sqltypes.h
(both per comments from Jaime Casanova)
- describe support: there's no separate ECPGdescribe2() now
(as per comment from Michael Meskes, debatable)
- string support: I am doing much less now unconditionally,
most of the parser changes (e.g. introducing STRING_P)
were unnecessary to make it working.
- added to the list my second attempt at fixing struct variable
in INTO list in Informix-mode. This fix (or the real one if
this is not the right approach) should be backpatched,
because the bug is real.

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

Böszörményi Zoltán írta:
> Hi,
>
> as asked by Michael Meskes, I have split up our ECPG patchset:
> 1. dynamic cursorname (DECLARE :cursorname ..., etc)
> 2. SQLDA support in Informix compat mode (C structure used for
> descriptor and data query)
> 3. DESCRIBE OUTPUT support for named and sqlda descriptors
> 4. "string" pseudo-type in Informix compat mode
>
> Best regards,
> Zoltán Böszörményi
>
> ------------------------------------------------------------------------
>
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> new versions attached, updated to apply to the current CVS cleanly.

Why is this messing with the core grammar?

regards, tom lane

Tom Lane írta:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>
>> new versions attached, updated to apply to the current CVS cleanly.
>>
>
> Why is this messing with the core grammar?
>
> regards, tom lane
>

It was explained in earlier mails, let me explain it again
but a bit more verbosely.

This was the evolution of my approaches adding the
dynamic cursorname:

At first, I changed ECPG grammar only. This meant adding
one new rule for every rule dealing with FETCH or
MOVE in the ECPG grammar. It turned out quickly,
that these two rules below:
FETCH fetch_direction from_in cursor_name ecpg_into
MOVE fetch_direction from_in cursor_name
created shift/reduce conflicts in fetch_direction. This conflict
could have been solved by decreasing factorization of
"fetch_direction", pulling "BACKWARD" and "FORWARD"
(without the row number indicator) up to the rules using
"fetch_direction". Which could be solved by two ways.

1. Leave the original (auto-generated) rules alone and add
the new ones together with a new fetch_direction2 rule
used by the dynamic cursorname FETCH/MOVE rules.
At this point the FETCH/MOVE ruleset was so proliferated
that my eyes started aching just by looking at it. And as
Michael Meskes said, it was a great step back in the ECPG
auto-generated grammar introduced in 8.4.

2. With the first approach being a dead-end, I tried to unify
the two ruleset (FETCH/MOVE with and without dynamic
cursorname variable). But it only worked if I add these changes:
- add the "cursor_name" rule to the main grammar, so it can be
picked up by the auto-generated ECPG grammar
- "cursor_name" has a new subrule dealing with host variables in ECPG
- the above solution for solving the shift/reduce problems in *ECPG*,
needed the change in the main grammar, so auto-generation still
works, and both the main grammar and the ECPG grammar are clean.

I hope I explained it well, the first approach would have made
the autogenerated ECPG grammar very unclean, you would have
rejected immediately seeing that proposed.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Mon, Aug 03, 2009 at 06:12:43PM +0200, Boszormenyi Zoltan wrote:
> - string support: I am doing much less now unconditionally,
> most of the parser changes (e.g. introducing STRING_P)
> were unnecessary to make it working.

I took the freedom to rewrite some parts of this patch and the commit it. Not
having read the Informix specs I might have broken something, so please test.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Mon, Aug 03, 2009 at 06:12:43PM +0200, Boszormenyi Zoltan wrote:
>
>> - string support: I am doing much less now unconditionally,
>> most of the parser changes (e.g. introducing STRING_P)
>> were unnecessary to make it working.
>>
>
> I took the freedom to rewrite some parts of this patch and the commit it. Not
> having read the Informix specs I might have broken something, so please test.
>

Hey, thanks. Did you only commit this, or all of
those in the patchset? Exluding the nonfix for
the struct problem of course.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Fri, Aug 07, 2009 at 01:05:33PM +0200, Boszormenyi Zoltan wrote:
> Hey, thanks. Did you only commit this, or all of
> those in the patchset? Exluding the nonfix for
> the struct problem of course.

So far only this. So there are three more left to go. :-)

Did you take care of the copyright/licensing issue with sqlda?

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Fri, Aug 07, 2009 at 01:05:33PM +0200, Boszormenyi Zoltan wrote:
>
>> Hey, thanks. Did you only commit this, or all of
>> those in the patchset? Exluding the nonfix for
>> the struct problem of course.
>>
>
> So far only this. So there are three more left to go. :-)
>

Okay :-)

> Did you take care of the copyright/licensing issue with sqlda?
>

I added notice about the PostgreSQL license. Is it ok?
Or should I resend without indicating the authors?

Thanks,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan wrote:
> Michael Meskes írta:

> > Did you take care of the copyright/licensing issue with sqlda?
>
> I added notice about the PostgreSQL license. Is it ok?
> Or should I resend without indicating the authors?

I think we're normally OK with mentioning the authors, i.e. "Author:
such and such", but the (C) line should attribute copyright to UCB/PGDG.
Michael is free to dictate something else for ECPG of course ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, Aug 07, 2009 at 12:08:00PM -0400, Alvaro Herrera wrote:
> I think we're normally OK with mentioning the authors, i.e. "Author:

Yes, it's OK, but I think we normally only acknowledge the author in our commit
messages, don't we?

> such and such", but the (C) line should attribute copyright to UCB/PGDG.
> Michael is free to dictate something else for ECPG of course ...

No special rule for ecpg, I absolutely agree.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

On Fri, Aug 07, 2009 at 04:03:38PM +0200, Boszormenyi Zoltan wrote:
> I added notice about the PostgreSQL license. Is it ok?
> Or should I resend without indicating the authors?

Normally all our source files are "Copyright PostgreSQL Global Development
Group" and I don't see a reason why this should be handled differently.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Fri, Aug 07, 2009 at 12:08:00PM -0400, Alvaro Herrera wrote:
>
>> I think we're normally OK with mentioning the authors, i.e. "Author:
>>
>
> Yes, it's OK, but I think we normally only acknowledge the author in our commit
> messages, don't we?
>
>
>> such and such", but the (C) line should attribute copyright to UCB/PGDG.
>> Michael is free to dictate something else for ECPG of course ...
>>
>
> No special rule for ecpg, I absolutely agree.
>
> Michael
>

OK, I can resend if you want. Or feel free
to delete the (C) lines from the sqlda.c file.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Mon, Aug 03, 2009 at 06:59:30PM +0200, Boszormenyi Zoltan wrote:
> > Why is this messing with the core grammar?
> ...

Zoltan, could you please explain why you unrolled FORWARD and BACKWARD? I tried
applying the rest of your patch, without this unrolling but didn't get any
shift/reduce problem. Might have been that I missed something, so could you please try again?

Tom, AFAICT we only need one core grammar change, moving the cursor name to
it's own rule that only resolves back to name. This rule should be eliminated
by bison during the build process anyway, so I see no problem adding it. It
does make the ecpg changes way smaller though. Is this okay with you?

Zoltan, two more things about this patch need to be cleared:
- I don't think your code is able to handle varchars.
- There is no test. Please add this to some of our test cases or write a new one.

Some variable handling commands look suspicious to me, a test case might
alleviate my concerns.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)org> writes:
> Tom, AFAICT we only need one core grammar change, moving the cursor name to
> it's own rule that only resolves back to name. This rule should be eliminated
> by bison during the build process anyway, so I see no problem adding it. It
> does make the ecpg changes way smaller though. Is this okay with you?

Sure, that one didn't bother me. It was the FORWARD/BACKWARD decomposition
that looked unnecessary (as your tests seem to bear out).

regards, tom lane

Michael Meskes írta:
> On Mon, Aug 03, 2009 at 06:59:30PM +0200, Boszormenyi Zoltan wrote:
>
>>> Why is this messing with the core grammar?
>>>
>> ...
>>
>
> Zoltan, could you please explain why you unrolled FORWARD and BACKWARD? I tried
> applying the rest of your patch, without this unrolling but didn't get any
> shift/reduce problem. Might have been that I missed something, so could you please try again?
>

Without a re-quoted explanation, please, compare
your modified patch with the attached one. I rolled
FORWARD and BACKWARD back into fetch_direction
in the core grammar, deleting the newly introduced FETCH
and MOVE rules from the core and ECPG grammar and
again I got this during the ECPG grammar compilation:

...
"/usr/bin/perl" ./parse.pl . < ../../../../src/backend/parser/gram.y >
preproc.y
/usr/bin/bison -d -o preproc.c preproc.y
preproc.y: conflicts: 2 shift/reduce
preproc.y: expected 0 shift/reduce conflicts
make[4]: *** [preproc.c] Error 1
make[4]: Leaving directory
`/home/zozo/Schönig-számlák/leoni/2/pgsql/src/interfaces/ecpg/preproc'
...

FYI:

$ rpm -q bison flex
bison-2.3-5.fc9.x86_64
flex-2.5.35-2.fc9.x86_64

> Tom, AFAICT we only need one core grammar change, moving the cursor name to
> it's own rule that only resolves back to name. This rule should be eliminated
> by bison during the build process anyway, so I see no problem adding it. It
> does make the ecpg changes way smaller though. Is this okay with you?
>
> Zoltan, two more things about this patch need to be cleared:
> - I don't think your code is able to handle varchars.
>

I will test that, thanks.

> - There is no test. Please add this to some of our test cases or write a new one.
>

I will write some regression tests, of course.

> Some variable handling commands look suspicious to me, a test case might
> alleviate my concerns.
>

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Tom Lane írta:
> Michael Meskes <meskes(at)postgresql(dot)org> writes:
>
>> Tom, AFAICT we only need one core grammar change, moving the cursor name to
>> it's own rule that only resolves back to name. This rule should be eliminated
>> by bison during the build process anyway, so I see no problem adding it. It
>> does make the ecpg changes way smaller though. Is this okay with you?
>>
>
> Sure, that one didn't bother me. It was the FORWARD/BACKWARD decomposition
> that looked unnecessary (as your tests seem to bear out).
>

Of course, that one bothered me as well.
Please, test the attached patch in my other mail.

I would like to know what bison version does
Michael use, maybe some difference from my
bison-2.3 might explain his test result. Tom,
you surely know more about bison releases
and its grammar changes than me, you might
give me some enlightenment on this issue.
It might turn out that my Fedora 9 bison is not bugfree.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Sat, Aug 08, 2009 at 05:48:57PM +0200, Boszormenyi Zoltan wrote:
> ...
> "/usr/bin/perl" ./parse.pl . < ../../../../src/backend/parser/gram.y >
> preproc.y
> /usr/bin/bison -d -o preproc.c preproc.y
> preproc.y: conflicts: 2 shift/reduce
> preproc.y: expected 0 shift/reduce conflicts
> make[4]: *** [preproc.c] Error 1
> make[4]: Leaving directory
> `/home/zozo/Schönig-számlák/leoni/2/pgsql/src/interfaces/ecpg/preproc'
> ...

Right, I missed this one. But it's ecpg specific and should not be fixed by
changing gram.y. The problem is that SignedIconst might be a char variable,
too. So how shall the parser know whether str in "FETCH BACKWARD :str" carries
the number of records to move backwards ot the cursor name. A possible solution
would be to force a numeric variable for numeric data. Also keep in mind that a
fetch statement without from/in is an addition on top of the standard. I'm not
sure if any other dbms still allows this construct, so we might we well remove
it for 8.5. Or move it to a special compatibility mode.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Sat, Aug 08, 2009 at 05:48:57PM +0200, Boszormenyi Zoltan wrote:
>
>> ...
>> "/usr/bin/perl" ./parse.pl . < ../../../../src/backend/parser/gram.y >
>> preproc.y
>> /usr/bin/bison -d -o preproc.c preproc.y
>> preproc.y: conflicts: 2 shift/reduce
>> preproc.y: expected 0 shift/reduce conflicts
>> make[4]: *** [preproc.c] Error 1
>> make[4]: Leaving directory
>> `/home/zozo/Schönig-számlák/leoni/2/pgsql/src/interfaces/ecpg/preproc'
>> ...
>>
>
> Right, I missed this one. But it's ecpg specific and should not be fixed by
> changing gram.y.

Debatable. :-)

> The problem is that SignedIconst might be a char variable,
> too. So how shall the parser know whether str in "FETCH BACKWARD :str" carries
> the number of records to move backwards ot the cursor name.

This was the problem, yes.

> A possible solution
> would be to force a numeric variable for numeric data.

By which you would remove a feature.
With the proposed core grammar change,
the feature where you can pass the number of
records to be fetched in a string variable
can be kept.

> Also keep in mind that a
> fetch statement without from/in is an addition on top of the standard.

It seems to be Informix-specific, I just looked it up in
their guide_to_sql_syntax.pdf.

> I'm not
> sure if any other dbms still allows this construct, so we might we well remove
> it for 8.5. Or move it to a special compatibility mode.
>

How would you do that? With a completely
different parser for Informix-compatibility?
It would reduce maintainability. Or does bison
allow conditionally enabled rules somehow?
It sure would come in handy in this case.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Sat, Aug 08, 2009 at 07:21:59PM +0200, Boszormenyi Zoltan wrote:
> > A possible solution
> > would be to force a numeric variable for numeric data.
>
> By which you would remove a feature.
> With the proposed core grammar change,
> the feature where you can pass the number of
> records to be fetched in a string variable
> can be kept.

Somehow I doubt this. Yes, your patch originally didn't come with a
shift/reduce problem, but I cannot see how this solved this. The same rule
still has the same problem.

> It seems to be Informix-specific, I just looked it up in
> their guide_to_sql_syntax.pdf.

The questin is, does Oracle so somthing similar?

> > I'm not
> > sure if any other dbms still allows this construct, so we might we well remove
> > it for 8.5. Or move it to a special compatibility mode.
> >
>
> How would you do that? With a completely
> different parser for Informix-compatibility?

No.

> It would reduce maintainability. Or does bison
> allow conditionally enabled rules somehow?
> It sure would come in handy in this case.

No. You have to code around it. What I meant was, that other dbms should have
the same problem. So they solved it one way or the other. And we could create
both solutions just depending on the mode we're in. Informix e.g. doesn't seem
to allow a variable to carry the number of records. Heck, I don't even see
FORWARD/BACKWARD. A number is only given in ABSOLUTE and RELATIVE but no
variable.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Sat, Aug 08, 2009 at 07:21:59PM +0200, Boszormenyi Zoltan wrote:
>
>>> A possible solution
>>> would be to force a numeric variable for numeric data.
>>>
>> By which you would remove a feature.
>> With the proposed core grammar change,
>> the feature where you can pass the number of
>> records to be fetched in a string variable
>> can be kept.
>>
>
> Somehow I doubt this. Yes, your patch originally didn't come with a
> shift/reduce problem, but I cannot see how this solved this. The same rule
> still has the same problem.
>

Err, no. E.g. if the whole statement is
FETCH BACKWARD cursor_name
then it can only carry a cursor name, as always did.
No matter if cursor_name is now a static or dynamic name.
The problem is with the original factorization of
"fetch_direction", now with dynamic cursor name
the grammar cannot decide between
"FETCH BACKWARD :no_of_rec ..."
and
"FETCH BACKWARD :cursor"
Same with the FORWARD rule. And _that_ can be solved
by decreasing factorization, i.e. pulling FORWARD and
BACKWARD up into the FetchStmt rule in the core grammar.

>> It seems to be Informix-specific, I just looked it up in
>> their guide_to_sql_syntax.pdf.
>>
>
> The questin is, does Oracle so somthing similar?
>

No idea. I can look up though.

>>> I'm not
>>> sure if any other dbms still allows this construct, so we might we well remove
>>> it for 8.5. Or move it to a special compatibility mode.
>>>
>>>
>> How would you do that? With a completely
>> different parser for Informix-compatibility?
>>
>
> No.
>
>
>> It would reduce maintainability. Or does bison
>> allow conditionally enabled rules somehow?
>> It sure would come in handy in this case.
>>
>
> No. You have to code around it. What I meant was, that other dbms should have
> the same problem. So they solved it one way or the other. And we could create
> both solutions just depending on the mode we're in. Informix e.g. doesn't seem
> to allow a variable to carry the number of records. Heck, I don't even see
> FORWARD/BACKWARD. A number is only given in ABSOLUTE and RELATIVE but no
> variable.
>
> Michael
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> Michael Meskes rta:
>> The problem is that SignedIconst might be a char variable,
>> too. So how shall the parser know whether str in "FETCH BACKWARD :str" carries
>> the number of records to move backwards ot the cursor name.

> This was the problem, yes.

>> A possible solution
>> would be to force a numeric variable for numeric data.

> By which you would remove a feature.

If you ask me, the real problem here is the productions ecpg adds to
make "from_in" optional. If a CVARIABLE can be either a fetch_count
or a cursor_name, then removing from_in makes the grammar fundamentally
ambiguous; no amount of rearrangement will fix that.

I'd look at requiring from_in as being the least-bad alternative. What
I now see is that Zoltan's previous patch is removing a different subset
of the possible parses (and has to modify the core grammar in order to
be able to do that); to wit, it's arbitrarily deciding that "FETCH
FORWARD variable" must be a cursor name variable and not a row count
variable. That strikes me as a non-orthogonal, error-prone kluge.

regards, tom lane

Tom Lane írta:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>
>> Michael Meskes Ă­rta:
>>
>>> The problem is that SignedIconst might be a char variable,
>>> too. So how shall the parser know whether str in "FETCH BACKWARD :str" carries
>>> the number of records to move backwards ot the cursor name.
>>>
>
>
>> This was the problem, yes.
>>
>
>
>>> A possible solution
>>> would be to force a numeric variable for numeric data.
>>>
>
>
>> By which you would remove a feature.
>>
>
> If you ask me, the real problem here is the productions ecpg adds to
> make "from_in" optional. If a CVARIABLE can be either a fetch_count
> or a cursor_name, then removing from_in makes the grammar fundamentally
> ambiguous; no amount of rearrangement will fix that.
>
> I'd look at requiring from_in as being the least-bad alternative. What
> I now see is that Zoltan's previous patch is removing a different subset
> of the possible parses (and has to modify the core grammar in order to
> be able to do that); to wit, it's arbitrarily deciding that "FETCH
> FORWARD variable" must be a cursor name variable and not a row count
> variable. That strikes me as a non-orthogonal, error-prone kluge.
>

Hm. "FETCH FORWARD variable" can only be a rowcount var
only if there's something afterwards, no? With the proposed
change in fetch_direction (moving FORWARD and BACKWARD
without the rowcount upper to the parent rules) now the parser is
able to look behind "FORWARD variable"...

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
> Tom Lane rta:
>> I'd look at requiring from_in as being the least-bad alternative.

> Hm. "FETCH FORWARD variable" can only be a rowcount var
> only if there's something afterwards, no? With the proposed
> change in fetch_direction (moving FORWARD and BACKWARD
> without the rowcount upper to the parent rules) now the parser is
> able to look behind "FORWARD variable"...

The fundamental reason that there's a problem here is that ecpg has
decided to accept a syntax that the backend doesn't (ie, FETCH with a
fetch direction but no FROM/IN). I think that that's basically a bad
idea: it's not helpful to users to be inconsistent, and it requires ugly
hacks in ecpg, and now ugly hacks in the core grammar as well. We
should resolve it either by taking out that syntax from ecpg, or by
making the backend accept it too. Not by uglifying the grammars some
more in order to keep them inconsistent.

If we were going to allow it in the core, I think moving the cursor
name into the fetch_direction production might work, ie, change
fetch_direction to fetch_args and make it cover everything that
FETCH and MOVE share. Probably from_in could become opt_from_in,
since the alternatives for it are fully reserved words already, and we
wouldn't need to double up any of the fetch_direction productions.

regards, tom lane

I wrote:
> The fundamental reason that there's a problem here is that ecpg has
> decided to accept a syntax that the backend doesn't (ie, FETCH with a
> fetch direction but no FROM/IN). I think that that's basically a bad
> idea: it's not helpful to users to be inconsistent, and it requires ugly
> hacks in ecpg, and now ugly hacks in the core grammar as well. We
> should resolve it either by taking out that syntax from ecpg, or by
> making the backend accept it too. Not by uglifying the grammars some
> more in order to keep them inconsistent.

On looking a bit closer at this: I think the reason the core grammar
requires FROM/IN after fetch_direction is to leave the door open for
someday generalizing the fetch count to be an expression, not just an
integer constant. If we made FROM/IN optional, then doing that would
require some ugly syntax hack or other, such as requiring parentheses
around nontrivial expressions. So I'd like to see an actual case made
that there's a strong reason for not requiring FROM/IN in ecpg.

regards, tom lane

Tom Lane írta:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>
>> Tom Lane írta:
>>
>>> I'd look at requiring from_in as being the least-bad alternative.
>>>
>
>
>> Hm. "FETCH FORWARD variable" can only be a rowcount var
>> only if there's something afterwards, no? With the proposed
>> change in fetch_direction (moving FORWARD and BACKWARD
>> without the rowcount upper to the parent rules) now the parser is
>> able to look behind "FORWARD variable"...
>>
>
> The fundamental reason that there's a problem here is that ecpg has
> decided to accept a syntax that the backend doesn't (ie, FETCH with a
> fetch direction but no FROM/IN). I think that that's basically a bad
> idea: it's not helpful to users to be inconsistent, and it requires ugly
> hacks in ecpg, and now ugly hacks in the core grammar as well. We
> should resolve it either by taking out that syntax from ecpg, or by
> making the backend accept it too. Not by uglifying the grammars some
> more in order to keep them inconsistent.
>
> If we were going to allow it in the core, I think moving the cursor
> name into the fetch_direction production might work, ie, change
> fetch_direction to fetch_args and make it cover everything that
> FETCH and MOVE share. Probably from_in could become opt_from_in,
> since the alternatives for it are fully reserved words already, and we
> wouldn't need to double up any of the fetch_direction productions.
>

And maybe, possibly with this change as a start, someday
we can support dynamic cursorname in plain SQL, too.
DECLARE $1 CURSOR FOR SELECT ...
It would be a much cleaner solution in ECPG, too.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Tom Lane írta:
> I wrote:
>
>> The fundamental reason that there's a problem here is that ecpg has
>> decided to accept a syntax that the backend doesn't (ie, FETCH with a
>> fetch direction but no FROM/IN). I think that that's basically a bad
>> idea: it's not helpful to users to be inconsistent, and it requires ugly
>> hacks in ecpg, and now ugly hacks in the core grammar as well. We
>> should resolve it either by taking out that syntax from ecpg, or by
>> making the backend accept it too. Not by uglifying the grammars some
>> more in order to keep them inconsistent.
>>
>
> On looking a bit closer at this: I think the reason the core grammar
> requires FROM/IN after fetch_direction is to leave the door open for
> someday generalizing the fetch count to be an expression, not just an
> integer constant. If we made FROM/IN optional, then doing that would
> require some ugly syntax hack or other, such as requiring parentheses
> around nontrivial expressions. So I'd like to see an actual case made
> that there's a strong reason for not requiring FROM/IN in ecpg.
>
> regards, tom lane
>

The only reason is I think was the Informix-compatible mode.
I don't know if it's strong enough, though.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan írta:
> Tom Lane írta:
>
>> I wrote:
>>
>>
>>> The fundamental reason that there's a problem here is that ecpg has
>>> decided to accept a syntax that the backend doesn't (ie, FETCH with a
>>> fetch direction but no FROM/IN). I think that that's basically a bad
>>> idea: it's not helpful to users to be inconsistent, and it requires ugly
>>> hacks in ecpg, and now ugly hacks in the core grammar as well. We
>>> should resolve it either by taking out that syntax from ecpg, or by
>>> making the backend accept it too. Not by uglifying the grammars some
>>> more in order to keep them inconsistent.
>>>
>>>
>> On looking a bit closer at this: I think the reason the core grammar
>> requires FROM/IN after fetch_direction is to leave the door open for
>> someday generalizing the fetch count to be an expression, not just an
>> integer constant. If we made FROM/IN optional, then doing that would
>> require some ugly syntax hack or other, such as requiring parentheses
>> around nontrivial expressions. So I'd like to see an actual case made
>> that there's a strong reason for not requiring FROM/IN in ecpg.
>>
>> regards, tom lane
>>
>>
>
> The only reason is I think was the Informix-compatible mode.
> I don't know if it's strong enough, though.
>

I am looking at the original gram.y and there's a special cased
"optional FROM/IN" case already for FETCH and MOVE:
"FETCH name" and "MOVE name". Which can be seen
as optional FORWARD (already in fetch_direction, as its
first rule) and optional FROM/IN.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Tom Lane írta:
> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>
>> Tom Lane írta:
>>
>>> I'd look at requiring from_in as being the least-bad alternative.
>>>
>
>
>> Hm. "FETCH FORWARD variable" can only be a rowcount var
>> only if there's something afterwards, no? With the proposed
>> change in fetch_direction (moving FORWARD and BACKWARD
>> without the rowcount upper to the parent rules) now the parser is
>> able to look behind "FORWARD variable"...
>>
>
> The fundamental reason that there's a problem here is that ecpg has
> decided to accept a syntax that the backend doesn't (ie, FETCH with a
> fetch direction but no FROM/IN). I think that that's basically a bad
> idea: it's not helpful to users to be inconsistent, and it requires ugly
> hacks in ecpg, and now ugly hacks in the core grammar as well. We
> should resolve it either by taking out that syntax from ecpg, or by
> making the backend accept it too. Not by uglifying the grammars some
> more in order to keep them inconsistent.
>
> If we were going to allow it in the core, I think moving the cursor
> name into the fetch_direction production might work, ie, change
> fetch_direction to fetch_args and make it cover everything that
> FETCH and MOVE share. Probably from_in could become opt_from_in,
> since the alternatives for it are fully reserved words already, and we
> wouldn't need to double up any of the fetch_direction productions.
>
> regards, tom lane
>

Your guess about making from_in into opt_from_in
seems good, mostly. I tried doing exactly that and simply
adding an empty match into from_in, I got shift/reduce conflicts
only in "opt_from_in cursor_name". So, this rule has to be
unrolled into 3 rules, or keeping a separate "from_in" just for
having a separate "cursor_name" and "from_in cursor_name".
I decided that I use the second method, it's shorter.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Sat, Aug 08, 2009 at 04:57:57PM -0400, Tom Lane wrote:
> The fundamental reason that there's a problem here is that ecpg has
> decided to accept a syntax that the backend doesn't (ie, FETCH with a
> fetch direction but no FROM/IN). I think that that's basically a bad

Which was added because most if not all other precompilers allow this syntax
and of course it didn't do any harm until now.

> idea: it's not helpful to users to be inconsistent, and it requires ugly
> hacks in ecpg, and now ugly hacks in the core grammar as well. We
> should resolve it either by taking out that syntax from ecpg, or by
> making the backend accept it too. Not by uglifying the grammars some
> more in order to keep them inconsistent.

Couldn't agree more.

I'd like to figure out exactly what syntax other DBMSes accept. It appears
Informix allows the cursor name as a variable but has neither FORWARD/BACKWARD
nor FROM/IN. Zoltan, could you please check whether my docs are right?

A quick google search seems to suggest that the same holds for Oracle that
apparently allows less options.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

On Sat, Aug 08, 2009 at 05:29:06PM -0400, Tom Lane wrote:
> around nontrivial expressions. So I'd like to see an actual case made
> that there's a strong reason for not requiring FROM/IN in ecpg.

I guess there's only one, compatibility.

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes(at)postgresql(dot)org> writes:
> On Sat, Aug 08, 2009 at 05:29:06PM -0400, Tom Lane wrote:
>> around nontrivial expressions. So I'd like to see an actual case made
>> that there's a strong reason for not requiring FROM/IN in ecpg.

> I guess there's only one, compatibility.

Yeah. Are there any other precompilers that actively reject FROM/IN
here? If we're just a bit more strict than they are, it's not as bad
as if there is no common syntax subset.

regards, tom lane

Michael Meskes írta:
> On Sat, Aug 08, 2009 at 04:57:57PM -0400, Tom Lane wrote:
>
>> The fundamental reason that there's a problem here is that ecpg has
>> decided to accept a syntax that the backend doesn't (ie, FETCH with a
>> fetch direction but no FROM/IN). I think that that's basically a bad
>>
>
> Which was added because most if not all other precompilers allow this syntax
> and of course it didn't do any harm until now.
>

:-( Why me? ;-)

>> idea: it's not helpful to users to be inconsistent, and it requires ugly
>> hacks in ecpg, and now ugly hacks in the core grammar as well. We
>> should resolve it either by taking out that syntax from ecpg, or by
>> making the backend accept it too. Not by uglifying the grammars some
>> more in order to keep them inconsistent.
>>
>
> Couldn't agree more.
>
> I'd like to figure out exactly what syntax other DBMSes accept. It appears
> Informix allows the cursor name as a variable but has neither FORWARD/BACKWARD
> nor FROM/IN. Zoltan, could you please check whether my docs are right?
>

Yes, your docs seems to be right. From my docs, Informix allows these:

FETCH
{ [NEXT] | PRIOR | PREVIOUS | FIRST | LAST | CURRENT |
ABSOLUTE pos_var_or_const |
RELATIVE { [+]pos_var_or_const | -pos_const }
}
{ cursor_id | cursor_var }
{ USING [SQL] DESCRIPTOR ... | INTO host_var_list... }

There's no FROM or IN anywhere in the syntax snake maze graph.

> A quick google search seems to suggest that the same holds for Oracle that
> apparently allows less options.
>
> Michael
>

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan írta:
> Tom Lane írta:
>
>> Boszormenyi Zoltan <zb(at)cybertec(dot)at> writes:
>>
>>
>>> Tom Lane írta:
>>>
>>>
>>>> I'd look at requiring from_in as being the least-bad alternative.
>>>>
>>>>
>>
>>
>>> Hm. "FETCH FORWARD variable" can only be a rowcount var
>>> only if there's something afterwards, no? With the proposed
>>> change in fetch_direction (moving FORWARD and BACKWARD
>>> without the rowcount upper to the parent rules) now the parser is
>>> able to look behind "FORWARD variable"...
>>>
>>>
>> The fundamental reason that there's a problem here is that ecpg has
>> decided to accept a syntax that the backend doesn't (ie, FETCH with a
>> fetch direction but no FROM/IN). I think that that's basically a bad
>> idea: it's not helpful to users to be inconsistent, and it requires ugly
>> hacks in ecpg, and now ugly hacks in the core grammar as well. We
>> should resolve it either by taking out that syntax from ecpg, or by
>> making the backend accept it too. Not by uglifying the grammars some
>> more in order to keep them inconsistent.
>>
>> If we were going to allow it in the core, I think moving the cursor
>> name into the fetch_direction production might work, ie, change
>> fetch_direction to fetch_args and make it cover everything that
>> FETCH and MOVE share. Probably from_in could become opt_from_in,
>> since the alternatives for it are fully reserved words already, and we
>> wouldn't need to double up any of the fetch_direction productions.
>>
>> regards, tom lane
>>
>>
>
> Your guess about making from_in into opt_from_in
> seems good, mostly. I tried doing exactly that and simply
> adding an empty match into from_in, I got shift/reduce conflicts
> only in "opt_from_in cursor_name". So, this rule has to be
> unrolled into 3 rules, or keeping a separate "from_in" just for
> having a separate "cursor_name" and "from_in cursor_name".
> I decided that I use the second method, it's shorter.
>

OK, here's the WIP patch for the unified core/ecpg grammar,
with opt_from_in. But I am still getting the 2 shift/reduce
conflicts exactly for the FORWARD and BACKWARD rules
that I was getting originally. Can you look at this patch and
point me to the right direction in solving it? Thanks in advance.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Tom Lane wrote:
>>> So I'd like to see an actual case made
>>> that there's a strong reason for not requiring FROM/IN in ecpg.
>>
>> I guess there's only one, compatibility.
>
> Yeah. Are there any other precompilers that actively reject FROM/IN
> here? If we're just a bit more strict than they are, it's not as bad
> as if there is no common syntax subset.

Oracle:

http://download.oracle.com/docs/cd/B28359_01/appdev.111/b28427/pc_afemb.htm#i9340

Yours,
Laurenz Albe

Boszormenyi Zoltan írta:
> OK, here's the WIP patch for the unified core/ecpg grammar,
> with opt_from_in. But I am still getting the 2 shift/reduce
> conflicts exactly for the FORWARD and BACKWARD rules
> that I was getting originally. Can you look at this patch and
> point me to the right direction in solving it? Thanks in advance.

Okay, I seem to start to succeed with the following strategy.
In ECPG, there's the possibility to ignore certain rules.
I just added these two lines to parse.pl:

$replace_line{'fetch_argsFORWARDopt_from_incursor_name'} = 'ignore';
$replace_line{'fetch_argsBACKWARDopt_from_incursor_name'} = 'ignore';

And I needed to pull up these into FetchStmt as:
FETCH fetch_args FORWARD cursor_name
FETCH fetch_args FORWARD from_in cursor_name
FETCH fetch_args BACKWARD cursor_name
FETCH fetch_args BACKWARD from_in cursor_name
MOVE fetch_args FORWARD cursor_name
MOVE fetch_args FORWARD from_in cursor_name
MOVE fetch_args BACKWARD cursor_name
MOVE fetch_args BACKWARD from_in cursor_name

But I have the following problem. When this is in ecpg.addon:
===============================
...
ECPG: FetchStmtFETCHfetch_args addon
ECPG: FetchStmtMOVEfetch_args addon
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;
...
ECPG: FetchStmtMOVEfetch_args rule
| FETCH fetch_args ecpg_into
{
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;
$$ = cat2_str(make_str("fetch"), $2);
}
...
===============================

After running parse.pl, I get this in preproc.y for FetchStmt:

===============================
FetchStmt:
FETCH fetch_args
{
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;

$$ = cat_str(2,make_str("fetch"),$2);
}
| MOVE fetch_args
{
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;
{ // THIS IS AN EXTRA "{"
$$ = cat_str(2,make_str("move"),$2);
}
...
===============================

With this code, I can prevent the extra "{" emitted:

===============================
ECPG: FetchStmtMOVEfetch_args block
{
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;
$$ = cat2_str(make_str("move"), $2);
}
| FETCH fetch_args ecpg_into
{
add_additional_variables(current_cursor, false);
free(current_cursor);
current_cursor = NULL;
$$ = cat2_str(make_str("fetch"), $2);
}
...
===============================

And it bothers me, it looks illegal, but at least ugly.

With the first code, if I delete that extra "{" manually,
preproc.y compiles fine, and "make check" in ecpg fails
only one test, and the "failure" is only in the generated source
as now there's no "from" emitted in the ECPG-created
statements where FROM or IN doesn't appear in the
*.pgc code, but the stdout/stderr results are the same
as what's expected. Michael, can you give me some help here?
The attached patch uses the second variation, at least it
produces usable preproc.y that compiles into what I wanted.

In the attached patch I added a regression test, as well.
Actually, two, but they are the same, one copy under preproc,
one copy under compat_informix, so the difference in
ECPG runs an be observed.

You had a comment in a previous mail: "Some variable
handling commands look suspicious to me, a test case
might alleviate my concerns." I suspect you meant
introducing remove_variable_from_list(). The regression
tesst may help me prove the usefulness of this function,
especially in the FETCH :count FROM :curname; where
multiple $0 references occur, or the PREPARED statement
cases, where the order of the parameters passed to ECPGdo()
would come out reversed, or the dynamic cursor name would
get duplicated in some other statements.

I also tried to test this new code with a varchar cursor,
you're right, it didn't work with cursor name in a varchar
variable. I fixed this case now, reflected in the regression test.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan írta:
> But I have the following problem. When this is in ecpg.addon:
> ===============================
> ...
> ECPG: FetchStmtFETCHfetch_args addon
> ECPG: FetchStmtMOVEfetch_args addon
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
> ...
> ECPG: FetchStmtMOVEfetch_args rule
> | FETCH fetch_args ecpg_into
> {
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
> $$ = cat2_str(make_str("fetch"), $2);
> }
> ...
> ===============================
>
> After running parse.pl, I get this in preproc.y for FetchStmt:
>
> ===============================
> FetchStmt:
> FETCH fetch_args
> {
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
>
> $$ = cat_str(2,make_str("fetch"),$2);
> }
> | MOVE fetch_args
> {
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
> { // THIS IS AN EXTRA "{"
> $$ = cat_str(2,make_str("move"),$2);
> }
> ...
> ===============================
>
> With this code, I can prevent the extra "{" emitted:
>
> ===============================
> ECPG: FetchStmtMOVEfetch_args block
> {
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
> $$ = cat2_str(make_str("move"), $2);
> }
> | FETCH fetch_args ecpg_into
> {
> add_additional_variables(current_cursor, false);
> free(current_cursor);
> current_cursor = NULL;
> $$ = cat2_str(make_str("fetch"), $2);
> }
> ...
> ===============================
>

Any word on the above? If no, I can accept this construct being legal...

Anyway, I adapted the remaining two patches (SQLDA and
DESCRIBE) to the previously sent dynamic cursor code
and also added regression tests. Please, review.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Mon, Aug 03, 2009 at 06:12:43PM +0200, Boszormenyi Zoltan wrote:
> - sqlda support:
> - sqlda.c now indicates license
> - #defines inside #if 0 ... #endif are now omitted from sqltypes.h
> (both per comments from Jaime Casanova)

I still owe you some first thoughts about this part of the patch, although I
didn't run it yet

> *************** ecpg_execute(struct statement * stmt)
> *** 1351,1356 ****
> --- 1409,1435 ----
> }
> var = var->next;
> }
> + else if (var != NULL && var->type == ECPGt_sqlda)
> + {
> + pg_sqlda_t **_sqlda = (pg_sqlda_t **)var->pointer;
> + pg_sqlda_t *sqlda = *_sqlda;
> +
> + if (!sqlda)
> + {
> + sqlda = ecpg_build_sqlda_for_PGresult(stmt->lineno, results);
> + if (!sqlda)
> + status = false;
> + else
> + *_sqlda = sqlda;
> + }
> + else if (!ecpg_compare_sqlda_with_PGresult(sqlda, results))
> + status = false;
> +
> + if (status == true)
> + ecpg_set_sqlda_from_PGresult(stmt->lineno, _sqlda, results);

Please add some ecpg_log output here. The same is doen for a descriptor and for
variables, so it should be doen for sqlda too. Also please add some meaningful
comment as to what the code is supposed to do.

> + pg_sqlda_t *
> + ecpg_build_sqlda_for_PGresult(int line, PGresult *res)
> + {
> + pg_sqlda_t *sqlda;
> + pg_sqlvar_t*sqlvar;
> + char *fname;
> + long size;
> + int i;
> +
> + size = sizeof(pg_sqlda_t) + PQnfields(res) * sizeof(pg_sqlvar_t);
> + for (i = 0; i < PQnfields(res); i++)
> + size += strlen(PQfname(res, i)) + 1;
> + /* round allocated size up to the next multiple of 8 */
> + if (size % 8)
> + size += 8 - (size % 8);

Same here, the question is not *what* does the code accomplish, but *why*.

> +
> + sqlda = (pg_sqlda_t *)ecpg_alloc(size, line);
> + if (!sqlda)
> + {
> + ecpg_raise(line, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
> + return NULL;
> + }

ecpg_alloc is being used because it already checks for ENOMEM, no need to do it again.

> + static long
> + ecpg_sqlda_size_round_align(long size, int alignment, int round)
> + {
> + if (size % alignment)
> + size += alignment - (size % alignment);
> + size += round;
> + return size;
> + }

Another implementation of the same code we saw a few lines ago?

> + static long
> + ecpg_sqlda_size_align(long size, int alignment)
> + {
> + if (size % alignment)
> + size += alignment - (size % alignment);
> + return size;
> + }

And yet another one? Sure I see that the above function has an additional add
command, I just wonder why we need the alignment three times.

> + sqlda = realloc(sqlda, size);

We have ecpg_realloc().

> *************** get_char_item(int lineno, void *var, enu
> *** 225,230 ****
> --- 226,237 ----
> return (true);
> }
>
> + #define RETURN_IF_NO_DATA if (ntuples < 1) \
> + { \
> + ecpg_raise(lineno, ECPG_NOT_FOUND, ECPG_SQLSTATE_NO_DATA, NULL); \
> + return (false); \
> + }
> +

Could you please explain why you removed this test for some queries? Is there a
bug in there?

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Mon, Aug 03, 2009 at 06:12:43PM +0200, Boszormenyi Zoltan wrote:
>
>> - sqlda support:
>> - sqlda.c now indicates license
>> - #defines inside #if 0 ... #endif are now omitted from sqltypes.h
>> (both per comments from Jaime Casanova)
>>
>
> I still owe you some first thoughts about this part of the patch, although I
> didn't run it yet
>

Okay, answers below...

>> *************** ecpg_execute(struct statement * stmt)
>> *** 1351,1356 ****
>> --- 1409,1435 ----
>> }
>> var = var->next;
>> }
>> + else if (var != NULL && var->type == ECPGt_sqlda)
>> + {
>> + pg_sqlda_t **_sqlda = (pg_sqlda_t **)var->pointer;
>> + pg_sqlda_t *sqlda = *_sqlda;
>> +
>> + if (!sqlda)
>> + {
>> + sqlda = ecpg_build_sqlda_for_PGresult(stmt->lineno, results);
>> + if (!sqlda)
>> + status = false;
>> + else
>> + *_sqlda = sqlda;
>> + }
>> + else if (!ecpg_compare_sqlda_with_PGresult(sqlda, results))
>> + status = false;
>> +
>> + if (status == true)
>> + ecpg_set_sqlda_from_PGresult(stmt->lineno, _sqlda, results);
>>
>
> Please add some ecpg_log output here. The same is doen for a descriptor and for
> variables, so it should be doen for sqlda too. Also please add some meaningful
> comment as to what the code is supposed to do.
>

Will add the ecpg_log(). What the code does is:
- sets up a minimal SQLDA on the first call (called with NULL ptr),
so the field types and field names and some other properties are in place.
doesn't do further work if out of memory
- upon subsequent calls it checks whether a "compatible" sqlda was
passed in,
i.e. same number of fields, same types, etc. Sanity check. Doesn't do
further
work if the check fails.
- fills in the field values

>> + pg_sqlda_t *
>> + ecpg_build_sqlda_for_PGresult(int line, PGresult *res)
>> + {
>> + pg_sqlda_t *sqlda;
>> + pg_sqlvar_t*sqlvar;
>> + char *fname;
>> + long size;
>> + int i;
>> +
>> + size = sizeof(pg_sqlda_t) + PQnfields(res) * sizeof(pg_sqlvar_t);
>> + for (i = 0; i < PQnfields(res); i++)
>> + size += strlen(PQfname(res, i)) + 1;
>> + /* round allocated size up to the next multiple of 8 */
>> + if (size % 8)
>> + size += 8 - (size % 8);
>>
>
> Same here, the question is not *what* does the code accomplish, but *why*.
>

Arbitrary alignment, maybe not needed, Will check.

>> +
>> + sqlda = (pg_sqlda_t *)ecpg_alloc(size, line);
>> + if (!sqlda)
>> + {
>> + ecpg_raise(line, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
>> + return NULL;
>> + }
>>
>
> ecpg_alloc is being used because it already checks for ENOMEM, no need to do it again.
>

Okay, thanks.

>> + static long
>> + ecpg_sqlda_size_round_align(long size, int alignment, int round)
>> + {
>> + if (size % alignment)
>> + size += alignment - (size % alignment);
>>

Padding to the position of the "current" variable.

>> + size += round;
>>

Position to the next variable.

>> + return size;
>> + }
>>
>
> Another implementation of the same code we saw a few lines ago?
>

It's called with different alignments and padding at several places.
Used for computing the offset of the next variable.

>> + static long
>> + ecpg_sqlda_size_align(long size, int alignment)
>> + {
>> + if (size % alignment)
>> + size += alignment - (size % alignment);
>>

Padding only.

>> + return size;
>> + }
>>
>
> And yet another one? Sure I see that the above function has an additional add
> command, I just wonder why we need the alignment three times.
>

The fixed size alignment can be done with a call
to ecpg_sqlda_size_round_align(), sure.

>> + sqlda = realloc(sqlda, size);
>>
>
> We have ecpg_realloc().
>

Okay, thanks.

>> *************** get_char_item(int lineno, void *var, enu
>> *** 225,230 ****
>> --- 226,237 ----
>> return (true);
>> }
>>
>> + #define RETURN_IF_NO_DATA if (ntuples < 1) \
>> + { \
>> + ecpg_raise(lineno, ECPG_NOT_FOUND, ECPG_SQLSTATE_NO_DATA, NULL); \
>> + return (false); \
>> + }
>> +
>>
>
> Could you please explain why you removed this test for some queries? Is there a
> bug in there?
>

DESCRIBE can be used on queries not returning tuples.
This check at the beginning of the function prevented it.
I only added the check back to two or three places where
tuples were actually processed. Maybe I missed places.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Fri, Aug 14, 2009 at 10:12:24PM +0200, Boszormenyi Zoltan wrote:
> Will add the ecpg_log(). What the code does is:
> - sets up a minimal SQLDA on the first call (called with NULL ptr),
> so the field types and field names and some other properties are in place.
> doesn't do further work if out of memory

So this is an empty but compatible sqlda, right?

> - upon subsequent calls it checks whether a "compatible" sqlda was
> passed in,
> i.e. same number of fields, same types, etc. Sanity check. Doesn't do
> further
> work if the check fails.

What heppens if the sqlda is incompatible?

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Fri, Aug 14, 2009 at 10:12:24PM +0200, Boszormenyi Zoltan wrote:
>
>> Will add the ecpg_log(). What the code does is:
>> - sets up a minimal SQLDA on the first call (called with NULL ptr),
>> so the field types and field names and some other properties are in place.
>> doesn't do further work if out of memory
>>
>
> So this is an empty but compatible sqlda, right?
>

Yes. Is's the same sqlda as DESCRIBE would create.

>> - upon subsequent calls it checks whether a "compatible" sqlda was
>> passed in,
>> i.e. same number of fields, same types, etc. Sanity check. Doesn't do
>> further
>> work if the check fails.
>>
>
> What heppens if the sqlda is incompatible?
>

Returns false?

> Michael
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Sun, Aug 16, 2009 at 05:59:46PM +0200, Boszormenyi Zoltan wrote:
> > What heppens if the sqlda is incompatible?
>
> Returns false?

I wasn't talking about this one function but about the flow of the resulting
program. How can it happen that sqlda is incompatible and what happens then?

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Sun, Aug 16, 2009 at 05:59:46PM +0200, Boszormenyi Zoltan wrote:
>
>>> What heppens if the sqlda is incompatible?
>>>
>> Returns false?
>>
>
> I wasn't talking about this one function but about the flow of the resulting
> program. How can it happen that sqlda is incompatible and what happens then?
>

Hm. The following may occur. One may pass the same
sqlda ptr to two different cursors in DECLARE or FETCH
in the same loop. In this case it's wrong to not process the
"incompatible" call. Modified patch is attached:
- fixed flow, frees up the "incompatible" sqlda and creates a new one
- added ecpg_log() calls
- no more realloc(), an "empty" sqlda is allocated for full size.
realloc() can destroy internal pointers (like the ones pointing to
field names and ->sqlvar.)

The previous pg85-describe-5-ctxdiff.patch still applies cleanly.

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

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/