Switch Nominatim connection handling to PGSimpleDataSource - #1117
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 709a827. Configure here.
|
|
||
| dataSource.setUrl(String.format("jdbc:postgresql://%s:%d/%s", | ||
| cfg.getHost(), cfg.getPort(), cfg.getDatabase())); | ||
| dataSource.setUsername(cfg.getUser()); |
There was a problem hiding this comment.
Removed autocommit breaks cursors
High Severity
Replacing BasicDataSource with PGSimpleDataSource drops the previous setDefaultAutoCommit(false) setup. With autocommit on, PostgreSQL JDBC ignores the connector’s large fetchSize, so Nominatim import queries that stream outside TransactionTemplate can load whole country result sets in memory and risk OOM or failed imports.
Reviewed by Cursor Bugbot for commit 709a827. Configure here.
There was a problem hiding this comment.
PGSimpleDataSource doesn't have a setDefaultAutoCommit function. And the source code seems to indicate that the default for autocommit is false. However, the part on ignoring fetchSize might have merit from what I see in the planet import. Need to investigate.


The JDBC postgres driver comes with its own DataSource implementation, which gives better access to the PostgreSQL connection parameters and, more importantly, allows using a pgpass file. This finally eliminates the need to write out the password on the command line and also means that a simple
photon importwill work in most of the cases, once the pgpass file is set up.Adapts the documentation to recommend the pgpass file.
Also adds some paranoia check for the host name. It was possible to break the JDBC connection string when using special characters in the host name. (And I'm not going to be surprised if somebody used this as a "feature" and will complain bitterly about the new check.)
This is a breaking change. It changes the default for the '-user' parameter from "nominatim" to empty and will break installations where users rely on the default behaviour. However, this is a good opportunity to switch to the more secure pgpass file.