Skip to content
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

perf: Cache server version number #464

Closed
wants to merge 1 commit into from
Closed

perf: Cache server version number #464

wants to merge 1 commit into from

Conversation

@zapov
Copy link
Contributor

@zapov zapov commented Dec 25, 2015

Save version number after parsing it from string.
No need to recalculate it again.

This improves Statement.setObject(int, UUID) which ckecks for server version.

Save version number after parsing it from string.
No need to recalculate it again.

This improves Statement.setObject(int, UUID) which ckecks for server version.
@@ -57,7 +57,7 @@ public String getServerVersion() {
public int getServerVersionNum() {
if (serverVersionNum != 0)
return serverVersionNum;
return Utils.parseServerVersionStr(serverVersion);
return serverVersionNum = Utils.parseServerVersionStr(serverVersion);

This comment has been minimized.

@valgog

valgog Dec 26, 2015
Contributor

Is this thread safe?

This comment has been minimized.

@zapov

zapov Dec 26, 2015
Author Contributor

I don't see why not. It's bound to socket so it can't really change.

This comment has been minimized.

@vlsi

vlsi Dec 26, 2015
Member

int is always atomic in java, thus the change is thread safe

This comment has been minimized.

@valgog

valgog Dec 26, 2015
Contributor

Since which version int is thread safe in java?

This comment has been minimized.

@zapov

zapov Dec 26, 2015
Author Contributor

It's not thread safe in a sense that once updated every other thread will immediately see the new version. But that's not really important, since the other thread will just recalculate it again.
It's atomic (unlike long) so that is not an issue.
Or did you have something else in mind?

This comment has been minimized.

@valgog

valgog Dec 27, 2015
Contributor

When looking into the code of this class, it looks like quite inconsistent about synchronization.

Half of the methods are not synchronized though they access non-volatile inner variables.

It definitely does not relate to that pull request.

This comment has been minimized.

@davecramer

davecramer Dec 27, 2015
Member

Yes we don't seem to have much consistency with respect to thread safety.

I think in general though connections are not meant to be shared between
threads.

Postgresql itself is not particularly friendly with respect to that

Dave Cramer

On 27 December 2015 at 16:29, Valentine Gogichashvili <
notifications@github.com> wrote:

In pgjdbc/src/main/java/org/postgresql/core/v3/ProtocolConnectionImpl.java
#464 (comment):

@@ -57,7 +57,7 @@ public String getServerVersion() {
public int getServerVersionNum() {
if (serverVersionNum != 0)
return serverVersionNum;

  •    return Utils.parseServerVersionStr(serverVersion);
    
  •    return serverVersionNum = Utils.parseServerVersionStr(serverVersion);
    

When looking into the code of this class, it looks like quite inconsistent
about synchronization.

Half of the methods are not synchronized though they access non-volatile
inner variables.

It definitely does not relate to that pull request.


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

This comment has been minimized.

@valgog

valgog Dec 27, 2015
Contributor

I wonder if the synchronization is needed at all here. The point that @zapov raised about no real reason to synchronize for the getServerVersionNum should apply to most of the methods in this class.

This comment has been minimized.

@davecramer

davecramer Dec 27, 2015
Member

There are a couple of places where internal threads touch it, but we
control those so we should be able to eliminate much of the synchronization

Dave Cramer

On 27 December 2015 at 16:39, Valentine Gogichashvili <
notifications@github.com> wrote:

In pgjdbc/src/main/java/org/postgresql/core/v3/ProtocolConnectionImpl.java
#464 (comment):

@@ -57,7 +57,7 @@ public String getServerVersion() {
public int getServerVersionNum() {
if (serverVersionNum != 0)
return serverVersionNum;

  •    return Utils.parseServerVersionStr(serverVersion);
    
  •    return serverVersionNum = Utils.parseServerVersionStr(serverVersion);
    

I wonder if the synchronization is needed at all here. The point that
@zapov https://github.com/zapov raised about no real reason to
synchronize for the getServerVersionNum should apply to most of the methods
in this class.


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

This comment has been minimized.

@vlsi

vlsi Dec 28, 2015
Member

@valgog , basically, all the "setter" methods (setStandardConformingStrings, addWarning, ..) are used from org.postgresql.core.v2.QueryExecutorImpl#interpretCommandStatus and similar only -> thus no synchronization is required. So removing synchronized seems to be a valid optimization.

@vlsi
Copy link
Member

@vlsi vlsi commented Dec 26, 2015

Even though it seems to be a good change I would like to elaborate a bit more.

Backend developers recommend using server_version_num parameter.
The backend does not send it due to postgres/postgres@e35ea51.

I would like to raise that issue with backend developers. I do not see why they "avoid sending server_versino_num" thus they effectively forbid us using that variable.

@vlsi vlsi closed this in d80d9a8 Dec 26, 2015
vlsi added a commit that referenced this pull request Dec 26, 2015
Save version number after parsing it from string.
No need to recalculate it again.

Backend does not always send server_version_num, thus the driver falls back to string parsing.
See discussion on hackers list: http://www.postgresql.org/message-id/CAMsr+YFt1NcjseExt_Ov+frk0Jzb0-DKqYKt8ALzVEXHBM0jKg@mail.gmail.com

This improves Statement.setObject(int, UUID) which ckecks for server version.

closes #464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants