-
Notifications
You must be signed in to change notification settings - Fork 839
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: Make warnings available as soon as they are received. #857
Conversation
|
This change makes use of the existing optimizations to operate in O(1) time for adding a new warning, it doesn't call PgResultSet#addWarning |
Here's what I mean: https://shipilev.net/blog/2014/safe-public-construction/#_safe_publication, https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#_conclusion_and_parting_thoughts, etc |
Until REL9.4.1210 warnings were available via Statement#getWarnings() and ResultSet#getWarnings() as soon as they were received from the server. This commit returns to that behavior. This is useful for long running queries, where it can be beneficial to know about a warning before the query completes. This addresses GH issue pgjdbc#856
Codecov Report
@@ Coverage Diff @@
## master #857 +/- ##
===========================================
+ Coverage 65.62% 65.8% +0.17%
+ Complexity 3551 3547 -4
===========================================
Files 166 167 +1
Lines 15248 15233 -15
Branches 2473 2471 -2
===========================================
+ Hits 10007 10024 +17
+ Misses 4062 4030 -32
Partials 1179 1179 |
Well I made some changes fixed the tests and rebased, this change is really just aimed at reclaiming the functionality available prior. I did however mark the PgStatement warnings volatile, which should help with potential cross thread visibility issues. |
I won't say "functionality was available prior". It is rather "it seemed like the functionality was available even though that availability was not intentional". |
@magJ , What thread semantics should be blessed to |
@vlsi I don't see a simple way of blessing the use of
I would simply recommend not calling The first issue can be fixed by synchronizing If you address the first issue then the second issue can be addressed by having the user hold a reference to the last warning they read and check if its SQLWarning lastProcessed = null;
while(statementIsExecuting) {
SQLWarning warn = statement.getWarnings();
//if next linked warning has value use that, otherwise keep using latest head
if (lastProcessed != null && lastProcessed.getNextWarning() != null) {
warn = lastProcessed.getNextWarning();
}
if(warn != null) {
processWarning(warn);
lastProcessed = warn;
if(warn == statement.getWarnings()) {
statement.clearWarnings();
}
}
} |
d2f54d5
to
adc8c37
Compare
Prior to this change, clearing warnings while a statement is executing could of resulted in a NullPointerException when the next warning was added, or warnings being added to an unreachable warning chain.
@vlsi I have added support for thread safe |
} | ||
super.handleCompletion(); | ||
public void handleCommandStatus(String status, int updateCount, long insertOID) { | ||
append(new ResultWrapper(updateCount, insertOID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refrain from method reordering. AFAIK handleCommandStatus
was not changed, however it is listed in the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just the way the diff algorithm interpreted the change, handleCompletion()
was removed and handleWarning()
was added, il have a play around with it so that it doesn’t appear so misleading.
Using the patience algorithm might help, otherwise il just place handleWarning()
below handleCommandStatus()
.
@Override | ||
public void run() { | ||
try { | ||
warning.set(preparedStatement.getWarnings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be just return preparedStatement.getWarnings()
(and could AtomicReference
be removed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, il just use a Callable
instead of a Runnable
.
try { | ||
warning.set(preparedStatement.getWarnings()); | ||
} catch (SQLException e) { | ||
fail("Exception thrown: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could exception be just propagated? Does fail
add some goods here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no real reason not to if I use a Callable
, but otherwise I cant throw a checked exception from a Runnable
executorService.shutdown(); | ||
|
||
assertNotNull(warning.get()); | ||
assertEquals(warning.get().getMessage(), "Test 1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is expected/actual order right here? Could you please use more meaningful messages and/or add an explanation message to assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
}); | ||
|
||
for (int i = 0; i < iterations; i++) { | ||
statement.addWarning(new SQLWarning("Warning " + i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you use addWarning
rather than running a SQL that produces warnings?
I think end-to-end test would make sense here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just because it was easier and faster for me to test locally, I agree it makes more sense to test it end-to-end.
lastProcessed = warn; | ||
if (warn == statement.getWarnings()) { | ||
//System.out.println("Clearing warnings"); | ||
statement.clearWarnings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can understand, a warning can be lost in the following scenario:
Precondition: satement has chain of 5 warnings.
- Thread 1 acquires
st.getWarnings()
- Thread 2 acquires
st.getWarnings()
and startsaddWarning
. Unfortunately it gets suspended. - Thread 1 performs clear warnings
- Thread 1 walks
getNextWarning
chain up to the very end, and "processes" all the warnings - Thread 2 acquires CPU and adds yet another warning to the chain (the chain head was obtained at step 2)
Thread 1 will never see the warning from step 5 since it did walk through the chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "suspended" are you referring to Thread#suspend()
, or something else?
Because I don't think there is really much that can be done about the former, it's a deprecated API for good reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to simulate your scenario by manually adding delay into the PgStatement#addWarning
method after it acquires the reference to the warning wrapper and also adding some delay between the checking of lastProcesed.getNextWarning()
and the statement.getWarnings()
call.
It was indeed possible for a very unlikely race to cause a warning to be missed.
I have fixed that issue by doing the warn = statement.getWarnings()
assignment up front, so there is no way for a racing thread to add any warnings between the check and the assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed that issue by doing the warn = statement.getWarnings() assignment up front, so there is no way for a racing thread to add any warnings between the check and the assignment.
I am afraid I do not see how the additional warn = statement.getWarnings()
solves the issue.
The issue is addWarning
thread might wait infinitely long before actually adding a warning. The only check "receiver" does it checks lastProcessed.getNextWarning()
, however that gives absolutely no clue on whether new warnings might be added or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well there are only two places a new warning can be added, onto the previous chain or added as the new head of the chain, lastProcessed
will be the previous chain.
This algorithm only needs to handle a single-producer scenario, if addWarning
takes an indeterminate amount of time, then the consumer will just spin until the producer finally wakes up.
#getNextWarning()
and statement.getWarnings()
are both volatile reads, so even if two new warnings are added, one to the previous chain and one to the new head, we wont start using the new head until everything is read from the last chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I see now.
I wonder how many users would be able to replicate this warning consumer loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be unlikely someone would replicate it without looking at this unit test.
Concurrently reading warnings from a statement is probably a fairly niche feature, I think for the majority of use-cases it is unlikely that someone is going to generate enough notices that they would consider using Statement#clearWarnings()
, but if they do I think the important thing is that the driver doesn’t crash, and we at least provide some way of the user reading the warnings safely, despite how abstruse the implementation might be.
I considered other approaches like employing locks/synchronization or maybe even exposing a queue data structure, but I felt that those options would introduce negative performance implications for users who don’t need the feature, or in the case of exposing a queue, wouldn’t work with the standard JDBC API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will work on incorporating your feedback when I get a bit of time, hopefully within the next couple of days.
} | ||
super.handleCompletion(); | ||
public void handleCommandStatus(String status, int updateCount, long insertOID) { | ||
append(new ResultWrapper(updateCount, insertOID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just the way the diff algorithm interpreted the change, handleCompletion()
was removed and handleWarning()
was added, il have a play around with it so that it doesn’t appear so misleading.
Using the patience algorithm might help, otherwise il just place handleWarning()
below handleCommandStatus()
.
}); | ||
|
||
for (int i = 0; i < iterations; i++) { | ||
statement.addWarning(new SQLWarning("Warning " + i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just because it was easier and faster for me to test locally, I agree it makes more sense to test it end-to-end.
@Override | ||
public void run() { | ||
try { | ||
warning.set(preparedStatement.getWarnings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, il just use a Callable
instead of a Runnable
.
try { | ||
warning.set(preparedStatement.getWarnings()); | ||
} catch (SQLException e) { | ||
fail("Exception thrown: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no real reason not to if I use a Callable
, but otherwise I cant throw a checked exception from a Runnable
executorService.shutdown(); | ||
|
||
assertNotNull(warning.get()); | ||
assertEquals(warning.get().getMessage(), "Test 1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
lastProcessed = warn; | ||
if (warn == statement.getWarnings()) { | ||
//System.out.println("Clearing warnings"); | ||
statement.clearWarnings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say "suspended" are you referring to Thread#suspend()
, or something else?
Because I don't think there is really much that can be done about the former, it's a deprecated API for good reason.
If the producer thread added multiple warnings after the check of lastProcessed.getNextWarning, then it was possible for warnings to be missed by the consumer thread.
@vlsi I have incorporated your feedback, please review. |
|
||
assertNotNull(warning); | ||
assertEquals("First warning received not first notice raised", | ||
warning.getMessage(), "Test 1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the proper expected, actual argument order: http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String, java.lang.Object, java.lang.Object)
lastProcessed = warn; | ||
if (warn == statement.getWarnings()) { | ||
//System.out.println("Clearing warnings"); | ||
statement.clearWarnings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed that issue by doing the warn = statement.getWarnings() assignment up front, so there is no way for a racing thread to add any warnings between the check and the assignment.
I am afraid I do not see how the additional warn = statement.getWarnings()
solves the issue.
The issue is addWarning
thread might wait infinitely long before actually adding a warning. The only check "receiver" does it checks lastProcessed.getNextWarning()
, however that gives absolutely no clue on whether new warnings might be added or not.
} | ||
if (warn != null) { | ||
warnings++; | ||
//System.out.println("Processing " + warn.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add assert for each received warning
@Override | ||
public void handleWarning(SQLWarning warning) { | ||
super.handleWarning(warning); | ||
PgResultSet.this.warnings = this.getWarning(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, this might cause double-processing via the following scenario:
- Consumer consumes all the warnings, and clears the chain
- Producer handles yet another warning, and
PgResultSet.this.warnings = this.getWarning();
write results in the same head being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for ResultSet.getWarnings()
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vlsi I decided to revert the change to ResultSet
because I couldn't actually come up with test a scenario where it made sense.
I'm not sure it is even possible for postgres to raise warnings while a result set is reading from a cursor.
* of calling #setNextWarning on the head. By encapsulating this into a single object it allows | ||
* users(ie PgStatement) to atomically set and clear the warning chain. | ||
*/ | ||
public class PGSQLWarningWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be PSQLWarningWrapper? We already have PSQLWarning, PSQLState, PSQLSavepoint, ...
On the other hand, I'm sure this (PGSQLWarningWrapper
) class should not be a part of public API, so it might make sense to put it to some .internal.
package (or specify in the javadoc that this class should not be used by end clients)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving it to org.postgresql.jdbc.PSQLWarningWrapper
and making the class package-private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If package-private is possible, please do that.
+ "BEGIN " | ||
+ "RAISE NOTICE 'Test 1'; " | ||
+ "RAISE NOTICE 'Test 2'; " | ||
+ "EXECUTE pg_sleep(2); " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative approach would be to use two connections, and make sure the outer transaction holds a row lock preventing notify_then_sleep from making progress.
That would ensure Java gets warnings before statement finish.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, I have now implemented this, it should speed up the test a bit and make it a little more robust.
lastProcessed = warn; | ||
if (warn == statement.getWarnings()) { | ||
//System.out.println("Clearing warnings"); | ||
statement.clearWarnings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I see now.
I wonder how many users would be able to replicate this warning consumer loop.
Revert change to ResultSet, as could nto conceive of scenario where it would be needed. Make PSQLWarningWrapper package-private and moved namespace. Refactored test to use a lock rather than rely on timing, should make the test faster and more robust.
con.createStatement().execute("SET SESSION statement_timeout = 1000"); | ||
final PreparedStatement preparedStatement = con.prepareStatement("SELECT notify_then_sleep()"); | ||
ExecutorService executorService = Executors.newSingleThreadExecutor(); | ||
Future<Void> future = executorService.submit(new Callable<Void>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
submit Runnable
instead of Callable<Void>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexElin Callable
is used because the method throws a checked exception, using a Runnable
would require catching the exception and manually fail()
ing the test, it was changed to Callable
based on earlier feedback from vlsi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
//If test takes longer than 2 seconds its a failure. | ||
future.get(2, TimeUnit.SECONDS); | ||
executorService.shutdown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have executorService
as a field and shutdown in @After
, to make sure it's always executed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to introduce a new field that is only used in a couple of tests.
This test class currently only has one field which is used in seemingly every test.
The executor will only not be shutdown if the test fails, so it shouldn't really be an issue.
Additionally the the executor will be automatically shut down when its finalizer runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a try-finally condition to ensure the executor is shut down.
SQLWarning next = warning.getNextWarning(); | ||
if (next != null) { | ||
assertEquals("Second warning received not second notice raised", | ||
"Test 2", warning.getNextWarning().getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/warning.getNextWarning()/next/
lastProcessed = warn; | ||
if (warn == statement.getWarnings()) { | ||
//System.out.println("Clearing warnings"); | ||
statement.clearWarnings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious to me what happens in the loop here and when warnings are cleared, etc. Would it be possible to improve readability somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings are cleared if the warning just processed is the head of the warning chain, does that explanation help?
I can add a comment to that affect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can see this condition, but trying to understand how often it happens. will try more making notes on paper :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often it happens depends on a few factors, I tried to explain it a bit in the javadoc comment.
Some factors that affect it are:
- How long the users "process" function takes.
- If they opt to add some delay between each
warn == null
iteration. - OS thread sheduling.
- Burstiness of the received warnings.
Overall it's fairly random, im not sure how to describe it in a useful way.
Implement PR feedback.
public Void call() throws SQLException { | ||
while (true) { | ||
SQLWarning warning = preparedStatement.getWarnings(); | ||
if (warning != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of failure, this will spin forever.
Please add polling delay and make the thread finish in case of "no warnings received for some reason"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread should now be interrupted upon shutting down of the executor.
final int iterations = 1000; | ||
final ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
con.createStatement() | ||
.execute("CREATE OR REPLACE FUNCTION notify_loop() RETURNS VOID AS " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the procedure be removed from the database after the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added function removal to the @After
test section.
Ensure that warning reader threads are shut down in case of test failure Remove tables and functions created during the tests.
@vlsi Thanks for all your detailed feedback, I appreciate that there may be more important issues that command your attention, but I would rather not have this PR open for an extended period of time. |
@vlsi is on holidays for a few more days
Dave Cramer
…On 22 August 2017 at 20:46, Magnus ***@***.***> wrote:
@vlsi <https://github.com/vlsi> Thanks for all your detailed feedback, I
appreciate that there may be more important issues that command your
attention, but I would rather not have this PR open for an extended period
of time.
Could you please let me know what if anything I can do to have this change
merged?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#857 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAYz9rSkCF6SVa_JuMWUMvFo270nnESIks5sa3ZcgaJpZM4OKNe9>
.
|
Any news? |
Until REL9.4.1210 warnings were available via Statement#getWarnings()
and ResultSet#getWarnings() as soon as they were received from the
server. This commit returns to that behavior.
This is useful for long running queries, where it can be beneficial
to know about a warning before the query completes.
This addresses GH issue #856