Expose parameter status messages (GUC_REPORT) to the user #1435
Conversation
|
✅ Build pgjdbc 1.0.160 completed (commit 479265df93 by @ringerc) |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
074db41
to
59674ee
Compare
|
✅ Build pgjdbc 1.0.161 completed (commit 9be0206670 by @ringerc) |
|
@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); |
There was a problem hiding this comment.
Old commented out method should be removed.
|
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 |
| @@ -0,0 +1,152 @@ | |||
| /* | |||
| * Copyright (c) 2018, PostgreSQL Global Development Group | |||
|
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. |
|
I'll fix the issue with the commented-out code. Whoops. As for special casing of
However, I checked |
|
They're also documented in the libpq manual so I'll follow that lead. |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
59674ee
to
f21715b
Compare
|
✅ Build pgjdbc 1.0.163 completed (commit 7a59a5702b by @ringerc) |
|
Failing on 8.2, 8.3, and 8.4 with |
|
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 WIP |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
f21715b
to
cd67795
Compare
|
Modified to apply appropriate server version filters. I hadn't realised PgJDBC tested as far back as 8.2; that's impressive actually. Building. |
|
✅ Build pgjdbc 1.0.164 completed (commit f3b61dbee1 by @ringerc) |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
cd67795
to
d427b45
Compare
|
❌ Build pgjdbc 1.0.165 failed (commit 3908c5650c by @ringerc) |
|
I've seen this connection timeout test failure in my local test runs too, it's an unrelated spurious failure: that I suspect is a test bug due to host timing/scheduling/load. The test probably needs a couple of retries. |
|
Seen this failure twice too |
|
❌ Build pgjdbc 1.0.166 failed (commit 4ac3d32674 by @ringerc) |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
a3a68ce
to
ee63ba2
Compare
|
✅ Build pgjdbc 1.0.167 completed (commit cf945dcc06 by @ringerc) |
| * @return unmodifiable map of case-insensitive parameter names to parameter values | ||
| * @since 42.2.6 | ||
| */ | ||
| Map<String,String> getParameterStatuses(); |
There was a problem hiding this comment.
Isn't getParameterStatus(String parameterName) just enough?
Is the returned Map live?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@bokken 👍
I'm inclining to a map with properties of:
- "server-native" casing
- "forbidden modifications"
- "might become out of date"
WHYT?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
Why is it a TreeMap?
Should the field be just Map and the value just HashMap?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
What does Keys are case-insensitive mean?
Does backend send the same parameter with multiple casing variations?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, Java accessor convention. Will do.
pgjdbc/src/test/java/org/postgresql/test/jdbc2/ParameterStatusTest.java
Outdated
Show resolved
Hide resolved
pgjdbc/src/test/java/org/postgresql/test/jdbc2/ParameterStatusTest.java
Outdated
Show resolved
Hide resolved
pgjdbc/src/test/java/org/postgresql/test/jdbc2/ParameterStatusTest.java
Outdated
Show resolved
Hide resolved
|
In general it looks good. I don't really think we should expose |
|
Without the |
Codecov Report
@@ 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 |
That's true. |
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
ee63ba2
to
de0665d
Compare
|
✅ Build pgjdbc 1.0.170 completed (commit e297180bda by @ringerc) |
|
Tests pass (with the follow-up commits that fix test problems), and issues raised have been resolved. |
|
@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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
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.
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 pgjdbc#1428
ringerc
force-pushed
the
dev/pgjdbc-1428-rm-7814-rt-64017
branch
from
de0665d
to
4ccff98
Compare
|
✅ Build pgjdbc 1.0.184 completed (commit 79b9f6d301 by @ringerc) |
|
@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 😄. |
|
@sehrope Ha, that's hilarious. No worries. |
|
@vlsi and @davecramer I think this is merge-ready. May I have your approval to merge? |
|
anyone have any issues with merging this ? |
|
LGTM |
|
PS. It would be great if changelog could be updated as well (== you could include relevant note to CHANGELOG.md) |
Add a new
Map PGConnection.getParameterStatuses()method that tracks thelatest values of all
GUC_REPORTparameters reported by the server inParameterStatusprotocol messages from the server. The map is read-only. Aconvenience
PGConnection.getParameterStatus(String)wrapper is also provided.This provides a PgJDBC equivalent to the
PQparameterStatus(...)libpqAPIfunction.
Extensions may define custom GUCs that are set as
GUC_REPORTwhen theyDefineCustomStringVariable(...)etc. This feature will properly handle suchGUCs, 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_REPORTor theirnames, so it'll work (with possible test case tweaks) on current and future
server versions.
Github issue #1428
All Submissions:
New Feature Submissions:
Changes to Existing Features: