Named placeholders #1946

kimjand · 2020-11-05T19:09:13Z

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:

Does your submission pass tests?
Does ./gradlew autostyleCheck checkstyleAll pass ?
Have you added your new test classes to an existing test suite in alphabetical order?

This PR adds new methods for binding parameter values to named placeholders. The new methods are specified in
PGPreparedStatement implemented in PgPreparedStatement. This allows the following usage:

String sql = "select :aa||:aa||:a AS teststr";
PgPreparedStatement ps = con.prepareStatement(sql).unwrap(PgPreparedStatement.class);
ps.setString("a", "1");
ps.setString("aa", "2");

Since we can reuse the parameter value specified for each placeholder, we only need to send the value for :aa to the back-end once. This might even enable the query planner to do some optimizations, but I am uncertain of that.
I also provided a method that returns a list of placeholder names so it is possible to bind the parameter values through the existing index based methods.

In order to make this work I have made changes to Parser.java enabling the capture of names starting with colon. I moved the state concerning placeholders to a new class ParameterContext, in an effort to avoid a few array<->list conversions and centralize the logic concerning information about placeholders, their position in the SQL text and the required back-end parameters.

I think this is a worthwhile addition, since it dispenses with the need for things like Spring NamedParameterJdbcTemplate for those that want to used named placeholders, enabling parameter reuse.

All tests are green, but I would like some input on where more test coverage is needed.

davecramer · 2020-11-05T19:43:09Z

JDBC doesn't support named parameters. I suspect that things like select * from foo where name like ':foo' might fail?

kimjand · 2020-11-05T19:53:00Z

JDBC doesn't support named parameters. I suspect that things like select * from foo where name like ':foo' might fail?

PreparedStatement doesn't, but CallableStatement does. I have no idea why that is so.

I have added your test case to org.postgresql.core.ParserTest.namedPlaceholderSimple. I already had that case covered, but put it in anyway.

kimjand · 2020-11-05T20:33:15Z

I see that org.postgresql.replication.ReplicationConnectionTest fails I don't that is related to this PR?

pgjdbc/src/main/java/org/postgresql/core/NativeQuery.java

pgjdbc/src/main/java/org/postgresql/core/Parser.java

davecramer · 2020-11-06T13:26:58Z

looks good to me. There are some format only changes that I would prefer to see removed

kimjand · 2020-11-06T15:49:48Z

looks good to me. There are some format only changes that I would prefer to see removed

I went through the changes, and have undone those changes. I also undid a few remnants of some exploratory surgery...

kimjand · 2020-11-06T17:57:48Z

Everything but org.postgresql.replication.ReplicationConnectionTest passes now.

davecramer · 2020-11-06T19:15:39Z

Everything but org.postgresql.replication.ReplicationConnectionTest passes now.

Don't worry about that one.

kimjand · 2020-11-07T09:14:05Z

Everything but org.postgresql.replication.ReplicationConnectionTest passes now.

Don't worry about that one.

Ok, I won't then. Anything else that needs taking care of?

davecramer · 2020-11-09T13:38:44Z

I'd like to see others review it as well. @vlsi ?

pgjdbc/src/main/java/org/postgresql/core/ParameterContext.java

kimjand · 2020-11-11T19:05:55Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java

 import java.util.Map;
 import java.util.TimeZone;
 import java.util.UUID;

-class PgPreparedStatement extends PgStatement implements PreparedStatement {
+class PgPreparedStatement extends PgStatement implements PGPreparedStatement {


I realized that having PGPreparedStatement extend PreparedStatement makes life easier as a user.
I don't have to handle both a PGPreparedStatement and PreparedStatement variable to get access to both standard and PG-specific methods.

kimjand · 2020-11-11T19:07:13Z

benchmarks/src/jmh/java/org/postgresql/benchmark/statement/ParseStatement.java

-
+  @Param({"false", "true"})
+  public boolean named;
+  @Param({"0", "1", "10", "20", "100", "1000"})


Using named placeholders doesn't seem to be hurting performance, at least not at 1000 placeholders.

Do you mean "old code" performs exactly the same as the new one?

Could you please show the results?

I would be happy to, as soon as the feature set stabilizes.

davecramer · 2020-11-12T14:04:51Z

@bokken are you good with this now ?

pgjdbc/src/main/java/org/postgresql/core/ParameterContext.java

sehrope · 2020-11-12T15:55:26Z

I like the feature of being able to bind multiple parameters to the same value though I'm not too fond of supporting non-standard features in the core driver. Main worry being we end up with a conflict with core as to what's valid SQL. (Would be nice to be able to use $1, $2, etc, with the regular positional binding...).

If we're going to add this let's make sure to document exactly what's a valid format for picking out parameters. From skimming the PR I think it's any identifier char following a : colon. IIRC, the Spring named param template supports a couple other formats too such as ${nameGoesHere}. There's no standard to follow for this so whatever the choice, it needs to be clear as we'll be supporting this going forward as part of the driver.

Does this always try to parse for named parameters? Wondering if there's a situation where existing code would break as a result of parsing out the new format. Also, if we're always parsing the user SQL for this pattern, any thoughts on supporting $1, $2, ... etc in a similar way (rather than just ? for positional params)?

pgjdbc/src/main/java/org/postgresql/core/ParameterContext.java

When to parse, (and what the aim is) seems unclear in places.

…ocessing.

…sing.

kimjand · 2024-04-13T16:19:20Z

@vlsi test are back in the green. Does anything else need sorting out?
I ran the ParseStatement benchmark on master (d5cfb04 just added more parameters):

Benchmark                          (autoSave)  (bindCount)  (unique)  Mode  Cnt     Score    Error  Units
ParseStatement.bindExecuteFetch  conservative            0     false  avgt   50   603,456 ±  8,855  us/op
ParseStatement.bindExecuteFetch  conservative            1     false  avgt   50   616,473 ±  5,140  us/op
ParseStatement.bindExecuteFetch  conservative           10     false  avgt   50   642,483 ±  5,358  us/op
ParseStatement.bindExecuteFetch  conservative           20     false  avgt   50   682,908 ± 13,192  us/op
ParseStatement.bindExecuteFetch  conservative           50     false  avgt   50   880,347 ± 91,526  us/op
ParseStatement.bindExecuteFetch  conservative          100     false  avgt   50   824,458 ±  7,281  us/op
ParseStatement.bindExecuteFetch  conservative         1000     false  avgt   50  2669,178 ± 26,445  us/op

Compared to this branch (2a36984)

Benchmark                          (autoSave)  (bindCount)  (bindStyle)  (unique)  Mode  Cnt     Score    Error  Units
ParseStatement.bindExecuteFetch  conservative            0         JDBC     false  avgt   50   607,748 ±  8,178  us/op
ParseStatement.bindExecuteFetch  conservative            0        NAMED     false  avgt   50   603,655 ±  5,763  us/op
ParseStatement.bindExecuteFetch  conservative            0       NATIVE     false  avgt   50   602,707 ±  5,067  us/op
ParseStatement.bindExecuteFetch  conservative            1         JDBC     false  avgt   50   617,957 ±  5,111  us/op
ParseStatement.bindExecuteFetch  conservative            1        NAMED     false  avgt   50   643,258 ± 37,307  us/op
ParseStatement.bindExecuteFetch  conservative            1       NATIVE     false  avgt   50   617,908 ±  4,882  us/op
ParseStatement.bindExecuteFetch  conservative           10         JDBC     false  avgt   50   646,722 ±  9,224  us/op
ParseStatement.bindExecuteFetch  conservative           10        NAMED     false  avgt   50   644,390 ±  5,118  us/op
ParseStatement.bindExecuteFetch  conservative           10       NATIVE     false  avgt   50   644,153 ±  4,878  us/op
ParseStatement.bindExecuteFetch  conservative           20         JDBC     false  avgt   50   666,247 ±  5,646  us/op
ParseStatement.bindExecuteFetch  conservative           20        NAMED     false  avgt   50   670,657 ±  7,310  us/op
ParseStatement.bindExecuteFetch  conservative           20       NATIVE     false  avgt   50   671,398 ±  8,415  us/op
ParseStatement.bindExecuteFetch  conservative           50         JDBC     false  avgt   50   731,754 ±  4,731  us/op
ParseStatement.bindExecuteFetch  conservative           50        NAMED     false  avgt   50   734,295 ±  5,417  us/op
ParseStatement.bindExecuteFetch  conservative           50       NATIVE     false  avgt   50   733,938 ±  4,550  us/op
ParseStatement.bindExecuteFetch  conservative          100         JDBC     false  avgt   50   825,748 ±  4,724  us/op
ParseStatement.bindExecuteFetch  conservative          100        NAMED     false  avgt   50   830,318 ±  5,253  us/op
ParseStatement.bindExecuteFetch  conservative          100       NATIVE     false  avgt   50   833,192 ±  7,081  us/op
ParseStatement.bindExecuteFetch  conservative         1000         JDBC     false  avgt   50  2766,777 ± 68,373  us/op
ParseStatement.bindExecuteFetch  conservative         1000        NAMED     false  avgt   50  2819,202 ± 43,939  us/op
ParseStatement.bindExecuteFetch  conservative         1000       NATIVE     false  avgt   50  2742,384 ± 23,574  us/op

That looks ok to me.

kimjand · 2024-05-09T13:26:01Z

@vlsi @davecramer Will this be going anywhere? If not, I might as well close the PR.

vlsi · 2024-05-09T20:30:21Z

@kimjand , thanks for pushing this forward.
It would be easier to review and merge if you could rebase your branch on top of master rather than generate merge commits. Having merge commits makes the history hard to browse.

I've reviewed the code a bit (see refactor commit in https://github.com/vlsi/pgjdbc/tree/pr1946 )

Some notes:

I think PlaceholderStyle parameter does not make sense for ParameterContext, so I suggest removing it.
It is strange that for native bind style you suggest using $1, $5 and so on bind names while you suggest omitting the colon for named. Oracle's and Spring's named parameters use :name for SQL and name for the parameter name, so we should follow. That is why I think we should go for $12 in SQL and 12 for the parameter name. WDYT?
I mean the code like prepareStatement("... id = $12"); ps.setInt("12", 5);
borrowQuery(String sql, PlaceholderStyle placeholderStyle) should rather be replaced with createQueryKey + borrowQueryByKey. It would allow having a simple key (e.g. no wrapper object) for a common case (e.g. default placeholder style).
Nice idea with BaseTest5. I would suggest making AbstractBaseTest, BaseTest4, BaseTest5 abstract though.

vlsi · 2024-05-09T20:33:07Z

pgjdbc/src/main/java/org/postgresql/PGConnection.java

+  PreparedStatement prepareStatement(String sql, PlaceholderStyle placeholderStyle) throws SQLException;
+
+  PreparedStatement prepareStatement(String sql, int autoGeneratedKeys, PlaceholderStyle placeholderStyle) throws SQLException;
+
+  PreparedStatement prepareStatement(String sql, int @Nullable [] columnIndexes, PlaceholderStyle placeholderStyle) throws SQLException;
+
+  PreparedStatement prepareStatement(String sql, String @Nullable[] columnNames, PlaceholderStyle placeholderStyle) throws SQLException;
+
+  PreparedStatement prepareStatement(String sql, int resultSetType, int resultSetConcurrency, PlaceholderStyle placeholderStyle) throws SQLException;
+
+  PreparedStatement prepareStatement(String sql, int resultSetType, int resultSetConcurrency, int resultSetHoldability, PlaceholderStyle placeholderStyle) throws SQLException;


Do we want duplicating all the prepare methods? Is it a shortcut for setPlacehoderStyle(...); prepareStatement(...)?

If that is a shortcut, I would avoid adding new prepareStatement, otherwise we could get quite a few of them quite fast

vlsi · 2024-05-09T20:35:52Z

pgjdbc/src/main/java/org/postgresql/PGProperty.java

+      "Specifies which extra non-standard styles of placeholders are to be recognized by the driver, if any. None will disable parameters in Prepared Statements",
+      false,
+      new String [] {"any", "jdbc", "named", "native", "none"}),


What would be the use case for having placeholderStyle=none?
I think it makes the usage confusing

vlsi · 2024-05-09T20:42:03Z

pgjdbc/src/main/java/org/postgresql/core/ParameterContext.java

+      if (placeholderNameToNativeParameterIndex == null) {
+        placeholderNameToNativeParameterIndex = new HashMap<>(placeholderCount());
+        for (int i = 0; i < placeholderCount(); i++) {
+          Nullness.castNonNull(placeholderNameToNativeParameterIndex)
+              .put(NativeQuery.bindName(i + 1), i);
+        }
+      }


If we exclude $ from the name of the native parameter, then this code could be like Integer.parseInt(placeholderName) + if (result > placeholderCount()) return null.

An alternative option could be a static map within NativeQuery + if (result > placeholderCount()) return null

vlsi · 2024-05-09T20:43:12Z

docs/content/documentation/pgpreparedstatment.md

+PGPreparedStatement ps = 
+  conn.prepareStatement("SELECT col_a FROM mytable WHERE col_a < $1 AND col_b > $1")
+      .unwrap(PGPreparedStatement.class);
+
+ps.setInt("$1", 42);


I would suggest having "1" here for consistency with named parameters

vlsi · 2024-05-09T20:50:59Z

pgjdbc/src/main/java/org/postgresql/core/Parser.java

+            } else {
+              fragmentStart = i;
+            }
+          } else {
+            i = end;
+          }


