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

Expose parameter status messages (GUC_REPORT) to the user #1435

Merged
merged 2 commits into from Jul 5, 2019

Conversation

@ringerc
Copy link
Member

commented Mar 12, 2019

Add a new Map PGConnection.getParameterStatuses() method that tracks the
latest values of all GUC_REPORT parameters reported by the server in
ParameterStatus protocol messages from the server. The map is read-only. A
convenience PGConnection.getParameterStatus(String) wrapper is also provided.

This provides a PgJDBC equivalent to the PQparameterStatus(...) libpq API
function.

Extensions may define custom GUCs that are set as GUC_REPORT when they
DefineCustomStringVariable(...) etc. This feature will properly handle such
GUCs, allowing applications to generate parameter status change messages in
their extensions and have them available to the JDBC driver without needing
round trips.

No assumptions are made about which server GUCs are GUC_REPORT or their
names, so it'll work (with possible test case tweaks) on current and future
server versions.

Github issue #1428

All Submissions:

  • 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. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?

Changes to Existing Features:

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

This comment has been minimized.

Copy link

commented Mar 12, 2019

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from 074db41 to 59674ee Mar 12, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 12, 2019

@davecramer

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@ringerc I'll try to look at this ASAP, off the top though, we need remove the tests for 8.3


String getParameterStatus(String parameterName);

//void setParameterStatus(String parameterName, String parameterStatus);

This comment has been minimized.

Copy link
@sehrope

sehrope Mar 12, 2019

Contributor

Old commented out method should be removed.

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Haven't tried building it but skimmed through the code and so far looks good. Found one commented out method that should be removed (separate comment for that; guessing its from older version of putting this together).

Why are the session_authorization and is_superuser GUCs blacklisted? It's the user's app so if they want to reference those things, why not? It's not like they can't run SHOW is_superuser and get it themselves right?

@@ -0,0 +1,152 @@
/*
* Copyright (c) 2018, PostgreSQL Global Development Group

This comment has been minimized.

Copy link
@sehrope

sehrope Mar 12, 2019

Contributor

s/2018/2019/

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Overall looks good. Builds locally and new tests pass. Only things I found were those small cosmetic ones.

Also, I don't have a particular opinion on exposing those two GUCs. I'm just not sure why they're being handled separately.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I'll fix the issue with the commented-out code. Whoops.

As for special casing of is_superuser and session_authorization that's because of the comments in guc.c:

/* Not for general use --- used by SET SESSION AUTHORIZATION */

However, I checked src/interfaces/libpq and its PQparameterStatus doesn't do anything like that. So I'll remove that logic.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

They're also documented in the libpq manual so I'll follow that lead.

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from 59674ee to f21715b Mar 13, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 13, 2019

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Failing on 8.2, 8.3, and 8.4 with

transactionalParametersCommit(org.postgresql.test.jdbc2.ParameterStatusTest)  Time elapsed: 0.018 sec  <<< FAILURE!
java.lang.AssertionError: expected:<Driver Tests> but was:<null>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.ParameterStatusTest.transactionalParametersCommon(ParameterStatusTest.java:89)
	at org.postgresql.test.jdbc2.ParameterStatusTest.transactionalParametersCommit(ParameterStatusTest.java:124)
transactionalParametersRollback(org.postgresql.test.jdbc2.ParameterStatusTest)  Time elapsed: 0.014 sec  <<< FAILURE!
java.lang.AssertionError: expected:<Driver Tests> but was:<null>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.ParameterStatusTest.transactionalParametersCommon(ParameterStatusTest.java:89)
	at org.postgresql.test.jdbc2.ParameterStatusTest.transactionalParametersRollback(ParameterStatusTest.java:109)
reportUpdatedParameters(org.postgresql.test.jdbc2.ParameterStatusTest)  Time elapsed: 0.021 sec  <<< ERROR!
org.postgresql.util.PSQLException: ERROR: unrecognized configuration parameter "application_name"
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2470)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2213)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:309)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:446)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:370)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:311)
	at org.postgresql.jdbc.PgStatement.executeCachedSql(PgStatement.java:297)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:274)
	at org.postgresql.jdbc.PgStatement.executeUpdate(PgStatement.java:246)
	at org.postgresql.test.jdbc2.ParameterStatusTest.reportUpdatedParameters(ParameterStatusTest.java:73)
expectedInitialParameters(org.postgresql.test.jdbc2.ParameterStatusTest)  Time elapsed: 0.017 sec  <<< FAILURE!
java.lang.AssertionError: expected:<Driver Tests> but was:<null>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.ParameterStatusTest.expectedInitialParameters(ParameterStatusTest.java:49)
transactionalParametersAutocommit(org.postgresql.test.jdbc2.ParameterStatusTest)  Time elapsed: 0.019 sec  <<< FAILURE!
java.lang.AssertionError: expected:<Driver Tests> but was:<null>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.ParameterStatusTest.transactionalParametersAutocommit(ParameterStatusTest.java:141)
@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

application_name was added in 9.0, so that's why.

I'll make that test conditional on version. I'm tempted to just skip the whole test on ancient postgres, but we might as well test the initial parameters and just skip the application_name and transactional boundary stuff.

WIP

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from f21715b to cd67795 Mar 14, 2019

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Modified to apply appropriate server version filters. I hadn't realised PgJDBC tested as far back as 8.2; that's impressive actually. Building.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 14, 2019

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from cd67795 to d427b45 Mar 14, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 14, 2019

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

I've seen this connection timeout test failure in my local test runs too, it's an unrelated spurious failure:

testShortQueryTimeout(org.postgresql.test.jdbc2.StatementTest)  Time elapsed: 0.219 sec  <<< ERROR!
org.postgresql.util.PSQLException: ERROR: canceling statement due to user request
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2470)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2213)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:309)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:446)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:370)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:311)
	at org.postgresql.jdbc.PgStatement.executeCachedSql(PgStatement.java:297)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:274)
	at org.postgresql.jdbc.PgStatement.executeQuery(PgStatement.java:225)
	at org.postgresql.test.jdbc2.StatementTest.testShortQueryTimeout(StatementTest.java:737)

that I suspect is a test bug due to host timing/scheduling/load. The test probably needs a couple of retries.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Seen this failure twice too

Failed tests: 
  DatabaseMetaDataTest.testCrossReference:242 expected:<ww_m_[]fkey> but was:<ww_m_[n_]fkey>
@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 14, 2019

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from a3a68ce to ee63ba2 Mar 14, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 14, 2019

* @return unmodifiable map of case-insensitive parameter names to parameter values
* @since 42.2.6
*/
Map<String,String> getParameterStatuses();

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 14, 2019

Member

Isn't getParameterStatus(String parameterName) just enough?
Is the returned Map live?

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 15, 2019

Author Member

The map returned is an unmodifiable wrapper.

If we don't expose the map then we should provide a List<String>-returning call to list known params instead. Just exposing the map is simpler.

This comment has been minimized.

Copy link
@bokken

bokken Mar 19, 2019

Member

What is a bit wonky is the handling of case sensitivity.
While we can certainly provide an implementation of String getParameterStatus(String param) which is case insensitive to param, getting the Map.keySet() off the return value here would return some specific case when iterating values.
I think I would lean towards 1 method to return the known parameters and a separate method to get the value for some given parameter in a case insensitive way.
This provides the needed/desired functionality and hides the implementation detail.

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 19, 2019

Member

It looks sane to return "server-provided" casing, and allow for case insensitive access, doesn't it?

ResultSet.getInt(String) allows for case-insensitive access.

This comment has been minimized.

Copy link
@bokken

bokken Mar 19, 2019

Member

@vlsi, I am good with the getParamterStatus(String) method being case insensitive to the parameter. I am a bit conflicted on returning a Map with all values and making any statement on it providing a case insensitive get.

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 19, 2019

Member

@bokken 👍
I'm inclining to a map with properties of:

  1. "server-native" casing
  2. "forbidden modifications"
  3. "might become out of date"

WHYT?

This comment has been minimized.

Copy link
@bokken

bokken Mar 20, 2019

Member

@vlsi, I think my preference would still be to return a collection of parameter names (with properties you list) to get the available names. But I am fine with the map you describe.

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 27, 2019

Author Member

We got rid of forbidden modifications. And I really fail to see the point of the rest, it seems excessively complex.

I take your point about keySet() returning the server-provided cases. But I don't really see the problem with it. The case won't tend to vary as the server doesn't change the case it uses, and the keys will get the correct parameters when looked up. The only possible issue I see is someReturnedKey.equals("APPLICATION_NAME") not matching ... and that's no different to how the system already behaves with a query of pg_settings.

@@ -52,6 +55,10 @@
private final LruCache<Object, CachedQuery> statementCache;
private final CachedQueryCreateAction cachedQueryCreateAction;

// For getParameterStatuses(), GUC_REPORT tracking
private final TreeMap<String,String> parameterStatuses
= new TreeMap<String,String>(String.CASE_INSENSITIVE_ORDER);

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 14, 2019

Member

Why is it a TreeMap?
Should the field be just Map and the value just HashMap?

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 15, 2019

Author Member

TreeMap is used to permit String.CASE_INSENSITIVE_ORDER, which is not supported by HashMap. That ensures that parameter lookups will find the parameter no matter what case is used - datestyle or DateStyle for example.

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 15, 2019

Author Member

Pg's parameters use a mix of camel case and underscore separators. Users generally expect case-folding behaviour in postgres, too. So I'd strongly prefer to do this.

* <p>A future version may invoke a client-defined listener class at this point,
* so this should be the only access path.</p>
*
* <p>Keys are case-insensitive and case-preserving.</p>

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 14, 2019

Member

What does Keys are case-insensitive mean?
Does backend send the same parameter with multiple casing variations?

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 15, 2019

Author Member

Applications may use parameters in any combination of letter cases and expect the same result. So the PgJDBC interface should be consistent with that, otherwise users will experience unexpected results if they

stmt.execute("SET Application_Name = 'ILoveCamelCase';");
// then in a more sensible part of the application
pgconn.getParameterStatus("application_name");

Observe

SET APPLICATION_NAME='myapp';
SHOW aPpLiCaTiON_name;
SHOW application_name;

As Java provides a trivial way to make map keys case-insensitive and case-preserving it makes sense to do so.

* @see org.postgresql.PGConnection#getParameterStatuses
* @see org.postgresql.PGConnection#getParameterStatus
*/
protected void setParameterStatus(String parameterName, String parameterStatus) {

This comment has been minimized.

Copy link
@vlsi

vlsi Mar 14, 2019

Member

I guess the naming implies the method would send the parameter status to the database, while the actual meaning is the other way around.
onParameterStatus might be better.

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 15, 2019

Author Member

Good point, Java accessor convention. Will do.

@vlsi

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

In general it looks good. I don't really think we should expose Map, should we?
Are there use cases for that?

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Without the Map there's no way to get all the parameter names (and values) as they're not necessarily known to the client. The server could set the value for a custom parameter (ex: for a fork) that the client does not know of and a user program may want to show that in a debug context.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 14, 2019

Codecov Report

Merging #1435 into master will increase coverage by 1.25%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master    #1435      +/-   ##
============================================
+ Coverage     68.79%   70.05%   +1.25%     
- Complexity     3937     5372    +1435     
============================================
  Files           179      179              
  Lines         16466    20182    +3716     
  Branches       2674     3669     +995     
============================================
+ Hits          11328    14138    +2810     
- Misses         3895     4609     +714     
- Partials       1243     1435     +192
@vlsi

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Without the Map there's no way to get all the parameter names

That's true.
My initial thought was client did know which parameters were needed.

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from ee63ba2 to de0665d Mar 15, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 15, 2019

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Tests pass (with the follow-up commits that fix test problems), and issues raised have been resolved.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@davecramer and @vlsi I think this is good to go now

throw new IllegalStateException("attempt to set GUC_REPORT parameter with null or empty-string name");
}

