New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Remove ClassCastException catch #527

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@marschall
Contributor

marschall commented Mar 3, 2016

CopyManager has two catches for ClassCastException. This ins't very
nice and can easily be changed to do a check before the cast.

refactor: Remove ClassCastException catch
CopyManager has two catches for ClassCastException. This ins't very
nice and can easily be changed to do a check before the cast.

@marschall marschall force-pushed the marschall:remove-ClassCastException branch from 6e15f4d to 6e5b472 Mar 3, 2016

@marschall

This comment has been minimized.

Contributor

marschall commented Mar 3, 2016

The tests are broken on master too.

@vlsi vlsi closed this in 8bee06b Mar 4, 2016

try {
op = queryExecutor.startCopy(sql, connection.getAutoCommit());
CopyOperation op = queryExecutor.startCopy(sql, connection.getAutoCommit());
if (op == null || op instanceof CopyIn) {

This comment has been minimized.

@davecramer

davecramer Mar 4, 2016

Member

Do we really want to return null here ?

This comment has been minimized.

@marschall

marschall Mar 5, 2016

Contributor

That was the behaviour of the old code. I wanted to keep this PR as a refactoring in the actual sense, not change the behaviour of the code.

This comment has been minimized.

@davecramer

davecramer Mar 5, 2016

Member

I'm still of the opinion that the code path is faster using the exception
as it truly should be an exception.

Dave Cramer

On 5 March 2016 at 07:17, Philippe Marschall notifications@github.com
wrote:

In pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
#527 (comment):

@@ -48,26 +48,24 @@ public CopyManager(BaseConnection connection) throws SQLException {
}

public CopyIn copyIn(String sql) throws SQLException {

  • CopyOperation op = null;
  • try {
  •  op = queryExecutor.startCopy(sql, connection.getAutoCommit());
    
  • CopyOperation op = queryExecutor.startCopy(sql, connection.getAutoCommit());
  • if (op == null || op instanceof CopyIn) {

That was the behaviour of the old code. I wanted to keep this PR as a
refactoring in the actual sense, not change the behaviour of the code.


Reply to this email directly or view it on GitHub
https://github.com/pgjdbc/pgjdbc/pull/527/files#r55120290.

This comment has been minimized.

@vlsi

vlsi Mar 5, 2016

Member

Do you mean "faster to read" or "faster to execute"?

This comment has been minimized.

@davecramer

davecramer Mar 5, 2016

Member

Faster to execute

Dave Cramer

On 5 March 2016 at 09:03, Vladimir Sitnikov notifications@github.com
wrote:

In pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
#527 (comment):

@@ -48,26 +48,24 @@ public CopyManager(BaseConnection connection) throws SQLException {
}

public CopyIn copyIn(String sql) throws SQLException {

  • CopyOperation op = null;
  • try {
  •  op = queryExecutor.startCopy(sql, connection.getAutoCommit());
    
  • CopyOperation op = queryExecutor.startCopy(sql, connection.getAutoCommit());
  • if (op == null || op instanceof CopyIn) {

Do you mean "faster to read" or "faster to execute"?


Reply to this email directly or view it on GitHub
https://github.com/pgjdbc/pgjdbc/pull/527/files#r55121473.

This comment has been minimized.

@vlsi

vlsi Mar 5, 2016

Member

I'm afraid, "exceptional way" was slower to execute here because code did use the exception object -> thus JVM had to create fully workable exception object, populate stacktrace, etc, etc.

On the other hand, I do not think one would be able to get measurable improvements via this change.

I would consider this as a mere code cleanup. "class cast exception" does not buy here much, so I think removing cce from "caused by" does clean up the resulting exception a bit.

This comment has been minimized.

@davecramer

davecramer Mar 5, 2016

Member

Works for me then

Dave Cramer

On 5 March 2016 at 09:10, Vladimir Sitnikov notifications@github.com
wrote:

In pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java
#527 (comment):

@@ -48,26 +48,24 @@ public CopyManager(BaseConnection connection) throws SQLException {
}

public CopyIn copyIn(String sql) throws SQLException {

  • CopyOperation op = null;
  • try {
  •  op = queryExecutor.startCopy(sql, connection.getAutoCommit());
    
  • CopyOperation op = queryExecutor.startCopy(sql, connection.getAutoCommit());
  • if (op == null || op instanceof CopyIn) {

I'm afraid, "exceptional way" was slower to execute here because code did
use the exception object -> thus JVM had to create fully workable exception
object, populate stacktrace, etc, etc.

On the other hand, I do not think one would be able to get measurable
improvements via this change.

I would consider this as a mere code cleanup. "class cast exception" does
not buy here much, so I think removing cce from "caused by" does clean up
the resulting exception a bit.


Reply to this email directly or view it on GitHub
https://github.com/pgjdbc/pgjdbc/pull/527/files#r55121544.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 4, 2016

I would think it was left as catching the exception for performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment