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

fix: Parsing server version with more than 3 parts #741

Merged
merged 2 commits into from Feb 2, 2017

Conversation

Projects
None yet
4 participants
@ericmack
Contributor

ericmack commented Feb 1, 2017

#667 - Allow for versions with greater than 3 parts. For versions with more than 3 parts, still return 3 parts (4th part ignored for now as no functionality is dependent on the 4th part. Allows for future versions of the server to utilize more than 3 part version numbers without upgrading the jdbc driver. This fix addresses #667

@jorsol

This comment has been minimized.

Contributor

jorsol commented Feb 1, 2017

Checkstyle fails:

[ERROR] ...core/ServerVersion.java:142: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

[ERROR] ...core/ServerVersion.java:143: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

[ERROR] ...core/ServerVersion.java:144: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

[ERROR] ...core/ServerVersion.java:146: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

[ERROR] ...test/util/ServerVersionParseTest.java:38: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]

You can test checkstyle prior commit using: mvn checkstyle:check

@jorsol

This comment has been minimized.

Contributor

jorsol commented Feb 2, 2017

Thanks Eric, but I have made more tests, and it looks that the current parser is fragile, let's suppose that EDB release a 10.x version, with current policy, it only allows a 2 part version, so if EDB release something like 10.0.3 or 10.0.0.3, this will break again.

@ericmack

This comment has been minimized.

Contributor

ericmack commented Feb 2, 2017

@ericmack

This comment has been minimized.

Contributor

ericmack commented Feb 2, 2017

@jorsol

This comment has been minimized.

Contributor

jorsol commented Feb 2, 2017

On the other hand I guess that this patch it's fine until a robust server version parsing code is available, I can work on that and if nobody has objections this can me merged since a release of EDB 10.x will be available is a year or two.

@vlsi

This comment has been minimized.

Member

vlsi commented Feb 2, 2017

@ericmack , the thing is PostgreSQL itself went for two-part version numbers since 10.0.
That is PostgreSQL will not have 10.0.1. There won't be such thing as PostgreSQL 10.0.0.1 neither.
See postgres/postgres@ca9112a and postgres/postgres@69dc5ae

I hope EDB would not go for "more than two part" 10.x versions, so we won't have to deal with different 10.x versions coming from EDB and PG.

@davecramer

This comment has been minimized.

Member

davecramer commented Feb 2, 2017

@vlsi

This comment has been minimized.

Member

vlsi commented Feb 2, 2017

robust server version parsing code is available

Well, current pgjdbc parsing logic follows libpq/fe-exec.c postgres/postgres@69dc5ae#diff-2df5cad06efe4485ad362b0eb765cec0R986, so there's no much room we can have there.

I'm fine with merging this in, however I would strongly advice EDB to either follow PostgreSQL version scheme or send server_version_num at the startup packet.
If database sends server_version_num (that is a number like 90400 for "9.4"), then pgjdbc would just use that number as a backend version and refrain from parsing altogether.

I wish PostgreSQL sent server_version_num, unfortunately however Tom & Co have decided not doing that.

@vlsi

This comment has been minimized.

Member

vlsi commented Feb 2, 2017

Hmmm.... there is no mandate for this driver to support forks.

To be clear: I'm fine with accepting this particular PR.
I see no harm in it.

However, I just wave a warning sign, that versions like 10.1.2.3 could blow the integration again.

@davecramer

This comment has been minimized.

Member

davecramer commented Feb 2, 2017

@vlsi vlsi added this to the 42.0.0 milestone Feb 2, 2017

@vlsi vlsi merged commit 8437f6c into pgjdbc:master Feb 2, 2017

2 checks passed

codecov/project 64.42% (+0.01%) compared to 4942f7d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorsol

This comment has been minimized.

Contributor

jorsol commented Feb 2, 2017

By robust I mean, handle 3 part of <= 9, and the rest is discarded (like this patch), and if >=10 handle 2 part and the rest is discarded, is not that this change any current logic, it simply ignore the extra part. I was thinking in something more elaborated like maven artifacts versions parsing but it really don't make sense here.

It's really unfortunate that PostgreSQL don't send server_version_num, that would make things a lot easier.

Thanks,

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