parameterStatuses.put(parameterName, parameterStatus);

This comment has been minimized.

Copy link
@bokken

bokken Mar 19, 2019

Member

do we have any potential concurrency issue here?
IIRC, query executors are used/reused for life of connection, which can get used across threads (particularly when connections pooled). Here we are potentially mutating the map on different threads from where it is being accessed/read and giving a live "view" of the map on the read method.

This comment has been minimized.

Copy link
@ringerc

ringerc Mar 27, 2019

Author Member

@bokken At any point where onParameterStatus may be called, the current thread holds the monitor of the object guarding the entrypoint. We can only get parameter status messages during query result processing or when processing notices by explicit user request in processNotifies(). The latter is synchronized on QueryExecutorImpl, and all the querying entry points also take the QueryExecutorImpl monitor. They have to, otherwise we'd have a horrible mess on our hands already.

No query executor can be used for more than one connection, and no connection may run more than one statement at a time or process results from more than one statement.

You could annotate it synchronized but that'd be (a) unnecessary and (b) if it was necessary, wrong and likely to cause deadlocks.

This comment has been minimized.

Copy link
@bokken

bokken Mar 27, 2019

Member

@ringerc, my concern was not necessarily concurrent threads calling onParamterStatus, but rather consistency between reading from the map on a different thread from where it was mutated.

ringerc added 2 commits Mar 8, 2019
Expose parameter status messages (GUC_REPORT) to the user
Add a new `Map PGConnection.getParameterStatuses()` method that tracks the
latest values of all `GUC_REPORT` parameters reported by the server in
`ParameterStatus` protocol messages from the server. The map is read-only. A
convenience `PGConnection.getParameterStatus(String)` wrapper is also provided.

This provides a PgJDBC equivalent to the `PQparameterStatus(...)` `libpq` API
function.

Extensions may define custom GUCs that are set as `GUC_REPORT` when they
`DefineCustomStringVariable(...)` etc. This feature will properly handle such
GUCs, allowing applications to generate parameter status change messages in
their extensions and have them available to the JDBC driver without needing
round trips.

No assumptions are made about which server GUCs are `GUC_REPORT` or their
names, so it'll work (with possible test case tweaks) on current and future
server versions.

Github issue #1428
Attempt to make StatementTest.testShortQueryTimeout() more reliable
I'm observing intermittent failures like:

    testShortQueryTimeout(org.postgresql.test.jdbc2.StatementTest)  Time elapsed: 0.219 sec  <<< ERROR!
    org.postgresql.util.PSQLException: ERROR: canceling statement due to user request

The cause of that isn't yet clear. But I noticed that the test doesn't check
for the SQLSTATE of the expected exception, so fix that.

@ringerc ringerc force-pushed the ringerc:dev/pgjdbc-1428-rm-7814-rt-64017 branch from de0665d to 4ccff98 Mar 27, 2019

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Rebased.

BTW @sehrope forgot to retain author metadata etc in 42d6bfa when picking out of this PR - I don't overly mind, but please keep it in mind in future.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Mar 27, 2019

@sehrope

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@ringerc I think it was a timing issue as I had ran into it myself trying out some encoding related changes. I saw your note regarding the test failures and then a few hours later, when I pushed a separate PR for the FK name changes, I guess you had also pushed one to this branch. I chalk up the names being similar to great minds thinking alike 😄.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@sehrope Ha, that's hilarious. No worries.

@ringerc

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@vlsi and @davecramer I think this is merge-ready. May I have your approval to merge?

@davecramer

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

anyone have any issues with merging this ?

@vlsi

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

LGTM

@vlsi

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

PS. It would be great if changelog could be updated as well (== you could include relevant note to CHANGELOG.md)

@davecramer davecramer merged commit ce8333a into pgjdbc:master Jul 5, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/project 70.05% (+1.25%) compared to bb018f7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.