In most cases, placing the shortest if branch first makes the code easier to follow.

vlsi · 2024-05-09T20:58:02Z

pgjdbc/src/main/java/org/postgresql/core/SqlCommandType.java

+  WITH(false);
+
+  final String lowerName;
+  static final Map<Integer, List<SqlCommandType>> sqlCommandTypeLookup = new HashMap<>();


Suggested change

static final Map<Integer, List<SqlCommandType>> sqlCommandTypeLookup = new HashMap<>();

static final Map<Integer, List<SqlCommandType>> SQL_COMMAND_TYPE_LOOKUP = new HashMap<>();

vlsi · 2024-05-09T21:03:14Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java

@@ -529,6 +530,173 @@ public void clearParameters() throws SQLException {
    preparedParameters.clear();
  }

+  @Override
+  public void setObject(String parameterName, Object x, int targetSqlType) throws SQLException {
+    this.setObject(preparedParameters.getIndex(parameterName), x, targetSqlType);


Suggested change

this.setObject(preparedParameters.getIndex(parameterName), x, targetSqlType);

setObject(preparedParameters.getIndex(parameterName), x, targetSqlType);

vlsi · 2024-05-09T21:06:25Z

pgjdbc/src/test/java/org/postgresql/test/jdbc2/MixedParameterMultiStatementTest.java

+    super.setUp();
+    PGProperty.REWRITE_BATCHED_INSERTS.set(props, true);


setUp opens a connection, so adding REWRITE_BATCHED_INSERTS after setUp is a bit too late

Suggested change

super.setUp();

PGProperty.REWRITE_BATCHED_INSERTS.set(props, true);

PGProperty.REWRITE_BATCHED_INSERTS.set(props, true);

super.setUp();

vlsi · 2024-05-12T10:09:03Z

Here's a reason why PlaceholderStyle.ANY can't be implemented.

WIth JDBC style, we use ? for a placeholder, and the user should use ?? if they want a plain ? in their SQL.
With NATIVE style, the query should be just a native SQL, so it could use ? (e.g. JSON operator).
With NAMED style, the placeholders would be :name, and I believe ? should be allowed as a standalone char.

That results in ambiguity for a SQL like SELECT '{"a":1, "b":2}'::jsonb ? 'b'. The driver won't be able to tell if ? means a JDBC placeholder or if it is just a plain ?.

kimjand · 2024-05-12T10:41:50Z

On 12-05-2024 12:09, Vladimir Sitnikov wrote: Here's a reason why |PlaceholderStyle.ANY| can't be implemented. WIth |JDBC| style, we use |?| for a placeholder, and the user should use |??| if they want a plain |?| in their SQL. With |NATIVE| style, the query should be just a native SQL, so it could use |?| (e.g. JSON operator). With |NAMED| style, the placeholders would be |:name|, and I believe |?| should be allowed as a standalone char. That results in ambiguity for a SQL like |SELECT '{"a":1, "b":2}'::jsonb ? 'b'|. The driver won't be able to tell if |?| means a |JDBC| placeholder or if it is just a plain |?|.

Yes, if we lift the restriction on escaping ? for non-JDBC prepared statements then ANY won't work. Removing the requirement for anything else than then JDBC-case may not be a big problem, since we don't require escaping for non-prepared statements, but I suppose it could be a source of confusion. I don't mind removing ANY. I will go through your other comments, and attempt a rebase. Thanks!

vlsi · 2024-05-12T10:58:20Z

Ideally, we would like to have zero parsing for NATIVE except something like reWriteBatchedInserts. However, I strongly believe with NATIVE mode we should not replace JDBC stuff like {fn ...}.
That means "parameter context" for NATIVE is not very useful as the driver should just accept whatever parameter user supplies.

vlsi · 2024-05-12T10:59:31Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java

+  @Override
+  public boolean hasParameterNames() {
+    return preparedParameters.hasParameterNames();
+  }
+
+  @Override
+  public List<String> getParameterNames() throws SQLException {
+    /* We need to consult Query.getPlaceholderStyle() here, since Query.createParameterList may have
+      returned a reference to ParameterContext.EMPTY_CONTEXT if no parameters were found.
+    */
+    return preparedParameters.getParameterNames(this.preparedQuery.query.getPlaceholderStyle());
+  }


This belongs to ParameterMetaData

kimjand · 2024-05-12T16:48:08Z

On 12-05-2024 12:58, Vladimir Sitnikov wrote: Ideally, we would like to have zero parsing for |NATIVE| except something like |reWriteBatchedInserts|. However, I strongly believe with |NATIVE| mode we should not replace JDBC stuff like |{fn ...}|. That means "parameter context" for |NATIVE| is not very useful as the driver should just accept whatever parameter user supplies.

I think that makes sense, if you go with the NATIVE-setting, you get what is basically a pass-through behavior. So we could just bounce the statement off of the back-end, and use describe to get the metadata. That is, if we accept that we can no longer use SIMPLE-mode, since we don't know anything about the statement at hand... Looking through the code, it seems to me that we need to be able perform a describe when either getMetaData() is called OR the first setXXX-method is being called. Only then do we know what to put into ParameterList. This changes a lot of things around. I think there are some limitations to this approach that needs to be agreed upon before going any further. Also I guess this was not really what I set out to do with this PR. I would be more inclined to remove all the NATIVE-related stuff from here, and then make that a separate project for later. What do you think?

vlsi · 2024-05-12T18:43:36Z

I
would be more inclined to remove all the NATIVE-related stuff from here,
and then make that a separate project for later.

What we learnt so far is:

Connection option is not the way to go
"autodetect" placeholder style can't be implemented
"native" placeholders should preferably make no other parsing/replacing within the SQL
"named" placeholder style should not interfere with ? (e.g. plain ? should not be escaped)

As always, the smaller the change, the easier it is to review. For instance, you've moved keyword parsing logic from Parser to SqlCommandType. It does not seem to be coupled with placeholder feature, and it would be better to review and merge such changes separately.

kimjand · 2024-05-27T16:49:58Z

I currently do not have the time to work on this. Perhaps I will pick it up again at a later date.

davecramer reviewed Nov 6, 2020

View reviewed changes

pgjdbc/src/main/java/org/postgresql/core/NativeQuery.java Outdated Show resolved Hide resolved

davecramer reviewed Nov 6, 2020

View reviewed changes

pgjdbc/src/main/java/org/postgresql/core/Parser.java Outdated Show resolved Hide resolved

bokken reviewed Nov 11, 2020

View reviewed changes