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

Fix:save points causing server to run out of resources #1409

Merged
merged 10 commits into from Feb 24, 2019

Conversation

Projects
None yet
6 participants
@davecramer
Copy link
Member

commented Feb 5, 2019

Fixes #1407

All Submissions:

  • [x ] Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. [X ] Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?

Changes to Existing Features:

  • [NO] Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • [NO] Have you written new tests for your core changes, as applicable?
  • [YES] Have you successfully run tests with your changes locally?
@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 5, 2019

&& getAutoSave() == AutoSave.ALWAYS
&& getTransactionState() == TransactionState.OPEN) {
try {
sendOneQuery(releaseAutoSave, SimpleQuery.NO_PARAMETERS, 1, 0,

This comment has been minimized.

Copy link
@vlsi

vlsi Feb 5, 2019

Member

@davecramer , does that imply network roundtrip?

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 5, 2019

Author Member

I'm afraid it does. I guess this is why you wanted to do this every 1000 times instead?

This comment has been minimized.

Copy link
@vlsi

vlsi Feb 5, 2019

Member

exactly

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 5, 2019

Author Member

Let me see what I can do...

return true;
}
return false;
}

private void releaseSavePoint(boolean autosave, int flags) throws SQLException {
if (autosave
&& (flags & QueryExecutor.QUERY_SUPPRESS_BEGIN) == 1

This comment has been minimized.

Copy link
@fb-datax

fb-datax Feb 6, 2019

After changing the check for the QUERY_SUPPRESS_BEGIN to 0 the changes fix the issue. Otherwise the savepoint is never released

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 6, 2019

Author Member

Sadly that doesn't pass the test suite...

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@vlsi so looking at this it appears we send a savepoint before commit which could be removed.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 6, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Feb 6, 2019

Codecov Report

Merging #1409 into master will increase coverage by 0.05%.
The diff coverage is 62.79%.

@@             Coverage Diff              @@
##             master    #1409      +/-   ##
============================================
+ Coverage     68.71%   68.76%   +0.05%     
- Complexity     3892     3913      +21     
============================================
  Files           179      179              
  Lines         16397    16436      +39     
  Branches       2668     2676       +8     
============================================
+ Hits          11267    11303      +36     
  Misses         3881     3881              
- Partials       1249     1252       +3
@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 6, 2019

@fb-datax

This comment has been minimized.

Copy link

commented Feb 7, 2019

This implementation breaks the whole Savepoint feature for the driver. The PGSQL documentation states "RELEASE SAVEPOINT also destroys all savepoints that were established after the named savepoint was established".

The following test demonstrates the issue:

@Test
public void testPgsqlJdbcSavepoint() throws Exception {
	
	Connection conn = TestUtil.openDB();
	
	BaseConnection baseConnection = conn.unwrap(BaseConnection.class);
	baseConnection.setAutosave(AutoSave.ALWAYS);
	baseConnection.setAutoCommit(false);
	
	TestUtil.createTable(conn, "rollbacktest", "a int, str text");
	
	int iterations = 1000;
	
	for (int i = 0; i < iterations; i++) {
		long startTime = System.nanoTime();
		
		Statement statement = conn.createStatement();
		statement.executeUpdate("insert into rollbacktest(a, str) values (" + i + ", '" + UUID.randomUUID() + "')");
		statement.close();
		
		if (i == 5) {
			Statement savePoint = conn.createStatement();
			savePoint.executeUpdate("SAVEPOINT NEW_SAVEPOINT");
			savePoint.close();
		}
		
		long timeElapsed = System.nanoTime() - startTime;
		System.out.println(i + "/" + iterations + " took " + timeElapsed / 1000000 + "ms ");
	}
	
	Statement savePoint = conn.createStatement();
	savePoint.executeUpdate("ROLLBACK TO NEW_SAVEPOINT");
	savePoint.close();
}
@davecramer

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@fb-datax good catch
So the alternative is an additional round trip per statement.

@fb-datax

This comment has been minimized.

Copy link

commented Feb 7, 2019

@davecramer
I think that his is the only viable solution. If there are performance concerns, what about making the cleanup optional through a jdbc connection setting?

@bokken

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

What if the approach was to do the "release" /before/ creating the 1000th savepoint?
This wipes out all 1000 savepoints, but that does not really matter as we no longer want to rollback to any of those.

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@bokken Unfortunately releasing all of them also releases SAVEPOINTS set by setSavePoint.

@bokken

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@davecramer, could we limit to the "auto save" workflow (i.e. no manual calls to setSavePoint)? That really seems to be the problematic workflow. Where a user has auto save, and then proceeds to execute thousands of things in a single transaction.

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@bokken in theory, How do you propose we disable setSavePoint? Throw an exception ?

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 7, 2019

@bokken

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

I am not suggesting we disable setSavePoint, I am suggesting that we stop automatically releasing savepoints once setSavePoint has been explicitly called.
Having said that, perhaps the automatic release could just go back to the explicit savepoint as opposed to any automatically created savepoint.

@bokken

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

And perhaps the explicit call to setSavePoint releases any/all automatic savepoints first, giving a clean baseline.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 17, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 17, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 17, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 17, 2019

@davecramer

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

And perhaps the explicit call to setSavePoint releases any/all automatic savepoints first, giving a clean baseline.

I think this seems reasonable... I'll work on implementing this.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 18, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 18, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 18, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 18, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 19, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 24, 2019

@davecramer davecramer force-pushed the davecramer:fixsafepoints branch from aca6059 to 95478df Feb 24, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 24, 2019

@davecramer davecramer merged commit af8f883 into pgjdbc:master Feb 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -111,6 +116,7 @@ public String getSql(ReturnColumns cols) {
}

private final AutoSave autoSave;
private final CleanSavePoint cleanSavePoint;

This comment has been minimized.

Copy link
@vlsi

vlsi Jun 18, 2019

Member

@davecramer , As far as I can tell, you create a field, but this field seems to be unused.

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