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

Url is not get correct while the url is a fail over url in BaseDataSource #1039

Open
joechen2010 opened this issue Dec 20, 2017 · 27 comments
Open

Comments

@joechen2010
Copy link

@joechen2010 joechen2010 commented Dec 20, 2017

while the jdbc url is like 'jdbc:postgresql://host1:1523,host2:1523/db?targetServerType=master' ,
but while the BaseDataSource allways get connection from 'jdbc:postgresql://host2:5432,host2:5432/db?targetServerType=master' , It allways change the ports to default port because parse to integer exception while multiple ports figure out.

@vlsi

This comment has been minimized.

Copy link
Member

@vlsi vlsi commented Dec 20, 2017

Would you please submit a PR with at test case and/or the fix?

@joechen2010

This comment has been minimized.

Copy link
Author

@joechen2010 joechen2010 commented Dec 21, 2017

Hi, here comes the test :
@test
public void testFailOverUrl() {
SimpleDataSource bds = new SimpleDataSource();
bds.setUrl("jdbc:postgresql://host1:1523,host2:1523/db?targetServerType=master");
bds.setUser("user");
bds.setPassword("password");
Assert.assertTrue("Url should contain 'host1:1523,host2:1523'", bds.getUrl().contains("host1:1523,host2:1523"));
}

getUrl is ''jdbc:postgresql://host1,host2/db?prepareThreshold=5&preparedStatementCacheQueries=256&preparedStatementCacheSizeMiB=5&databaseMetadataCacheFields=65536&databaseMetadataCacheFieldsMiB=5&defaultRowFetchSize=0&binaryTransfer=true&readOnly=false&binaryTransferEnable=&binaryTransferDisable=&unknownLength=2147483647&logUnclosedConnections=false&disableColumnSanitiser=false&tcpKeepAlive=false&loginTimeout=0&connectTimeout=10&socketTimeout=0&cancelSignalTimeout=10&receiveBufferSize=-1&sendBufferSize=-1&ApplicationName=PostgreSQL JDBC Driver&useSpnego=false&gsslib=auto&sspiServiceClass=POSTGRES&allowEncodingChanges=false&targetServerType=master&loadBalanceHosts=false&hostRecheckSeconds=10&preferQueryMode=extended&autosave=never&reWriteBatchedInserts=false"

@joechen2010

This comment has been minimized.

Copy link
Author

@joechen2010 joechen2010 commented Dec 21, 2017

The value of PG_PORT is '1523,1523', so NumberFormatException will cause portNumber =0.
case PG_PORT:
try {
portNumber = Integer.parseInt(value);
} catch (NumberFormatException e) {
portNumber = 0;
}
break;

While getUrl :

StringBuilder url = new StringBuilder(100);
url.append("jdbc:postgresql://");
url.append(serverName);
if (portNumber != 0) {
url.append(":").append(portNumber);
}
url.append("/").append(databaseName);

and getConnection:
Connection con = DriverManager.getConnection(getUrl(), user, password);

@joechen2010

This comment has been minimized.

Copy link
Author

@joechen2010 joechen2010 commented Dec 22, 2017

Is any one can answer whether this is a bug or not, if yes, is there any solution to work around this issue

@Hades32

This comment has been minimized.

Copy link

@Hades32 Hades32 commented Jun 25, 2018

Hey, this still seems to be the case, just had this in Wildfly...
My workaround is to append at the URL "&PGPORT=6666" and then it works...
Strange how this is not a major blocker...

@Hades32

This comment has been minimized.

Copy link

@Hades32 Hades32 commented Jun 25, 2018

BTW the same driver (42.2.2) works fine in e.g. Squirrel, but if used as Datasource in Wildfly this problem arises.

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Jun 25, 2018

SimpleDataSource is deprecated and as such we haven't paid much attention to it. Apparently it is being used though so we should probably fix this. Thanks!

@Hades32

This comment has been minimized.

Copy link

@Hades32 Hades32 commented Jun 25, 2018

@davecramer thanks for taking care, what would the preferred way of using it be then?
Most (old?) tutorials for Wildfly and Postgres suggest using PGPoolingDataSource, what we are doing...

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Jun 25, 2018

@Hades32 if you wish you can use PGSimpleDataSource. That is not deprecated. If you want something with pooling which you likely do with an appserver such as Wildfly then you really should be using something like HikariCP, vibur-dbcp, commons-dbcp, c3p0. PGPoolingDataSource ( https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/ds/PGPoolingDataSource.java) is not very sophisticated. That said it appears that SimpleDataSource does not properly parse multiple hosts and should be fixed.

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Jun 26, 2018

So looking at this it would require a breaking change to BaseDataSource. getPort returns an int which clearly has no idea how to deal with port,port. Alternatively we could create another DataSource class which returned port as a String and thus be able to deal with multiple ports.

@Hades32

This comment has been minimized.

Copy link

@Hades32 Hades32 commented Jun 26, 2018

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Jun 26, 2018

I can see how to code that. I'm a little worried about the case where the port isn't the same ?

@Benny-Git

This comment has been minimized.

Copy link

@Benny-Git Benny-Git commented Mar 4, 2019

I have just spent a few hours trying to get this to work, and wondering why the failback to 5432 occurs on IBM Liberty.

Now with ?PGPORT=64000 I could get this port to work.

Maybe we can at least highlight this workaround in the documentation?

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Mar 5, 2019

@Benny-Git This seems rather random why does 64000 work ?

@Benny-Git

This comment has been minimized.

Copy link

@Benny-Git Benny-Git commented Mar 5, 2019

Sorry, didn't clarify: 64000 is the TCP port our Postgres instances are listening on. Requirement from our network team.

So specifying the port via PGPORT worked fine. Now I run into a similar issue with the hostnames, but that's a different topic.

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Mar 22, 2019

Hello, I seem to have the same problem with XADataSource; is this the same issue ?

Context: trying to use jdbc connection failover in a wildfly xa-datasource.
Obviously I cannot use the separate

<xa-datasource-property name="ServerName">localhost</xa-datasource-property>
<xa-datasource-property name="PortNumber">15432</xa-datasource-property>

but it seems to kind of work with
<xa-datasource-property name="Url">jdbc:postgresql://host1:15432,host2:25432/dbname?targetServerType=master&amp;ApplicationName=wildfly</xa-datasource-property>
except, well, it connects to port 5432.

If this is unrelated, I'll be happy to file a dedicated report, please let me know.

@Benny-Git

This comment has been minimized.

Copy link

@Benny-Git Benny-Git commented Mar 22, 2019

We use IBM Liberty, not sure how Wildfly handles this.
The connection string that finally worked for us is this:

jdbc:postgresql://notused/dbname?targetServerType=master&user=db_user&password=db_pw&PGPORT=INVALID&PGHOST=postgres_hostname:64000,postgres_hostname2:64000

As I said earlier, our Postgres instances run with Port 64000. I used the PGHOST parameter to specify both hostnames including their port, but also had to specify an invalid PGPORT parameter - if I specify 64000 there, it will only work for one hostname. If I leave out PGPORT altogether it will work as if I had provided 5432.

This workaround is so dirty however, that I currently consider using pgBouncer instead and using a Pause-ConfigReload-Resume instead to switch over between the instances.

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Mar 25, 2019

Hello I just checked the source a bit (sorry if I'm wrong somewhere - looking at it for the first time).

The BaseDataSource.getConnection() is calling DriverManager.getConnection(getUrl()) alright,
which would probably work for multiple servers and ports.
However BaseDataSource.setUrl() does not store the url as string, but rather after parsing it into properties (in org.postgresql.Driver.parseURL() which is still handling mutliple hosts/ports correctly) but then fails to account for multiple ports in setProperty() case PG_PORT.
Which is basically what joechen2010 wrote above.
then getUrl() reassembles an url from the (still correct) host and (now broken) port info.

so what BasicDataSource would have to be completely changed to get port as String or maybe int[]
Actually I didn't quite see why the separate int getPortNumber() and String getServerName() would be needed at all. they are just called from test cases and not mandated by interfaces.

but maybe a easier fix would be to just leave the url as a string ?
if aBasicDatasource.setUrl(aBasicDatasource.getUrl()) would not break the string everything would be fine ?

like

private String url;
public String getUrl() {
if (url == null) { //reassable as we did before, but only if url was not set explicitly.
    url = buildUrlFromProperties();
}
return url:
}
@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Apr 3, 2019

Hi @davecramer @vlsi , I made a (naive) attempt at fixing this, and it doesn't look so bad.
What do you think?
diff attached.
ds_multiports.txt

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Apr 3, 2019

@teicher it would be much better if you could provide us with a PR. This enables travis-ci to build it which at least proves it's compilable and doesn't break anything.

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Apr 3, 2019

@davecramer of course, but before I do that I would like to know if you trink this goes in the right direction - is a path that we should follow...

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Apr 3, 2019

@teicher it's much easier for me to read here than in a txt diff file..

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Apr 11, 2019

Hi @davecraner where do we go from here?
Is this sufficient to review and test the approach?

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented May 2, 2019

Hi @davecramer where do we go from here?
Is this sufficient to review and test the approach?
(sorry mistyped the name in the previous comment)

teicher added a commit to teicher/pgjdbc that referenced this issue Jun 21, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Jun 21, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Jun 21, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Jun 22, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Jun 22, 2019
teicher pushed a commit to teicher/pgjdbc that referenced this issue Jun 24, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
sehrope added a commit to sehrope/pgjdbc that referenced this issue Jul 17, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Jul 22, 2019
teicher pushed a commit to teicher/pgjdbc that referenced this issue Jul 22, 2019
teicher pushed a commit to teicher/pgjdbc that referenced this issue Jul 22, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Aug 28, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Aug 28, 2019
teicher pushed a commit to teicher/pgjdbc that referenced this issue Sep 9, 2019
teicher pushed a commit to teicher/pgjdbc that referenced this issue Sep 9, 2019
teicher added a commit to teicher/pgjdbc that referenced this issue Sep 9, 2019
davecramer added a commit that referenced this issue Sep 26, 2019
* fix DataSources broken by connection failover urls (#1039)

* removing java8 string join method and fixing indentation (#1039)

* preserve original test "bds" as we are modifying ours (#1039)

* storing old bds reference before changing it (#1039)

* moving the url-modifiying test out of BaseDataSourceTest (#1039)
@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Dec 6, 2019

@davecramer this is missing in the new release notes, probably because the status is still open.

@davecramer

This comment has been minimized.

@teicher

This comment has been minimized.

Copy link
Contributor

@teicher teicher commented Dec 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.