Thread: Fw: PATCH (Re: Startup message issues)

Fw: PATCH (Re: Startup message issues)

From
"Chris Smith"
Date:
Sorry.  Again forgot to copy the list.  (I'll get the hang of this
eventually...).  Here's what I mistakenly sent only to Kris, but meant to send
to the list.

Chris Smith wrote:
> Kris Jurka wrote:
> > This would be a good thing to do.  I believe the V3 protocol was put
> > in with the goal of getting it working which meant a copy, paste,
> > and minimal edit.  So yes V3 implies 7.4 and it would be nice if it
> > took full advantage of what is offered.
>
> Okay, great!  Could someone review the attached patch?  It makes the
> following changes:
>
>     1. Remove backward-compatibility for pre-7.4 versions from
>        the v3 protocol connection code.
>     2. Move client_encoding and DateStyle setup to startup message.
>     3. Parse ParameterStatus for server_version.
>
> The result of #2 and #3 is that v3 protocol connections are
> established with only one round-trip to the server (in the trust auth
> case; two when authentication is required).  I've done some ad-hoc
> testing... is there a test suite that should be run against this?

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation

Attachment

Re: Fw: PATCH (Re: Startup message issues)

From
Kris Jurka
Date:

> > Okay, great!  Could someone review the attached patch?  It makes the
> > following changes:
> >
> >     1. Remove backward-compatibility for pre-7.4 versions from
> >        the v3 protocol connection code.
> >     2. Move client_encoding and DateStyle setup to startup message.
> >     3. Parse ParameterStatus for server_version.
> >
> > The result of #2 and #3 is that v3 protocol connections are
> > established with only one round-trip to the server (in the trust auth
> > case; two when authentication is required).  I've done some ad-hoc
> > testing... is there a test suite that should be run against this?
>


Patch applied.  In the future please submit patches based on cvs instead
of the 7.4 code.  Driver development is now being done here:

http://gborg.postgresql.org/project/pgjdbc/projdisplay.php

Kris Jurka


Re: Fw: PATCH (Re: Startup message issues)

From
"Chris Smith"
Date:
Kris Jurka wrote:
> Patch applied.  In the future please submit patches based on cvs
> instead of the 7.4 code.  Driver development is now being done here:
>
> http://gborg.postgresql.org/project/pgjdbc/projdisplay.php

Thanks.  I had actually submitted that patch for review, though.  I'm not
entirely confident that it's correct at this point.  All the unit tests pass,
but I'm still thinking about when I said the encoding field to UNICODE, and
learning a lot more about the source as time goes on.

I have just grabbed a version of the driver from CVS.  I have a bit of a
conflict here because I'm being paid to get stuff working under 7.4.1 for our
production application; but if there's not a huge amount of effort involved,
I'll try to submit patches for CVS instead.

(By the way, where should I send JDBC driver patches in the future that I
intend to be committed.  I've realized just now that I'm the only person
sending them here.)

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation


Re: Fw: PATCH (Re: Startup message issues)

From
Dave Cramer
Date:
Chris,

You can send patches here

Dave
On Sat, 2004-02-14 at 09:39, Chris Smith wrote:
> Kris Jurka wrote:
> > Patch applied.  In the future please submit patches based on cvs
> > instead of the 7.4 code.  Driver development is now being done here:
> >
> > http://gborg.postgresql.org/project/pgjdbc/projdisplay.php
>
> Thanks.  I had actually submitted that patch for review, though.  I'm not
> entirely confident that it's correct at this point.  All the unit tests pass,
> but I'm still thinking about when I said the encoding field to UNICODE, and
> learning a lot more about the source as time goes on.
>
> I have just grabbed a version of the driver from CVS.  I have a bit of a
> conflict here because I'm being paid to get stuff working under 7.4.1 for our
> production application; but if there's not a huge amount of effort involved,
> I'll try to submit patches for CVS instead.
>
> (By the way, where should I send JDBC driver patches in the future that I
> intend to be committed.  I've realized just now that I'm the only person
> sending them here.)
--
Dave Cramer
519 939 0336
ICQ # 14675561


Re: Fw: PATCH (Re: Startup message issues)

From
Kris Jurka
Date:

On Sat, 14 Feb 2004, Chris Smith wrote:

> Kris Jurka wrote:
> > Patch applied.  In the future please submit patches based on cvs
> > instead of the 7.4 code.  Driver development is now being done here:
> >
> > http://gborg.postgresql.org/project/pgjdbc/projdisplay.php
>
> Thanks.  I had actually submitted that patch for review, though.  I'm not
> entirely confident that it's correct at this point.  All the unit tests pass,
> but I'm still thinking about when I said the encoding field to UNICODE, and
> learning a lot more about the source as time goes on.

Well, I didn't just blindly apply it.  I read it, did some testing, and
actually modified it slightly.  You had a comment saying V2 when you meant
V3 protocol.  In StartupPacket I put the user and database variables into
the params Hashtable.  (I'm not convinced this is much better, but I was
playing with this idea and thats how things ended up when I was ready to
commit.)  My only concern about the code is that String.getBytes is called
in StartupPacket without specifying an encoding.  I think this is
potentially a problem depending on the user's encoding and if the values
for username and database name.  The code has been this way since it was
originally written, so that's no reason not to commit your patch.

> I have just grabbed a version of the driver from CVS.  I have a bit of a
> conflict here because I'm being paid to get stuff working under 7.4.1 for our
> production application; but if there's not a huge amount of effort involved,
> I'll try to submit patches for CVS instead.

Working for a 7.4.1 database or working for the 7.4.1 jar file?  As you
have noticed the driver has a bunch of backwards compatibility stuff, so
you know the cvs driver will happing run against older versions of the
database.

> (By the way, where should I send JDBC driver patches in the future that I
> intend to be committed.  I've realized just now that I'm the only person
> sending them here.)
>

People send them here, just rather infrequently.  Keep them coming.

Kris Jurka


Re: Fw: PATCH (Re: Startup message issues)

From
"Chris Smith"
Date:
Kris Jurka wrote:
> Working for a 7.4.1 database or working for the 7.4.1 jar file?  As
> you have noticed the driver has a bunch of backwards compatibility
> stuff, so you know the cvs driver will happing run against older
> versions of the database.

Okay, I'm convinced.  Since the changes I'm planning on making are fairly
major, I suppose it's senseless to apply normal logic about not using
development code for a production application.  So I'm now working with CVS
and will plan to deploy from there.

--
www.designacourse.com
The Easiest Way to Train Anyone... Anywhere.

Chris Smith - Lead Software Developer/Technical Trainer
MindIQ Corporation