Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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;
+ longsize;
+ 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: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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;
+longsize;
+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
Re: [HACKERS] Split-up ECPG patches
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/
pg85-dyncursor-unified-grammar-6-ctxdiff.patch.gz
Description: Unix tar archive
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Tom Lane írta: Boszormenyi Zoltan z...@cybertec.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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Michael Meskes mes...@postgresql.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Boszormenyi Zoltan írta:
Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.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/
diff -dcrpN pgsql.orig/src/backend/parser/gram.y pgsql/src/backend/parser/gram.y
*** pgsql.orig/src/backend/parser/gram.y 2009-08-03 10:38:28.0 +0200
--- pgsql/src/backend/parser/gram.y 2009-08-09 17:48:36.0 +0200
*** static TypeName *TableFuncTypeName(List
*** 253,259
%type str relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name file_name cluster_index_specification
%type list func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator validator_clause
--- 253,259
%type str relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name cursor_name file_name cluster_index_specification
%type list func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator validator_clause
*** static TypeName *TableFuncTypeName(List
*** 331,337
%type ival opt_column event cursor_options opt_hold opt_set_data
%type objtype reindex_type drop_type comment_type
! %type node fetch_direction select_limit_value select_offset_value
select_offset_value2 opt_select_fetch_first_value
%type ival row_or_rows first_or_next
--- 331,337
%type ival opt_column event cursor_options opt_hold opt_set_data
%type objtype reindex_type drop_type comment_type
! %type node fetch_args select_limit_value select_offset_value
select_offset_value2 opt_select_fetch_first_value
%type ival row_or_rows first_or_next
*** reloption_elem:
*** 1915,1921
*/
ClosePortalStmt:
! CLOSE name
{
ClosePortalStmt *n = makeNode(ClosePortalStmt);
n-portalname = $2;
--- 1915,1921
*/
ClosePortalStmt:
! CLOSE cursor_name
{
ClosePortalStmt *n = makeNode(ClosePortalStmt);
n-portalname = $2;
*** comment_text:
*** 4082,4223
*
*/
! FetchStmt: FETCH fetch_direction from_in name
{
FetchStmt *n = (FetchStmt *) $2;
- n-portalname = $4;
- n-ismove = FALSE;
-
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Michael Meskes mes...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/
diff -dcrpN pgsql.orig/src/backend/parser/gram.y pgsql/src/backend/parser/gram.y
*** pgsql.orig/src/backend/parser/gram.y 2009-08-03 10:38:28.0 +0200
--- pgsql/src/backend/parser/gram.y 2009-08-08 17:26:00.0 +0200
*** static TypeName *TableFuncTypeName(List
*** 253,259
%type str relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name file_name cluster_index_specification
%type list func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator validator_clause
--- 253,259
%type str relation_name copy_file_name
database_name access_method_clause access_method attr_name
! index_name name cursor_name file_name cluster_index_specification
%type list func_name handler_name qual_Op qual_all_Op subquery_Op
opt_class opt_validator validator_clause
*** reloption_elem:
*** 1915,1921
*/
ClosePortalStmt:
! CLOSE name
{
ClosePortalStmt *n = makeNode(ClosePortalStmt);
n-portalname = $2;
--- 1915,1921
*/
ClosePortalStmt:
! CLOSE cursor_name
{
ClosePortalStmt *n = makeNode(ClosePortalStmt);
n-portalname = $2;
*** comment_text:
*** 4082,4095
*
*/
! FetchStmt: FETCH fetch_direction from_in name
{
FetchStmt *n = (FetchStmt *) $2;
n-portalname = $4;
n-ismove = FALSE;
$$ = (Node *)n;
}
! | FETCH name
{
FetchStmt *n = makeNode(FetchStmt);
n-direction = FETCH_FORWARD;
--- 4082,4095
*
*/
! FetchStmt: FETCH fetch_direction from_in cursor_name
{
FetchStmt *n = (FetchStmt *) $2;
n-portalname = $4;
n-ismove = FALSE;
$$ = (Node *)n;
}
! | FETCH cursor_name
{
FetchStmt *n = makeNode(FetchStmt);
n-direction = FETCH_FORWARD;
*** FetchStmt: FETCH fetch_direction from_in
*** 4098,4111
n-ismove = FALSE;
$$ = (Node *)n;
}
! | MOVE fetch_direction from_in name
{
FetchStmt *n = (FetchStmt *) $2;
n-portalname = $4;
n-ismove = TRUE;
$$ = (Node *)n;
}
! | MOVE name
{
FetchStmt *n = makeNode(FetchStmt);
n-direction = FETCH_FORWARD;
--- 4098,4111
n-ismove = FALSE;
$$ = (Node *)n;
}
! | MOVE fetch_direction from_in
Re: [HACKERS] Split-up ECPG patches
Tom Lane írta: Michael Meskes mes...@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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Boszormenyi Zoltan z...@cybertec.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Tom Lane írta: Boszormenyi Zoltan z...@cybertec.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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Boszormenyi Zoltan z...@cybertec.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Tom Lane írta: Boszormenyi Zoltan z...@cybertec.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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Boszormenyi Zoltan z...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
Tom Lane írta: Boszormenyi Zoltan z...@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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Split-up ECPG patches
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 z...@cybertec.at * Hans-J??rgen Sch??nig h...@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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 z...@cybertec.at * Hans-J??rgen Sch??nig h...@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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
