Thread: Re: [GENERAL] Re: [PHP3] Re: PostgreSQL vs Mysql comparison
> > tested your patch and there was no change in result. I think it > > wouldn't be nice if this will point out a bug in the perl pg driver > > because I can't imagine that you would like to do such things in > > there ... > > > > the new crash-me tests results are sent to monty so I think he will > > put them online tomorrow (today for you I think). I also did a test > > run on oracle and on a microsoft sql 7 server on windows nt (oracle > > on linux). > > Enclosed is a patch that shows our perl interface can't handle '--' > comments, even though psql and the backend directly can handle them. > > To add complexity to this, the backend -d3 log from the perl test > session shows the same query that works perfectly in a direct backend > connection. > > Can anyone suggest a cause for this? OK, fix attached. Seems our "--" comments required a newline on the end, which was not being done in interfaces like Perl. Added a test in the perl code for the trailing comments, and patched scan.l. Seems this should only be applied to 6.6. Applied to 6.6. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 ? doc/src/sgml/install.htm ? src/log ? src/config.log ? src/config.cache ? src/config.status ? src/GNUmakefile ? src/Makefile.global ? src/Makefile.custom ? src/backend/fmgr.h ? src/backend/parse.h ? src/backend/postgres ? src/backend/global1.bki.source ? src/backend/local1_template1.bki.source ? src/backend/global1.description ? src/backend/local1_template1.description ? src/backend/bootstrap/bootparse.c ? src/backend/bootstrap/bootstrap_tokens.h ? src/backend/bootstrap/bootscanner.c ? src/backend/catalog/genbki.sh ? src/backend/catalog/global1.bki.source ? src/backend/catalog/global1.description ? src/backend/catalog/local1_template1.bki.source ? src/backend/catalog/local1_template1.description ? src/backend/port/Makefile ? src/backend/utils/Gen_fmgrtab.sh ? src/backend/utils/fmgr.h ? src/backend/utils/fmgrtab.c ? src/bin/cleardbdir/cleardbdir ? src/bin/createdb/createdb ? src/bin/createlang/createlang ? src/bin/createuser/createuser ? src/bin/destroydb/destroydb ? src/bin/destroylang/destroylang ? src/bin/destroyuser/destroyuser ? src/bin/initdb/initdb ? src/bin/initlocation/initlocation ? src/bin/ipcclean/ipcclean ? src/bin/pg_dump/Makefile ? src/bin/pg_dump/pg_dump ? src/bin/pg_id/pg_id ? src/bin/pg_passwd/pg_passwd ? src/bin/pg_version/Makefile ? src/bin/pg_version/pg_version ? src/bin/pgtclsh/mkMakefile.tcldefs.sh ? src/bin/pgtclsh/mkMakefile.tkdefs.sh ? src/bin/pgtclsh/Makefile.tkdefs ? src/bin/pgtclsh/Makefile.tcldefs ? src/bin/pgtclsh/pgtclsh ? src/bin/pgtclsh/pgtksh ? src/bin/psql/Makefile ? src/bin/psql/psql ? src/include/version.h ? src/include/config.h ? src/interfaces/ecpg/lib/Makefile ? src/interfaces/ecpg/lib/libecpg.so.3.0.1 ? src/interfaces/ecpg/lib/libecpg.so.3.0.3 ? src/interfaces/ecpg/preproc/ecpg ? src/interfaces/libpgtcl/Makefile ? src/interfaces/libpgtcl/libpgtcl.so.2.0 ? src/interfaces/libpq/Makefile ? src/interfaces/libpq/libpq.so.2.0 ? src/interfaces/libpq++/Makefile ? src/interfaces/libpq++/libpq++.so.3.0 ? src/interfaces/odbc/GNUmakefile ? src/interfaces/odbc/Makefile.global ? src/interfaces/perl5/blib ? src/interfaces/perl5/pm_to_blib ? src/interfaces/perl5/Pg.c ? src/interfaces/perl5/Pg.bs ? src/interfaces/perl5/Makefile ? src/lextest/lex.yy.c ? src/lextest/lextest ? src/pl/plpgsql/src/Makefile ? src/pl/plpgsql/src/mklang.sql ? src/pl/plpgsql/src/pl_gram.c ? src/pl/plpgsql/src/pl.tab.h ? src/pl/plpgsql/src/pl_scan.c ? src/pl/plpgsql/src/libplpgsql.so.1.0 ? src/pl/tcl/mkMakefile.tcldefs.sh ? src/pl/tcl/Makefile.tcldefs ? src/test/regress/regress.out ? src/test/regress/regression.diffs ? src/test/regress/expected/copy.out ? src/test/regress/expected/create_function_1.out ? src/test/regress/expected/create_function_2.out ? src/test/regress/expected/misc.out ? src/test/regress/expected/constraints.out ? src/test/regress/expected/install_plpgsql.out ? src/test/regress/results/boolean.out ? src/test/regress/results/char.out ? src/test/regress/results/name.out ? src/test/regress/results/varchar.out ? src/test/regress/results/text.out ? src/test/regress/results/strings.out ? src/test/regress/results/int2.out ? src/test/regress/results/int4.out ? src/test/regress/results/int8.out ? src/test/regress/results/oid.out ? src/test/regress/results/float4.out ? src/test/regress/results/float8.out ? src/test/regress/results/numerology.out ? src/test/regress/results/point.out ? src/test/regress/results/lseg.out ? src/test/regress/results/box.out ? src/test/regress/results/path.out ? src/test/regress/results/polygon.out ? src/test/regress/results/circle.out ? src/test/regress/results/geometry.out ? src/test/regress/results/timespan.out ? src/test/regress/results/datetime.out ? src/test/regress/results/reltime.out ? src/test/regress/results/abstime.out ? src/test/regress/results/tinterval.out ? src/test/regress/results/horology.out ? src/test/regress/results/inet.out ? src/test/regress/results/comments.out ? src/test/regress/results/oidjoins.out ? src/test/regress/results/type_sanity.out ? src/test/regress/results/opr_sanity.out ? src/test/regress/results/create_function_1.out ? src/test/regress/results/create_type.out ? src/test/regress/results/create_table.out ? src/test/regress/results/create_function_2.out ? src/test/regress/results/constraints.out ? src/test/regress/results/triggers.out ? src/test/regress/results/copy.out ? src/test/regress/results/onek.data ? src/test/regress/sql/copy.sql ? src/test/regress/sql/create_function_1.sql ? src/test/regress/sql/create_function_2.sql ? src/test/regress/sql/misc.sql ? src/test/regress/sql/constraints.sql ? src/test/regress/sql/install_plpgsql.sql ? src/tools/backend/flow.eps ? src/tools/backend/flow.ps ? src/tools/backend/flow.png ? src/tools/backend/flow.tif Index: src/backend/parser/scan.l =================================================================== RCS file: /usr/local/cvsroot/pgsql/src/backend/parser/scan.l,v retrieving revision 1.57 diff -c -r1.57 scan.l *** src/backend/parser/scan.l 1999/09/28 03:41:36 1.57 --- src/backend/parser/scan.l 1999/10/08 04:58:23 *************** *** 167,173 **** param \${integer} ! comment ("--"|"//").*\n space [ \t\n\f] other . --- 167,173 ---- param \${integer} ! comment ("--"|"//").* space [ \t\n\f] other . Index: src/interfaces/perl5/test.pl =================================================================== RCS file: /usr/local/cvsroot/pgsql/src/interfaces/perl5/test.pl,v retrieving revision 1.9 diff -c -r1.9 test.pl *** src/interfaces/perl5/test.pl 1998/09/27 19:12:26 1.9 --- src/interfaces/perl5/test.pl 1999/10/08 04:58:30 *************** *** 147,153 **** ######################### create and insert into table ! $result = $conn->exec("CREATE TABLE person (id int4, name char(16))"); die $conn->errorMessage unless PGRES_COMMAND_OK eq $result->resultStatus; my $cmd = $result->cmdStatus; ( "CREATE" eq $cmd ) --- 147,153 ---- ######################### create and insert into table ! $result = $conn->exec("CREATE TABLE person (id int4, name char(16)) -- test"); die $conn->errorMessage unless PGRES_COMMAND_OK eq $result->resultStatus; my $cmd = $result->cmdStatus; ( "CREATE" eq $cmd )
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> *** src/backend/parser/scan.l 1999/09/28 03:41:36 1.57
> --- src/backend/parser/scan.l 1999/10/08 04:58:23
> ***************
> *** 167,173 ****
>
> param \${integer}
>
> ! comment ("--"|"//").*\n
>
> space [ \t\n\f]
> other .
> --- 167,173 ----
>
> param \${integer}
>
> ! comment ("--"|"//").*
>
> space [ \t\n\f]
> other .
Ah, so the problem was that the perl interface didn't append a newline?
Good catch. I don't like this fix, however, since I fear it will
alter behavior for the case where there is an embedded newline in the
query buffer. For exampleCREATE TABLE mytab -- comment \n (f1 int)
can be sent to the backend as one string (though not via psql). With
the above change in scan.l I think the comment will be taken to include
everything from -- to the end of the buffer, which is wrong.
A better solution IMHO is to leave scan.l as it was and instead
always append a \n to the presented query string before we parse.
BTW, might be a good idea to add \r to that list of "space" characters
so we don't mess up on DOS-style newlines (\r\n).
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > *** src/backend/parser/scan.l 1999/09/28 03:41:36 1.57
> > --- src/backend/parser/scan.l 1999/10/08 04:58:23
> > ***************
> > *** 167,173 ****
> >
> > param \${integer}
> >
> > ! comment ("--"|"//").*\n
> >
> > space [ \t\n\f]
> > other .
> > --- 167,173 ----
> >
> > param \${integer}
> >
> > ! comment ("--"|"//").*
> >
> > space [ \t\n\f]
> > other .
>
> Ah, so the problem was that the perl interface didn't append a newline?
> Good catch. I don't like this fix, however, since I fear it will
> alter behavior for the case where there is an embedded newline in the
> query buffer. For example
> CREATE TABLE mytab -- comment \n (f1 int)
No problem. I just added test code to see if it works, and it does:
$result = $conn->exec("CREATE TABLE person (id int4, -- test\n name char(16)) -- test");
Tests embedded newline, and comment without newline.
I will commit this so it will always be tested by the perl test code.
> can be sent to the backend as one string (though not via psql). With
> the above change in scan.l I think the comment will be taken to include
> everything from -- to the end of the buffer, which is wrong.
No, seems lex only goes the end-of-line unless you specifically say \n.
>
> A better solution IMHO is to leave scan.l as it was and instead
> always append a \n to the presented query string before we parse.
Problem here is that perl is not the only interface that would have this
problem. In fact, I am not sure why libpq doesn't have this problem.
Maybe it does. Anyway, changing all the interfaces would be a pain, and
non-portable to older releases.
>
> BTW, might be a good idea to add \r to that list of "space" characters
> so we don't mess up on DOS-style newlines (\r\n).
Interesting idea. I tried that, but the problem is things like this:
xqliteral [\\](.|\n)
If I change it to:
xqliteral [\\](.|\n|\r)
then \r\n is not going to work, and if I change it to:
xqliteral [\\](.|\n|\r)+
Then \n\n is going to be accepted when it shouldn't. Seems I will have
to leave it alone for now.
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> Ah, so the problem was that the perl interface didn't append a newline?
>> Good catch. I don't like this fix, however, since I fear it will
>> alter behavior for the case where there is an embedded newline in the
>> query buffer.
> I will commit this so it will always be tested by the perl test code.
But how often do we run that?
> No, seems lex only goes the end-of-line unless you specifically say \n.
OK, I see in the flex manual that "." matches everything except newline,
so I guess it will work. At least with flex. But ".*" patterns with
no clearly defined terminator always make me itch --- it doesn't take
much change to get the wrong result.
>> A better solution IMHO is to leave scan.l as it was and instead
>> always append a \n to the presented query string before we parse.
> Problem here is that perl is not the only interface that would have this
> problem. In fact, I am not sure why libpq doesn't have this problem.
No, I wasn't suggesting patching the perl interface; I was suggesting
changing the backend, ie, adding the \n to the received query in
postgres.c just before we hand it off to the parser.
>> BTW, might be a good idea to add \r to that list of "space" characters
>> so we don't mess up on DOS-style newlines (\r\n).
> Interesting idea. I tried that, but the problem is things like this:
> xqliteral [\\](.|\n)
Hmm, didn't think about what to do with \r inside literals. I agree,
it's not worth trying to be smart about those, so I suppose ignoring
them outside literals would be inconsistent. Still, how many people
try to enter newlines within literals? Adding \r to the whitespace
set and nothing else might still be a useful compatibility gain.
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> >> Ah, so the problem was that the perl interface didn't append a newline?
> >> Good catch. I don't like this fix, however, since I fear it will
> >> alter behavior for the case where there is an embedded newline in the
> >> query buffer.
>
> > I will commit this so it will always be tested by the perl test code.
>
> But how often do we run that?
Well, at least it is there now, and I will do --with-perl here, so it
will be run.
> > No, seems lex only goes the end-of-line unless you specifically say \n.
>
> OK, I see in the flex manual that "." matches everything except newline,
> so I guess it will work. At least with flex. But ".*" patterns with
> no clearly defined terminator always make me itch --- it doesn't take
> much change to get the wrong result.
True, but it fixes the problem.
>
> >> A better solution IMHO is to leave scan.l as it was and instead
> >> always append a \n to the presented query string before we parse.
>
> > Problem here is that perl is not the only interface that would have this
> > problem. In fact, I am not sure why libpq doesn't have this problem.
>
> No, I wasn't suggesting patching the perl interface; I was suggesting
> changing the backend, ie, adding the \n to the received query in
> postgres.c just before we hand it off to the parser.
I try to avoid hacks like that if I can. Removing \n from the comment
termination is much clearer and more limited.
>
> >> BTW, might be a good idea to add \r to that list of "space" characters
> >> so we don't mess up on DOS-style newlines (\r\n).
>
> > Interesting idea. I tried that, but the problem is things like this:
> > xqliteral [\\](.|\n)
>
> Hmm, didn't think about what to do with \r inside literals. I agree,
> it's not worth trying to be smart about those, so I suppose ignoring
> them outside literals would be inconsistent. Still, how many people
> try to enter newlines within literals? Adding \r to the whitespace
> set and nothing else might still be a useful compatibility gain.
Added \r to the {space} pattern.
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026