Re: Prepare and prepare ?

On Thu, 6 Feb 2003, David Wheeler wrote:

> Unfortunately I have no time to work on DBD::Pg stuff right now. :-(
> I'll have to rely on others to evaluate your approach. Post to the
> dbi-dev list, too -- those folks have a better idea on how to implement
> DBI drivers, I think.
>
> I hope I can find time to get back to this stuff one day, but I'm
> (happily) way too busy right now.
>
> Regards,
>
> David
>

Since David is too busy to work on DBD::Pg right now, I have ported the
changes that I made against 1.13 to CVS HEAD from gborg.postgresql.org.
The patch can be found here:
http://www.remotelinux.com/rlippan/dbd_pg.patch (I did not include it in
this email because I did not want to spam everyone with a 100K diff).

I'd like to start putting together some developer releases (set up a cvs
branch for this?), and see about getting together a stable release version
that addresses the all of the outstanding problems with DBD::Pg.

So, if people are interested, where do I go from here to get things set
up for doing dev releases &c.?

What the patch does:

1. $dbh->prepare() rewrites the SQL statement into an internal for
striping out comments and whitespace, and if PostgreSQL > 7.3 takes the
stripped statement
and passes that to postgress' PREPARE statement, then rewrites the
statement as 'EXECUTE "DBD::PG::cached_query n" ($1, $2, ... $n, $n+1)'
for DBD::Pg's execute.

2. Allows the use of :n and :foo bind params. So: (SELECT * FROM foo where
1 = :this and 2 = :that) will now work.

3. Complains on execute when unbound bind params are submitted (instead of
defaulting to NULL)

4. Switched over to use driver.xst.

5. pg_error() only removes \n's don't truncate message on first \n

6. fixed statement scan problem where the preparse of
"SELECT foo[3:33] from bar" was scanning :33 as a placeholder

7. moved the quoting of bind values out of execute() and into
bind -- as there is no need to requote the value every time execute
is called.

8. :veryverylongplaceholdername == Long walk. Sort pier -- fixed.

9. quote() is now in C and uses same code as bind_param.

10. quoting and dequoting now use libpq quoting functions where available
(I still need
to take the libpq functions swiped out of quote.c and move it into
libpqswip.c
with license info &c., and switch ifndefs to ifdefs)

11. bind_param() will convert from 1,0 to TRUE/FALSE when pg_type is
PGBOOLOID.

12. fixed many heap buffer overruns.

Known problems with patch:

1. Broke new pg_bool_tf attribute.

2. Broke rows() method (Just needs a define in makefile)

3. quote("stuff", {typeinfo }) is broken (only when hashref is passed in)

4. broke chopblanks.

5. has some // style comments around debug statements that need to be
converted into logging statements

6. PREPARE is not bullet-proof casts in RSH of equality and functions
LHS of equality can break serverside prepare, so decide exactly how to
do this and how to get prepares of INSERT statements to work? Move
prepqre to execute and build list column list in bind_param? Add
attributes to prepare to identify the columns and serverside prepare if
they exist?

7. Code is not as clean as it should be -- some kludgeness in there.

8. Documentation has not been updated.

9. Needs more tests.

On my immediate todo list:

1. Change quote and dequote functions to take Sv instead of string so that
things like arrays can be seralized by the quote function. This will
take care of broken chopblanks and pg_bool_tf (pass the quote/dequote
options struct to function quote/dequote functions).

2. Export the full list of supported OID types.

3. finish all type mappings in types.c

4. supoort for begin_work() method.

5. clean logging and trace levels.

6. dblogin to handle user names, table table names and other
names that containg ';'s

7. add attributes to control preparing of statements.

8. rewrite hacked version detection code (and get format for string so
it is done correctly with patch level &c.).

9. various code cleanups and polishing.

10. Copy over external test cases from DBD::Churlpg and write more tests.

11. Update documentation.

There are probably many other things that I cannot remember off the top of
my head since I have not done much (other than copy code over to CVS head)
with this code since late Nov/ early Dec.

-r

On Tuesday, February 11, 2003, at 10:51 PM, Rudy Lippan wrote:

> Since David is too busy to work on DBD::Pg right now, I have ported the
> changes that I made against 1.13 to CVS HEAD from gborg.postgresql.org.
> The patch can be found here:
> http://www.remotelinux.com/rlippan/dbd_pg.patch (I did not include it
> in
> this email because I did not want to spam everyone with a 100K diff).

Wow. Reading your description below, I'm really impressed. That's a lot
of work! Good thing I stepped aside! ;-)

> I'd like to start putting together some developer releases (set up a
> cvs
> branch for this?), and see about getting together a stable release
> version
> that addresses the all of the outstanding problems with DBD::Pg.
>
> So, if people are interested, where do I go from here to get things set
> up for doing dev releases &c.?

No need for a separate branch, I think. Just change the version number
to have an undersore, e.g., 1.30_01. That will automatically make it a
beta release number when it's put on CPAN.

> What the patch does:
>
> 1. $dbh->prepare() rewrites the SQL statement into an internal for
> striping out comments and whitespace, and if PostgreSQL > 7.3 takes
> the
> stripped statement
> and passes that to postgress' PREPARE statement, then rewrites the
> statement as 'EXECUTE "DBD::PG::cached_query n" ($1, $2, ... $n,
> $n+1)'
> for DBD::Pg's execute.

Cool.

> 2. Allows the use of :n and :foo bind params. So: (SELECT * FROM foo
> where
> 1 = :this and 2 = :that) will now work.

Oh, was that broken?

> 3. Complains on execute when unbound bind params are submitted
> (instead of
> defaulting to NULL)

Didn't know about that one.

> 4. Switched over to use driver.xst.

Yay!

> 5. pg_error() only removes \n's don't truncate message on first \n

Oh, good. I think it should be changed to do_error, too, with renaming
in dbdimp.h, no?

> 6. fixed statement scan problem where the preparse of
> "SELECT foo[3:33] from bar" was scanning :33 as a placeholder
>
> 7. moved the quoting of bind values out of execute() and into
> bind -- as there is no need to requote the value every time execute
> is called.

Cool.

> 8. :veryverylongplaceholdername == Long walk. Sort pier -- fixed.
>
> 9. quote() is now in C and uses same code as bind_param.

Nice. Faster, t hen.

> 10. quoting and dequoting now use libpq quoting functions where
> available
> (I still need
> to take the libpq functions swiped out of quote.c and move it into
> libpqswip.c
> with license info &c., and switch ifndefs to ifdefs)

Excellent.

> 11. bind_param() will convert from 1,0 to TRUE/FALSE when pg_type is
> PGBOOLOID.

Smart.

> 12. fixed many heap buffer overruns.

Do you include tests for all of these things, too? That'd be great.

> Known problems with patch:
>
> 1. Broke new pg_bool_tf attribute.

Ah. Can be fixed, I assume?

> 2. Broke rows() method (Just needs a define in makefile)
>
> 3. quote("stuff", {typeinfo }) is broken (only when hashref is passed
> in)

Probably is never used, anyway. Or shouldn't be. Is it fixable?

> 4. broke chopblanks.
>
> 5. has some // style comments around debug statements that need to be
> converted into logging statements
>
> 6. PREPARE is not bullet-proof casts in RSH of equality and functions
> LHS of equality can break serverside prepare, so decide exactly how
> to
> do this and how to get prepares of INSERT statements to work? Move
> prepqre to execute and build list column list in bind_param? Add
> attributes to prepare to identify the columns and serverside
> prepare if
> they exist?

Not sure I understand the problems here. Is it weaknesses in
PostgreSQL's PREPARE?

> 7. Code is not as clean as it should be -- some kludgeness in there.

Oh, well *that's* not new! ;-)

> 8. Documentation has not been updated.
>
> 9. Needs more tests.
>
>
>
> On my immediate todo list:
>
> 1. Change quote and dequote functions to take Sv instead of string so
> that
> things like arrays can be seralized by the quote function. This
> will
> take care of broken chopblanks and pg_bool_tf (pass the
> quote/dequote
> options struct to function quote/dequote functions).

Cool.

> 2. Export the full list of supported OID types.

For constants? I think I added a partial list in 1.20.

> 3. finish all type mappings in types.c
>
> 4. supoort for begin_work() method.
>
> 5. clean logging and trace levels.
>
> 6. dblogin to handle user names, table table names and other
> names that containg ';'s
>
> 7. add attributes to control preparing of statements.
>
> 8. rewrite hacked version detection code (and get format for string so
> it is done correctly with patch level &c.).

You'll want to borrow code from PostgreSQL's ODBC implementation for
this, I think. Tim sent some links a while ago.

> 9. various code cleanups and polishing.
>
> 10. Copy over external test cases from DBD::Churlpg and write more
> tests.
>
> 11. Update documentation.
>
> There are probably many other things that I cannot remember off the
> top of
> my head since I have not done much (other than copy code over to CVS
> head)
> with this code since late Nov/ early Dec.

Still, it's a lot of work. I had no idea you'd done this, Rudy. I
suspect that once you get this committed, others will contribute, as it
represents the first real advances in DBD::Pg in years. I'll help out
where I can, too.

David

--
David Wheeler AIM: dwTheory
david(at)kineticode(dot)com ICQ: 15726394
Yahoo!: dew7e
Jabber: Theory(at)jabber(dot)org
Kineticode. Setting knowledge in motion.[sm]

On Wed, 12 Feb 2003, David Wheeler wrote:

>
> > I'd like to start putting together some developer releases (set up a
> > cvs
> > branch for this?), and see about getting together a stable release
> > version
> > that addresses the all of the outstanding problems with DBD::Pg.
> >
> > So, if people are interested, where do I go from here to get things set
> > up for doing dev releases &c.?
>
> No need for a separate branch, I think. Just change the version number
> to have an undersore, e.g., 1.30_01. That will automatically make it a
> beta release number when it's put on CPAN.
>

I was thinking more for bug fixes agaist the old version before the
devel release are tested enough to go to production.

As for CPAN, should I go ahead set up a PAUSE account (I need to anyway)
for making the dev release, or do you want to handle making the releases?
(I warn you, though, that I am planing on releasing quite often until I
get a release that is stable enough for a production environment ;) ) If
the former what would need to be done to get CPAN to pick up my uploads as
part of DBD::Pg?

I signed up for the DBD::Pg project on gborg.org few days ago, so what do
I need to do to get CVS commit access?

> Cool.
>
> > 2. Allows the use of :n and :foo bind params. So: (SELECT * FROM foo
> > where
> > 1 = :this and 2 = :that) will now work.
>
> Oh, was that broken?
>

I think that the docs say that it does not work, but the code (from
DBD::oracle) was still there in preparse and bind, so it would work up
until execute() where it would break because execute was not looking for
:foo style placeholders.

This version saves the values for placeholer 1..n in an array and just
uses the key :foo as a lookup for the index into that array.

>
> > 5. pg_error() only removes \n's don't truncate message on first \n
>
> Oh, good. I think it should be changed to do_error, too, with renaming
> in dbdimp.h, no?
>

To get away from possible namespace collisions with libpq? the libpq
functions start with PQ..., BTA pg_eror should probably be declared
static as should several other functions.

> > 8. :veryverylongplaceholdername == Long walk. Sort pier -- fixed.
> >
> > 9. quote() is now in C and uses same code as bind_param.
>
> Nice. Faster, t hen.

Probably. But the big win is that it uses the same quoting code that
bind_param does; therefore, they both use the libpq quoting function.

> > 12. fixed many heap buffer overruns.
>
> Do you include tests for all of these things, too? That'd be great.

Yup. But they are not in the patch.

>
> > Known problems with patch:
> >
> > 1. Broke new pg_bool_tf attribute.
>
> Ah. Can be fixed, I assume?
>

Yeah. This just comes about because the way I implemented quoting. There
is a struct of pq_types that contains pointers to quote and dequote
functions. Right now the quote and dequote functions don't know anything
about perl/dbi... "Take string send to libpq return string returned from
libpq."

> > 3. quote("stuff", {typeinfo }) is broken (only when hashref is passed
> > in)
>
> Probably is never used, anyway. Or shouldn't be. Is it fixable?
>

These are the calls that are broken:
quote(foo, {type=>SQL_INTEGER})
quote(foo, {pg_type=>DBD::Pg::PG_INTEGER})

Which I do see being used. So this fix is at the top of my list.

> > 6. PREPARE is not bullet-proof casts in RSH of equality and functions
> > LHS of equality can break serverside prepare, so decide exactly how
> > to
> > do this and how to get prepares of INSERT statements to work? Move
> > prepqre to execute and build list column list in bind_param? Add
> > attributes to prepare to identify the columns and serverside
> > prepare if
> > they exist?
>
> Not sure I understand the problems here. Is it weaknesses in
> PostgreSQL's PREPARE?

Yes. PostgreSQL's PREPARE wants a list of column types when you call
prepare, so as proof of concept I did a (varchar, varchar, varchar...) for
the column types, but that ends up breaking some things (like UPDATE
INSERT, and when it checks an operation for castability).

Which does not make sense to me... postgres knows the fields types why
does it not set them correctly instead of making the application
programmer enumerate them... (or maybe give a type that causes pg to
not do checking. -- pg does the checks twice once on prepare and then it
does them again on execute, so again why not just sub in the correct types
if nothing was enumerated beforhand on prepare and let the checks on
execute handle the type checks?. Or am I missing something? Grr.

>
> > 2. Export the full list of supported OID types.
>
> For constants? I think I added a partial list in 1.20.
>

I know, those might be easier for the user, but for all interal stuff, I
am using the ones extraced from pg header files:

grep '^#define' /data/dnloads/postgresql-7.3/src/include/catalog/pg_type.h
|grep OID >pg_typeOID.h

And I think that they should be available to the perl programmer as well.

> > 8. rewrite hacked version detection code (and get format for string so
> > it is done correctly with patch level &c.).
>
> You'll want to borrow code from PostgreSQL's ODBC implementation for
> this, I think. Tim sent some links a while ago.
>

I'll look at doing that. Might save me some work :)

-r

On Wednesday, February 12, 2003, at 07:42 PM, Rudy Lippan wrote:

> I was thinking more for bug fixes agaist the old version before the
> devel release are tested enough to go to production.

Oh, well we can create a branch for the old version. Bruce, do you have
a CVS tag convention that you like to use?

> As for CPAN, should I go ahead set up a PAUSE account (I need to
> anyway)
> for making the dev release, or do you want to handle making the
> releases?
> (I warn you, though, that I am planing on releasing quite often until I
> get a release that is stable enough for a production environment ;) )
> If
> the former what would need to be done to get CPAN to pick up my
> uploads as
> part of DBD::Pg?

Either way. I can give you ownership if you like, but even if I don't
you can still upload releases. Let me know your preference.

> I signed up for the DBD::Pg project on gborg.org few days ago, so what
> do
> I need to do to get CVS commit access?

Tell me your GBorg login name and I'll give it to you.

> I think that the docs say that it does not work, but the code (from
> DBD::oracle) was still there in preparse and bind, so it would work up
> until execute() where it would break because execute was not looking
> for
> :foo style placeholders.
>
> This version saves the values for placeholer 1..n in an array and just
> uses the key :foo as a lookup for the index into that array.

Makes sense.

> To get away from possible namespace collisions with libpq? the libpq
> functions start with PQ..., BTA pg_eror should probably be declared
> static as should several other functions.

Yes. DBI::DBD says that dbdimp.h in part

defines macros that rename the generic names
like dbd_db_login to database specific names like
ora_db_login. This avoids name clashes and enables use of
different drivers when you work with a statically linked
perl.

> Yes. PostgreSQL's PREPARE wants a list of column types when you call
> prepare, so as proof of concept I did a (varchar, varchar, varchar...)
> for
> the column types, but that ends up breaking some things (like UPDATE
> INSERT, and when it checks an operation for castability).

Tom has made some mumblings about making this easier, but I don't know
if anything has been done. But I was under the assumption that if you
used "text" for the data types that they would be cast without too much
trouble.

> Which does not make sense to me... postgres knows the fields types why
> does it not set them correctly instead of making the application
> programmer enumerate them... (or maybe give a type that causes pg to
> not do checking. -- pg does the checks twice once on prepare and then
> it
> does them again on execute, so again why not just sub in the correct
> types
> if nothing was enumerated beforhand on prepare and let the checks on
> execute handle the type checks?. Or am I missing something? Grr.

This is beyond my limited knowledge, I'm afraid.

> I know, those might be easier for the user, but for all interal stuff,
> I
> am using the ones extraced from pg header files:
>
> grep '^#define'
> /data/dnloads/postgresql-7.3/src/include/catalog/pg_type.h
> |grep OID >pg_typeOID.h
>
> And I think that they should be available to the perl programmer as
> well.

Absolutely.

Regards,

David

--
David Wheeler AIM: dwTheory
david(at)kineticode(dot)com ICQ: 15726394
Yahoo!: dew7e
Jabber: Theory(at)jabber(dot)org
Kineticode. Setting knowledge in motion.[sm]

David Wheeler wrote:
> On Wednesday, February 12, 2003, at 07:42 PM, Rudy Lippan wrote:
>
> > I was thinking more for bug fixes agaist the old version before the
> > devel release are tested enough to go to production.
>
> Oh, well we can create a branch for the old version. Bruce, do you have
> a CVS tag convention that you like to use?

Let's get the small UTF patch into a DBD:pg release, then we can just
put it all into the main CVS and do a beta/final release with his
changes. I don't see why we need to create a branch.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Wednesday, February 12, 2003, at 08:49 PM, Bruce Momjian wrote:

> Let's get the small UTF patch into a DBD:pg release, then we can just
> put it all into the main CVS and do a beta/final release with his
> changes. I don't see why we need to create a branch.

Works for me...unless Rudy's planning to break the backwards
compatibility of the UTF-8 patch. ;-)

David

--
David Wheeler AIM: dwTheory
david(at)kineticode(dot)com ICQ: 15726394
Yahoo!: dew7e
Jabber: Theory(at)jabber(dot)org
Kineticode. Setting knowledge in motion.[sm]

On Wed, 12 Feb 2003, David Wheeler wrote:

> On Wednesday, February 12, 2003, at 07:42 PM, Rudy Lippan wrote:
>
> Oh, well we can create a branch for the old version. Bruce, do you have
> a CVS tag convention that you like to use?
>
> > As for CPAN, should I go ahead set up a PAUSE account (I need to
> > anyway)
> > for making the dev release, or do you want to handle making the
> > releases?
> > (I warn you, though, that I am planing on releasing quite often until I
> > get a release that is stable enough for a production environment ;) )
> > If
> > the former what would need to be done to get CPAN to pick up my
> > uploads as
> > part of DBD::Pg?
>
> Either way. I can give you ownership if you like, but even if I don't
> you can still upload releases. Let me know your preference.
>

I will go fill out the PAUSE account form tomorrow morning, & when I do
I will ask if ownership is required.

> > I signed up for the DBD::Pg project on gborg.org few days ago, so what
> > do
> > I need to do to get CVS commit access?
>
> Tell me your GBorg login name and I'll give it to you.
>

rlippan

> > Yes. PostgreSQL's PREPARE wants a list of column types when you call
> > prepare, so as proof of concept I did a (varchar, varchar, varchar...)
> > for
> > the column types, but that ends up breaking some things (like UPDATE
> > INSERT, and when it checks an operation for castability).
>
> Tom has made some mumblings about making this easier, but I don't know
> if anything has been done. But I was under the assumption that if you
> used "text" for the data types that they would be cast without too much
> trouble.
>

Maybe I will go over to PostgreSQL hackers and cause some trouble ;) It is
a *major* pain. I looked at PostgreSQL & it looks do-able, just
non-trivial.

-r

On Wed, 12 Feb 2003, David Wheeler wrote:

>
> On Wednesday, February 12, 2003, at 08:49 PM, Bruce Momjian wrote:
>
> > Let's get the small UTF patch into a DBD:pg release, then we can just
> > put it all into the main CVS and do a beta/final release with his
> > changes. I don't see why we need to create a branch.

Sounds good to me.

>
> Works for me...unless Rudy's planning to break the backwards
> compatibility of the UTF-8 patch. ;-)
>

I would rather it be done the right way the first time; that being said,
we can add the whatever_utf_8 (don't remember name from patch) attribute
and then: 1) Flag it in the docs as expermental and don't use. 2)
or when DBD::Pg decides that PostgreSQL wants to send UTF data, use the
whatever_utf_8 attribute to decide whether to flag the returned data as
such (ie. set the default to false for now, & if whatever_utf_8 is true
trun on UTF for all data coming out of the db. Then in the next version
set the default to true, and set UTF8 iff whatever_utf_8 is true and
DBD::Pg the client character set is UTF8). 3) do both. 4) do something
better.

With 2, the patch goes in as is and backwards compatibility does not
get broken in the next version. Just a thought.

Ideas, Suggestions?

-r

On Wednesday, February 12, 2003, at 10:32 PM, Rudy Lippan wrote:

>> Tell me your GBorg login name and I'll give it to you.
>
> rlippan

You're now officially a developer. :-)

> Maybe I will go over to PostgreSQL hackers and cause some trouble ;)
> It is
> a *major* pain. I looked at PostgreSQL & it looks do-able, just
> non-trivial.

Good idea, though they've been known to join discussions on the
interfaces list, too.

David

--
David Wheeler AIM: dwTheory
david(at)kineticode(dot)com ICQ: 15726394
Yahoo!: dew7e
Jabber: Theory(at)jabber(dot)org
Kineticode. Setting knowledge in motion.[sm]