Support for JDBC setQueryTimeout, et al.

David Fetter <david@fetter.org> wrote:

Please find enclosed a WIP patch from one of my co-workers
intended to support JDBC's setQueryTimeout, along with the patch
for JDBC that uses it.

I agree that it would be very nice to support this JDBC feature, but
I'm not clear on why this can't be done with just JDBC changes using
the java.util.Timer class and the existing Statement.cancel()
method. Can you explain why the backend needed to be touched?

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

David Fetter <david@fetter.org> wrote:

Please find enclosed a WIP patch from one of my co-workers
intended to support JDBC's setQueryTimeout, along with the patch
for JDBC that uses it.

I agree that it would be very nice to support this JDBC feature, but
I'm not clear on why this can't be done with just JDBC changes using
the java.util.Timer class and the existing Statement.cancel()
method. Can you explain why the backend needed to be touched?

... and, if you are seriously expecting to have that happen, why the
patch was submitted to pgsql-jdbc not pgsql-hackers? I hadn't even
read it because I assumed it was strictly a JDBC change.

regards, tom lane

On Mon, Oct 11, 2010 at 16:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

David Fetter <david@fetter.org> wrote:

Please find enclosed a WIP patch from one of my co-workers
intended to support JDBC's setQueryTimeout, along with the patch
for JDBC that uses it.

I agree that it would be very nice to support this JDBC feature, but
I'm not clear on why this can't be done with just JDBC changes using
the java.util.Timer class and the existing Statement.cancel()
method.  Can you explain why the backend needed to be touched?

... and, if you are seriously expecting to have that happen, why the
patch was submitted to pgsql-jdbc not pgsql-hackers?  I hadn't even
read it because I assumed it was strictly a JDBC change.

To be fair to David, it was sent to *both* pgsql-jdbc and
pgsql-hackers. Maybe it was held for moderation on one and arrived out
of order?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

On Mon, Oct 11, 2010 at 16:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:

... and, if you are seriously expecting to have that happen, why the
patch was submitted to pgsql-jdbc not pgsql-hackers?

To be fair to David, it was sent to *both* pgsql-jdbc and
pgsql-hackers. Maybe it was held for moderation on one and arrived out
of order?

... Oh. My apologies to David. My copy came through via pgsql-jdbc,
and I wasn't awake enough to notice it'd been posted to -hackers too.

regards, tom lane

On Mon, 11 Oct 2010 08:29:16 -0500, "Kevin Grittner"
<Kevin.Grittner@wicourts.gov> wrote:

David Fetter <david@fetter.org> wrote:

Please find enclosed a WIP patch from one of my co-workers
intended to support JDBC's setQueryTimeout, along with the patch
for JDBC that uses it.

I agree that it would be very nice to support this JDBC feature, but
I'm not clear on why this can't be done with just JDBC changes using
the java.util.Timer class and the existing Statement.cancel()
method. Can you explain why the backend needed to be touched?

-Kevin

