Split-up ECPG patches

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@cybertec.at>
* Hans-J??rgen Sch??nig <hs@cybertec.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@jabber.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@cybertec.at>
* Hans-J??rgen Sch??nig <hs@cybertec.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@cybertec.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@cybertec.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@jabber.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@jabber.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@jabber.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@jabber.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@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes <meskes@postgresql.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@postgresql.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@jabber.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/