Skip to content

Commit

Permalink
perf: fix 1ms per async CopyAPI (regression since 42.2.5) (#1314)
Browse files Browse the repository at this point in the history
The problem is SSL sockets use buffers for input, and `stream#available()` might easily return 0 even though the data is available in the downstream socket. The only viable WA is to perform `read` calls, however it might impact performance (e.g. for async copy operations). The resolution is to throttle read calls for copy api, and keep 1ms reads for replication api.

fixes #1312
  • Loading branch information
vlsi committed Oct 25, 2018
1 parent e44e4e8 commit e2623d6
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -9,8 +9,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
### Added ### Added


### Fixed ### Fixed
- Fixed async copy performance regression [PR 1314](https://github.com/pgjdbc/pgjdbc/pull/1314)


## [42.2.5] (2018-08-27) ## [42.2.5] (2018-08-27)
### Known issues
- 1ms per async copy call [issue 1312](https://github.com/pgjdbc/pgjdbc/issues/1312)

### Changed ### Changed
- `ssl=true` implies `sslmode=verify-full`, that is it requires valid server certificate [cdeeaca4](https://github.com/pgjdbc/pgjdbc/commit/cdeeaca47dc3bc6f727c79a582c9e4123099526e) - `ssl=true` implies `sslmode=verify-full`, that is it requires valid server certificate [cdeeaca4](https://github.com/pgjdbc/pgjdbc/commit/cdeeaca47dc3bc6f727c79a582c9e4123099526e)


Expand Down
3 changes: 3 additions & 0 deletions docs/_posts/2018-08-27-42.2.5-release.md
Expand Up @@ -7,6 +7,9 @@ version: 42.2.5
--- ---
**Notable changes** **Notable changes**


### Known issues
- 1ms per async copy call [issue 1312](https://github.com/pgjdbc/pgjdbc/issues/1312)

### Changed ### Changed
- `ssl=true` implies `sslmode=verify-full`, that is it requires valid server certificate [cdeeaca4](https://github.com/pgjdbc/pgjdbc/commit/cdeeaca47dc3bc6f727c79a582c9e4123099526e) - `ssl=true` implies `sslmode=verify-full`, that is it requires valid server certificate [cdeeaca4](https://github.com/pgjdbc/pgjdbc/commit/cdeeaca47dc3bc6f727c79a582c9e4123099526e)


Expand Down
15 changes: 15 additions & 0 deletions pgjdbc/src/main/java/org/postgresql/core/PGStream.java
Expand Up @@ -44,6 +44,11 @@ public class PGStream implements Closeable, Flushable {
private OutputStream pg_output; private OutputStream pg_output;
private byte[] streamBuffer; private byte[] streamBuffer;


private long nextStreamAvailableCheckTime;
// This is a workaround for SSL sockets: sslInputStream.available() might return 0
// so we perform "1ms reads" once in a while
private int minStreamAvailableCheckDelay = 1000;

private Encoding encoding; private Encoding encoding;
private Writer encodingWriter; private Writer encodingWriter;


Expand Down Expand Up @@ -114,6 +119,12 @@ public boolean hasMessagePending() throws IOException {
return true; return true;
} }
// In certain cases, available returns 0, yet there are bytes // In certain cases, available returns 0, yet there are bytes
long now = System.currentTimeMillis();
if (now < nextStreamAvailableCheckTime && minStreamAvailableCheckDelay != 0) {
// Do not use ".peek" too often
return false;
}
nextStreamAvailableCheckTime = now + minStreamAvailableCheckDelay;
int soTimeout = getNetworkTimeout(); int soTimeout = getNetworkTimeout();
setNetworkTimeout(1); setNetworkTimeout(1);
try { try {
Expand All @@ -125,6 +136,10 @@ public boolean hasMessagePending() throws IOException {
} }
} }


public void setMinStreamAvailableCheckDelay(int delay) {
this.minStreamAvailableCheckDelay = delay;
}

/** /**
* Switch this stream to using a new socket. Any existing socket is <em>not</em> closed; it's * Switch this stream to using a new socket. Any existing socket is <em>not</em> closed; it's
* assumed that we are changing to a new socket that delegates to the original socket (e.g. SSL). * assumed that we are changing to a new socket that delegates to the original socket (e.g. SSL).
Expand Down
Expand Up @@ -28,11 +28,7 @@ public long endCopy() throws SQLException {
} }


public byte[] readFromCopy() throws SQLException { public byte[] readFromCopy() throws SQLException {
if (received.isEmpty()) { return readFromCopy(true);
queryExecutor.readFromCopy(this, true);
}

return received.poll();
} }


@Override @Override
Expand Down
Expand Up @@ -129,6 +129,8 @@ private void configureSocketTimeout(CommonOptions options) throws PSQLException
} }


pgStream.getSocket().setSoTimeout(minimalTimeOut); pgStream.getSocket().setSoTimeout(minimalTimeOut);
// Use blocking 1ms reads for `available()` checks
pgStream.setMinStreamAvailableCheckDelay(0);
} catch (IOException ioe) { } catch (IOException ioe) {
throw new PSQLException(GT.tr("The connection attempt failed."), throw new PSQLException(GT.tr("The connection attempt failed."),
PSQLState.CONNECTION_UNABLE_TO_CONNECT, ioe); PSQLState.CONNECTION_UNABLE_TO_CONNECT, ioe);
Expand Down

0 comments on commit e2623d6

Please sign in to comment.