I sent such patch fully in Java
(http://archives.postgresql.org/pgsql-jdbc/2009-11/msg00010.php),
implementing cancellation with Timer and "cancel query" facility of PSQL
server, unfortunately none has revised it, even that setQuertyTimeout todo
is for long time on dashboard, and it's important for enterprise class
software.

----------
Radosław Smogura
http://www.softperience.eu

On Tue, Oct 12, 2010 at 04:04:56AM -0500, Radosław Smogura wrote:

On Mon, 11 Oct 2010 08:29:16 -0500, "Kevin Grittner"
<Kevin.Grittner@wicourts.gov> wrote:

David Fetter <david@fetter.org> wrote:

Please find enclosed a WIP patch from one of my co-workers
intended to support JDBC's setQueryTimeout, along with the patch
for JDBC that uses it.

I agree that it would be very nice to support this JDBC feature, but
I'm not clear on why this can't be done with just JDBC changes using
the java.util.Timer class and the existing Statement.cancel()
method. Can you explain why the backend needed to be touched?

-Kevin

I sent such patch fully in Java
(http://archives.postgresql.org/pgsql-jdbc/2009-11/msg00010.php),
implementing cancellation with Timer and "cancel query" facility of
PSQL server,

Would you like to update it?

Is there something incomplete about the ones I sent, and if so, what?

unfortunately none has revised it, even that setQuertyTimeout todo
is for long time on dashboard, and it's important for enterprise
class software.

That sounds a tad buzzword-y. What exactly is it that *you* see as
important to delivering this feature? I'm thinking JDBC1 compliance,
but you might have something very different in mind.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> wrote:

Is there something incomplete about the ones I sent, and if so,
what?

Well, I'm still curious why it was necessary to modify the server
side to implement an interface feature for which everything needed
seems to be present on the client side. Is this intended to be
useful for other interfaces? If so, we should probably have an
implementation in some other interface to confirm that the
server-side support fits. If not, why touch the server side code at
all?

These are not rhetorical questions. There may be some reason, but
if so, I'd like to see it stated, rather than trying to infer it
through reverse engineering.

-Kevin

On Tue, Oct 12, 2010 at 10:37:00AM -0500, Kevin Grittner wrote:

David Fetter <david@fetter.org> wrote:

Is there something incomplete about the ones I sent, and if so,
what?

Well, I'm still curious why it was necessary to modify the server
side to implement an interface feature for which everything needed
seems to be present on the client side.

Not everything is.

Let's imagine you have a connection pooler with two clients, A and B.
A calls setQueryTimeout, then starts a query, which terminates in
time, but dies before handling it. B connects to the pool, gets A's
connection, and finds a statement_timeout that's not the default, even
though only A's single query was supposed to have that
statement_timeout. This is not a situation that can be resolved
without being able to set a timer *on the server side*.

Is this intended to be useful for other interfaces?

Anybody doing similar functionality, namely a per-statement timeout,
would need this infrastructure, and for the same reason.

If so, we should probably have an implementation in some other
interface to confirm that the server-side support fits. If not, why
touch the server side code at all?

See above.

While I'd *like* to put in a whole infrastructure for setting GUCs on
a per-statement basis, I don't believe that we need to get out that
giant sledgehammer for this case, even though it's worth solving.

Incidentally, this dovetails neatly with the isolation concerns that
motivated the "true serializability" patch you're working on :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> wrote:

Let's imagine you have a connection pooler with two clients,
A and B.

I'm with you so far.

A calls setQueryTimeout, then starts a query, which terminates in
time, but dies before handling it.

Here you lost me. I don't know what that means.

B connects to the pool, gets A's connection, and finds a
statement_timeout that's not the default

Why? I would consider the JDBC QueryTimeout property to be
orthogonal to the server's statement_timeout GUC. Perhaps that's
why we're seeing things so differently.

even though only A's single query was supposed to have that
statement_timeout. This is not a situation that can be resolved
without being able to set a timer *on the server side*.

That will be true if we conflate these two things. I don't think we
should.

http://download.oracle.com/javase/6/docs/api/java/sql/Statement.html#setQueryTimeout%28int%29

This time limit should apply to the overall time allowed in the
driver for the execute, executeQuery and executeUpdate of the Java
Statement object. It should not be trying to pick apart individual
SQL statements within the execute request, and it should not affect
any other statement on the connection.

I think both patches are making this harder than it needs to be.

-Kevin

On Tue, Oct 12, 2010 at 17:55, David Fetter <david@fetter.org> wrote:

On Tue, Oct 12, 2010 at 10:37:00AM -0500, Kevin Grittner wrote:

David Fetter <david@fetter.org> wrote:

Is there something incomplete about the ones I sent, and if so,
what?

Well, I'm still curious why it was necessary to modify the server
side to implement an interface feature for which everything needed
seems to be present on the client side.

Not everything is.

Let's imagine you have a connection pooler with two clients, A and B.
A calls setQueryTimeout, then starts a query, which terminates in
time, but dies before handling it.  B connects to the pool, gets A's
connection, and finds a statement_timeout that's not the default, even
though only A's single query was supposed to have that
statement_timeout.  This is not a situation that can be resolved
without being able to set a timer *on the server side*.

Sure it can. The connection pooler just needs to issue a RESET ALL
statement when it hands over a connection from one client to another.
Isn't that what for example pgbouncer does - at least when configured
per instructions?

Also, doesn't this affect *all* settings, not just timeout, if it
doesn't? Imagine client A executing a SET datestyle for example.

AFAICS, any connection pooler that *doesn't* issue a reset between
handing this around is broken, isn't it?

If so, we should probably have an implementation in some other
interface to confirm that the server-side support fits.  If not, why
touch the server side code at all?

See above.

While I'd *like* to put in a whole infrastructure for setting GUCs on
a per-statement basis, I don't believe that we need to get out that
giant sledgehammer for this case, even though it's worth solving.

We don't usually put in fixes for just one out of 105 cases, do we?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

David Fetter <david@fetter.org> writes:

Let's imagine you have a connection pooler with two clients, A and B.
A calls setQueryTimeout, then starts a query, which terminates in
time, but dies before handling it. B connects to the pool, gets A's
connection, and finds a statement_timeout that's not the default, even
though only A's single query was supposed to have that
statement_timeout. This is not a situation that can be resolved
without being able to set a timer *on the server side*.

Actually, that seems like a fine argument why this should *not* be
implemented on the server side... although I would expect a connection
pooler to roll back GUC changes when switching users, so the argument
seems to presume several rather broken implementation decisions in
order to make the scenario possible.

While I'd *like* to put in a whole infrastructure for setting GUCs on
a per-statement basis, I don't believe that we need to get out that
giant sledgehammer for this case, even though it's worth solving.

You'd need to first convince people that SET LOCAL doesn't solve the
problem well enough already.

regards, tom lane

This, what I see in your patch, is sending additional statement to server.
This adds some unnecessery (especially TCP/IP) latency. gura

I sent such patch fully in Java
(http://archives.postgresql.org/pgsql-jdbc/2009-11/msg00010.php),
implementing cancellation with Timer and "cancel query" facility of
PSQL server,

Would you like to update it?

I updated patch to latets CVS version, I didn't have time to remove some
trashes from it.

If something will be wrong with patch, give a feedback.

Kind regards,
Radosław Smogura
http://softperience.pl

Rados*aw Smogura<rsmogura@softperience.eu> wrote:

I updated patch to latets CVS version, I didn't have time to
remove some trashes from it.

If something will be wrong with patch, give a feedback.

I skimmed it and agree that it is the right general approach --
using java.util.Timer to trigger the cancel method. It doesn't
confuse the function of the setQueryTimeout method of the JDBC
driver with the statement_timeout GUC of PostgreSQL, which strike me
as no more or less similar to each other than the brakes on my car
are to a highway guardrail -- both are designed to stop something,
but under different circumstances.

I certainly can't fault you for lack of testing, since about
two-thirds of the patch is testing classes. I didn't see any need
to include the last two classes (ByteConverter and
InfiniteTimerTask); can you explain why those are in there?

That said, some of the details of the implementation gave me pause
-- there seem to be rather more moving parts and more places to
check things than the overall complexity of the problem would seem
to warrant; however, it's entirely possible that on closer review
I'll find that they were necessary for reasons which escape me on a
quick scan of the code.

If you could add this to the open CommitFest, I'll be glad to review
it (if nobody beats me to it):

https://commitfest.postgresql.org/action/commitfest_view/open

-Kevin

On Wed, Oct 13, 2010 at 2:27 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Rados*aw Smogura<rsmogura@softperience.eu> wrote:

I updated patch to latets CVS version, I didn't have time to
remove some trashes from it.

If something will be wrong with patch, give a feedback.

I skimmed it and agree that it is the right general approach --
using java.util.Timer to trigger the cancel method.  It doesn't
confuse the function of the setQueryTimeout method of the JDBC
driver with the statement_timeout GUC of PostgreSQL, which strike me
as no more or less similar to each other than the brakes on my car
are to a highway guardrail -- both are designed to stop something,
but under different circumstances.

I certainly can't fault you for lack of testing, since about
two-thirds of the patch is testing classes.  I didn't see any need
to include the last two classes (ByteConverter and
InfiniteTimerTask); can you explain why those are in there?

That said, some of the details of the implementation gave me pause
-- there seem to be rather more moving parts and more places to
check things than the overall complexity of the problem would seem
to warrant; however, it's entirely possible that on closer review
I'll find that they were necessary for reasons which escape me on a
quick scan of the code.

If you could add this to the open CommitFest, I'll be glad to review
it (if nobody beats me to it):

https://commitfest.postgresql.org/action/commitfest_view/open

Is this a JDBC patch or a PG patch? Are we tracking JDBC patches
using the CF app?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 13 Oct 2010 21:01:06 -0400, Robert Haas <robertmhaas@gmail.com>
wrote:

Is this a JDBC patch or a PG patch? Are we tracking JDBC patches
using the CF app?

It is JDBC patch. I will clean it and submit on this site. I didn't know
about such application and such process.

----------
Radosław Smogura
http://www.softperience.eu

On Thu, Oct 14, 2010 at 2:59 AM, Radosław Smogura
<rsmogura@softperience.eu> wrote:

On Wed, 13 Oct 2010 21:01:06 -0400, Robert Haas <robertmhaas@gmail.com>
wrote:

Is this a JDBC patch or a PG patch?  Are we tracking JDBC patches
using the CF app?

It is JDBC patch. I will clean it and submit on this site. I didn't know
about such application and such process.

I'm not aware that the JDBC folks participate in the CommitFest
process, so it's probably best to work with the folks on pgsql-jdbc to
figure out how they'd like to see this submitted.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

Is this a JDBC patch or a PG patch? Are we tracking JDBC patches
using the CF app?

If this were the only patch for setQueryTimeout in front of us I
probably wouldn't have suggested that, but this thread started with a
patch proposal to implement the same JDBC feature through adding new
backend functions. Unless that patch is withdrawn or rejected, it
seems odd for two different groups to be simultaneously considering
patches to implement exactly the same functionality....

-Kevin

On Thu, Oct 14, 2010 at 8:55 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Robert Haas  wrote:

Is this a JDBC patch or a PG patch? Are we tracking JDBC patches
using the CF app?

If this were the only patch for setQueryTimeout in front of us I
probably wouldn't have suggested that, but this thread started with a
patch proposal to implement the same JDBC feature through adding new
backend functions.  Unless that patch is withdrawn or rejected, it
seems odd for two different groups to be simultaneously considering
patches to implement exactly the same functionality....

True. I thought we had decided on the client-side approach, but maybe
I'm confused. I don't have a position one way or the other, just
trying to understand the state of the conversation.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Oct 14, 2010 at 08:22:21AM -0400, Robert Haas wrote:

On Thu, Oct 14, 2010 at 2:59 AM, Radosław Smogura
<rsmogura@softperience.eu> wrote:

On Wed, 13 Oct 2010 21:01:06 -0400, Robert Haas <robertmhaas@gmail.com>
wrote:

Is this a JDBC patch or a PG patch?  Are we tracking JDBC patches
using the CF app?

It is JDBC patch. I will clean it and submit on this site. I didn't know
about such application and such process.

I'm not aware that the JDBC folks participate in the CommitFest
process, so it's probably best to work with the folks on pgsql-jdbc to
figure out how they'd like to see this submitted.

Perhaps they'd like to participate :)

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate