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

RETURNING * blindy appended to prepared batch when clause already exists in statement #488

Closed
apjoseph opened this Issue Jan 2, 2016 · 48 comments

Comments

Projects
None yet
7 participants
@apjoseph

apjoseph commented Jan 2, 2016

Connection.prepareStatement(String, int) results in RETURNING * being appended to SQL batched statements regardless of whether or not a RETURNING clause already exists in the statement. So for example:

INSERT INTO death_star(id,exhaust_port) VALUES ( :id, :exhaust_port) RETURNING id;

becomes

INSERT INTO death_star(id,exhaust_port) VALUES ( ?, ?) RETURNING id RETURNING *;

I have been made aware that it is possible to just use Connection.prepareStatement(String), however this is not possible using many popular frameworks such as JDBI, since those operations are done under the hood.

I know pgjdbc cannot make it its mission to make life easier for every framework out there; yet, I am not sure I understand the need to append RETURNING * when generated keys are requested. Shouldn't the user explicitly specify the keys they want returned in RETURNING clause? Could a global option be added to disable the current behavior and append nothing to the SQL?

See related StackOverflow issue (JDBI)

As a side note, I'm not sure if it is actually worth doing prepared batches with pgjdbc in my case since batching is apparently forced off when user defined types, arrays, or text fields are used. -I have these types in many tables (I considered user defined types one main benefits of Postgres in the first place!). This constraint is seemingly paradoxical because batching functionality is exactly what one would desire when they have many rows of large (think postgis geometry) user defined types and thus need to transmit data more effectively than individualized insert statements with round trips in order to insert/update data as quickly as possible. Sure one could use COPY, but it doesn't return values and isn't supported by any of the ORM frameworks (JDBI, Spring JDBC Template, etc) out there

@vlsi

This comment has been minimized.

Member

vlsi commented Jan 5, 2016

@apjoseph , can you please provide a test case that reproduces returning * issue?

I'm not sure if it is actually worth doing prepared batches with pgjdbc in my case since batching is apparently forced off when user defined types, arrays, or text fields are used.

Can you file another issue for that?

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

This issue affects users of our Application Server (https://github.com/lucee/Lucee) as well. The syntax of the language that our engine processes does not allow us to know in advance whether we need to pass RETURN_GENERATED_KEYS or not, so we always set it on behalf of the user.

That is not an issue with INSERT/UPDATE/DELETE, but when the user issues a SELECT statement, it becomes

SELECT * FROM some_table RETURNING *;

Which of course results in an error as that is an invalid SQL statement.

I would argue that if the user wants to add RETURNING * to their SQL statement then they would just add it, and I do not understand why the driver "does" that on the user's behalf.

Please note that our application server is database-agnostic, and we do not experience this issue with any other drivers.

For the time being we reverted to the pgjdbc driver v 8.3, but we really would like to update the driver to the latest version as soon as possible.

Thank you.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 7, 2016

The reason it is added is to be able to satisfy returning keys.

Dave Cramer

On 7 March 2016 at 12:13, Igal notifications@github.com wrote:

This issue affects users of our Application Server (
https://github.com/lucee/Lucee) as well. The syntax of the language that
our engine processes does not allow us to know in advance whether we need
to pass RETURN_GENERATED_KEYS or not, so we always set it on behalf of the
user.

That is not an issue with INSERT/UPDATE/DELETE, but when the user issues a
SELECT statement, it becomes

SELECT * FROM some_table RETURNING *;

Which of course results in an error as that is an invalid SQL statement.

I would argue that if the user wants to add RETURNING * to their SQL
statement then they would just add it, and I do not understand why the
driver "does" that on the user's behalf.

Please note that our application server is database-agnostic, and we do
not experience this issue with any other drivers.

For the time being we reverted to the pgjdbc driver v 8.3, but we really
would like to update the driver to the latest version as soon as possible.

Thank you.


Reply to this email directly or view it on GitHub
#488 (comment).

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

The syntax of the language that our engine processes does not allow us to know in advance whether we need to pass RETURN_GENERATED_KEYS or not, so we always set it on behalf of the user.

Did you benchmark it?
It seems to be a "fatal flaw" from performance point of view.
I would recommend adding some support of "optional disable/enable generated keys", so your users can choose if they want to enable "generated keys" on a statement-by-statement basis.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

Unfortunately we must maintain compatibility both with the Adobe product and with our previous versions. But let me ask you this:

Are the statements executed in a transaction? If so, can't you just append

; SELECT lastval() AS GENERATED_KEY;

instead of

RETURNING *

? (GENERATED_KEY is the MySQL name, by the way. you can see other implementation names at: http://help.adobe.com/en_US/ColdFusion/9.0/CFMLRef/WSc3ff6d0ea77859461172e0811cbec22c24-7fae.html#d17e83754 )

Our server ships with drivers for all of the major database systems and none of them have that issue except for pgjdbc.

I would recommend adding some support of "optional disable/enable generated keys", so your users can choose if they want to enable "generated keys" on a statement-by-statement basis.

That's a good idea, but we can not change the default.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 7, 2016

lastval only returns the most recent nextval(), which may or may not be the
correct one.

What happens if there is more than one sequence in the table ?

Dave Cramer

On 7 March 2016 at 13:04, Igal notifications@github.com wrote:

Unfortunately we must maintain compatibility both with the Adobe product
and with our previous versions. But let me ask you this:

Are the statements executed in a transaction? If so, can't you just append SELECT
lastval() AS GENERATED_KEY; instead of RETURNING *? (GENERATED_KEY is the
MySQL name, by the way. you can see other implementation names at:
http://help.adobe.com/en_US/ColdFusion/9.0/CFMLRef/WSc3ff6d0ea77859461172e0811cbec22c24-7fae.html#d17e83754
)

Our server ships with drivers for all of the major database systems and
none of them have that issue except for pgjdbc.


Reply to this email directly or view it on GitHub
#488 (comment).

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

Those are very good questions. Any idea how other DBMSs handle this?

I know that SQL Server does not allow for more than one sequence column per table, so they took the easy way out. But what about MySQL, Oracle, etc.?

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

How about adding an option to the connection string which will determine if the driver should add RETURNING * or not? That way we will be able to disable it for our users by default.

If they want to return something then they can always add their own RETURNING id and since they know the tables they're updating they will be able to return only the value they need without the whole record which can be rather large.

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

I guess the main question here is:

  1. If we can safely assume "generated keys" are required for INSERT statements only
  2. If we can return ctid in case user issues executeUpdate(String sql, int autoGeneratedKeys=RETURN_GENERATED_KEYS)

Alternative way would be to figure out PK columns somehow, but it is much harder than simple returning ctid

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

How about adding an option to the connection string which will determine if the driver should add RETURNING * or not? That way we will be able to disable it for our users by default.

How about adding an option to the app server configuration?
Believe me or not, you ask RETURN_GENERATED_KEYS, then you complain.
I feel your pain, yet it is better not to abuse the database.

Parsing SQL at the client side is not the easiest task, so the issue is rather hard to get right.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

How about adding an option to the app server configuration?

I am discussing that with my team as well.

The problem is that even if we add it, the default has to stay as RETURN_GENERATED_KEYS=true due to compatibility issues with the Adobe product and our own previous versions.

Our users have code that has been written over the past 20 years almost (yes, it's that old), so we can not just change it for them and tell them to go update thousands of places.

you ask RETURN_GENERATED_KEYS, then you complain

I understand what you mean, but unfortunately it is not entirely up to us at this point.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

  1. If we can safely assume "generated keys" are required for INSERT statements only

I would think so. A key is "generated" only upon insertion of a new record.

  1. If we can return ctid in case user issues executeUpdate(String sql, int autoGeneratedKeys=RETURN_GENERATED_KEYS)

According to http://www.postgresql.org/docs/current/static/ddl-system-columns.html :
"a row's ctid will change if it is updated or moved by VACUUM FULL. Therefore ctid is useless as a long-term row identifier"

I wonder if the PostgreSQL developers will think that it's a good idea to add something like RETURNING primary_key() which will return the primary key of a table on insertion.

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

like RETURNING primary_key() which will return the primary key of a table on insertion.

What if a primary key is missing?
What if it is a multi-column key?
What if there a multiple unique keys on the same table?
I do not think backend developers would implement returning primary_key()

a row's ctid will change if it is updated or moved by VACUUM FULL. Therefore ctid is useless as a long-term row identifier

That's unfortunate.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

What if a primary key is missing?

If missing, then a single row with a single column with the value NULL is returned.

What if it is a multi-column key?

The returned value should be a record set with the multi-column keys, or a single column key in the case of single-column PK.

What if there a multiple unique keys on the same table?

Unique keys do not constitute a "primary key". If a user wants a primary key returned, he should define a primary key on the table.

@davecramer

This comment has been minimized.

Member

davecramer commented Mar 7, 2016

Dave Cramer

On 7 March 2016 at 14:24, Igal notifications@github.com wrote:

What if a primary key is missing?

If missing, then a single row with a single column with the value NULL is
returned.

What if it is a multi-column key?

The returned value should be a record set with the multi-column keys, or a
single column key in the case of single-column PK.

What if there a multiple unique keys on the same table?

Unique keys do not constitute a "primary key". If a user wants a primary
key returned, he should define a primary key on the table.

Well the spec allows you to ask for any column, which is why we return them
all.


Reply to this email directly or view it on GitHub
#488 (comment).

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

Well the spec allows you to ask for any column, which is why we return them all.

Yes, but if I insert a row with data from a file that was uploaded and it is 10mb in size, do I really want to have 10mb record returned?

All I want is the key which in most cases will be just a few bytes, and sending 10mb back can add unnecessary overhead for CPU, IO, Memory, and Network.

The fact that the spec allows it doesn't mean we should do it.

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

All I want is the key which in most cases will be just a few bytes.

You want to code a reliable application, don't you?
So you use java.sql.Statement#execute(java.lang.String sql, java.lang.String columnNames[]) API, so you always know which columns are returned in which order.

Just for your information: Oracle JDBC driver returns ROWID when you ask RETURN_GENERATED_KEYS. I'm not sure the spec requires to return a "primary key" => you would have hard time trying to use that in database-agnostic way.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

So you use java.sql.Statement#execute(java.lang.String sql, java.lang.String columnNames[]) API, so you always know which columns are returned in which order.

We have the same code for all of the databases, and we need to pass the flag for RETURN_GENERATED_KEYS for compatibility with Adobe ColdFusion.

In any event, this is not our issue. Our issue is that the driver adds "RETURNING" automatically, and that breaks the SQL command, regardless of what or how many columns are returned.

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

Suppose the following: pgjdbc is altered to return ctid by default. Will ColdFusion eagerly consume that?

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

Yes, that's not a problem, as long as it will return it as

; SELECT MAX(ctid) FROM table1;

But if you just append RETURNING ctid then we will still have a problem because it will create an invalid SQL statement like

SELECT * FROM table1 RETURNING ctid;

@vlsi

This comment has been minimized.

Member

vlsi commented Mar 7, 2016

Of course I mean INSERT statements here. Selects are out of the equation -- RETURNING should not be added to them at all.

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

If the driver will only add RETURNING for INSERT then our immediate problem will be resolved.

You should also check, of course, that the user didn't add his own RETURNING clause, as mentioned at the OP above.

At that point I, TBH, do not care if is set to return * or whatever value. I am still unsure about the ctid but RETURNING * is fine by me.

I am writing a proposal to pgsql-hackers for a feature request of primary_key() as written above.

Thank you!

@isapir

This comment has been minimized.

isapir commented Mar 7, 2016

Upon further investigation of ctid, I'm pretty sure that you do not want to return it. Consider the following simple test done in psql:

CREATE TABLE test (name TEXT, id SERIAL PRIMARY KEY);
INSERT INTO test VALUES ('PostgresQL');
INSERT INTO test VALUES ('JDBC');

postgres=# SELECT *, ctid FROM test;
name       | id | ctid
------------+----+-------
PostgresQL |  1 | (0,1)
JDBC       |  2 | (0,2)
(2 rows)

UPDATE test SET name='Lucee' WHERE id=1;

postgres=# SELECT *, ctid FROM test;
name  | id | ctid
-------+----+-------
JDBC  |  2 | (0,2)
Lucee |  1 | (0,3)
(2 rows)
@davecramer

This comment has been minimized.

Member

davecramer commented Mar 7, 2016

We are contemplating putting a parse phase in to determine if we are doing
an insert or not.

Dave Cramer

On 7 March 2016 at 16:05, Igal notifications@github.com wrote:

Upon further investigation of ctid, I'm pretty sure that you do not want
to return it. Consider the following simple test done in psql:

CREATE TABLE test (name TEXT, id SERIAL PRIMARY KEY);
INSERT INTO test VALUES ('PostgresQL');
INSERT INTO test VALUES ('JDBC');

postgres=# SELECT *, ctid FROM test;
name | id | ctid
------------+----+-------
PostgresQL | 1 | (0,1)
JDBC | 2 | (0,2)
(2 rows)

UPDATE test SET name='Lucee' WHERE id=1;

postgres=# SELECT *, ctid FROM test;
name | id | ctid
-------+----+-------
JDBC | 2 | (0,2)
Lucee | 1 | (0,3)
(2 rows)


Reply to this email directly or view it on GitHub
#488 (comment).

@ringerc

This comment has been minimized.

Member

ringerc commented Mar 8, 2016

On 8 March 2016 at 03:15, Vladimir Sitnikov notifications@github.com
wrote:

like RETURNING primary_key() which will return the primary key of a table
on insertion.

What if a primary key is missing?
What if it is a multi-column key?
What if there a multiple unique keys on the same table?
I do not think backend developers would implement returning primary_key()

a row's ctid will change if it is updated or moved by VACUUM FULL.
Therefore ctid is useless as a long-term row identifier

That's unfortunate.

It's sufficient for you to do stuff like grab keys while you haven't
committed yet, so you still have the tuple locked, though.

The real problem there is that you don't get the ctid(s) returned in the
insert response, so you still have to return something with a RETURNING
clause. It also adds unnecessary round trips to get the ctids then fetch
the desired cols, when you can just RETURN(ing) them directly. Performance
would be atrocious.

Most apps that use the 'int generatedKeys' form of
Connection.prepareStatement to request generated keys should use the
String[] form to name the columns they want returned. This is much (MUCH)
more efficient, and in PgJDBC's case allows use of pipelined batching that
isn't possible with the boolean form. Really, the int flag form should be
considered legacy API and I wish the JDBC spec would mark it so.

See the -hackers thread on this topic for more discussion on the RETURNING
PRIMARY KEY issue.

Personally I wish we had a protocol-level alternative to RETURNING that let
us specify a wanted-column-list, so we didn't have to hack around with
query text. That's not going to happen in the near-to-middle-future though,
so for now we're kind of stuck with RETURNING.

I guess we could wrap the whole query...

SELECT (
$ORIGINAL_QUERY
) AS pgjdbc_wrapper
RETURNING *;

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

@ringerc

This comment has been minimized.

Member

ringerc commented Mar 8, 2016

On 8 March 2016 at 22:31, Craig Ringer craig@2ndquadrant.com wrote:

I guess we could wrap the whole query...

SELECT (
$ORIGINAL_QUERY
) AS pgjdbc_wrapper
RETURNING *;

Er, ignore that. 11pm-itis. Pg doesn't support DML nesting like that, and a
wCTE is a giant hammer to use for the job, as well as harder than the
original problem.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

@polobo

This comment has been minimized.

polobo commented Apr 4, 2016

I think this has been recognized but not explicitly acknowledged at this point.

The first thing we need to do is conform to the published API for this (and related) method.

This parameter is ignored if the SQL statement is not an INSERT statement, or an SQL statement able to return auto-generated keys (the list of such statements is vendor-specific).

https://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#prepareStatement(java.lang.String,%20int)

The fact that the OP gives an example using INSERT slightly misleads the uninitiated since while the problem occurs there it is much more prominent if one issues a SELECT statement in this mode. SELECT is unable to return auto-generated keys and so the call should, in effect, be re-directed to prepareStatement(String) on behalf of the user.

Solving how to actually performantly handle the non-ignored cases (i.e., INSERT) is a separate patch/pull request.

Furthermore, I am unsure how to consider the original query. At some point telling the user that specifying their own RETURNING clause and asking for auto-generated keys is not supported - either setup your INSERT to return the keys yourself, and whatever other columns you desire, or do not specify RETURNING and let the driver detect and return the auto-generated keys. While possibly being overly nuanced it can be argued that "INSERT" and "INSERT-RETURNING" are distinct and that the later one ends up bypassing the autoGeneratedKeys argument as it is not an "INSERT" statement. Since "RETURNING" is not part of the SQL standard I don't feel particularly remorseful in making this distinction.

David J.

@davecramer

This comment has been minimized.

Member

davecramer commented Apr 4, 2016

There is a pending PR #491 which
includes a rudimentary parser, once we get this in we can work on this.

Dave Cramer

On 4 April 2016 at 01:49, David Johnston notifications@github.com wrote:

I think this has been recognized but not explicitly acknowledged at this
point.

The first thing we need to do is conform to the published API for this
(and related) method.

This parameter is ignored if the SQL statement is not an INSERT statement,
or an SQL statement able to return auto-generated keys (the list of such
statements is vendor-specific).

https://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#prepareStatement(java.lang.String,%20int)

The fact that the OP gives an example using INSERT slightly misleads the
uninitiated since while the problem occurs there it is much more prominent
if one issues a SELECT statement in this mode. SELECT is unable to return
auto-generated keys and so the call should, in effect, be re-directed to
prepareStatement(String) on behalf of the user.

Solving how to actually performantly handle the non-ignored cases (i.e.,
INSERT) is a separate patch/pull request.

Furthermore, I am unsure how to consider the original query. At some point
telling the user that specifying their own RETURNING clause and asking for
auto-generated keys is not supported - either setup your INSERT to return
the keys yourself, and whatever other columns you desire, or do not specify
RETURNING and let the driver detect and return the auto-generated keys.
While possibly being overly nuanced it can be argued that "INSERT" and
"INSERT-RETURNING" are distinct and that the later one ends up bypassing
the autoGeneratedKeys argument as it is not an "INSERT" statement. Since
"RETURNING" is not part of the SQL standard I don't feel particularly
remorseful in making this distinction.

David J.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#488 (comment)

@polobo

This comment has been minimized.

polobo commented Apr 4, 2016

On Mon, Apr 4, 2016 at 3:35 AM, Dave Cramer notifications@github.com
wrote:

There is a pending PR #491 which
includes a rudimentary parser, once we get this in we can work on this.

​I'd humbly suggest someone take the handful of lines in that PR that
pertain to identifying INSERT/RETURNING/VALUES (rudimentary indeed...),
pull them out, and commit them. Then #491 and a PR to fix our API
non-conformance can both be built off of that code - the later should be
considerably easier.

In terms of useful and testable functionality it seems like it deserves its
own commit regardless and not to be buried in something larger.

​The existing logic in #491​ seems inadequate...we should be simply
attaching attributes to the query itself and then the "rewriter" or other
modules, e.g. "auto-generated key returner", inspect those attributes and
alter their behavior accordingly.

David J.

@isapir

This comment has been minimized.

isapir commented Apr 4, 2016

full SQL parsing is required to identify ? bind placeholders and rewrite them into PostgreSQL compatible ones like $1.

Is that already implemented?

@ringerc

This comment has been minimized.

Member

ringerc commented Apr 26, 2016

On 4 April 2016 at 13:49, David Johnston notifications@github.com wrote:

This parameter is ignored if the SQL statement is not an INSERT statement,
or an SQL statement able to return auto-generated keys (the list of such
statements is vendor-specific).

https://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#prepareStatement(java.lang.String,%20int)

The fact that the OP gives an example using INSERT slightly misleads the
uninitiated since while the problem occurs there it is much more prominent
if one issues a SELECT statement in this mode. SELECT is unable to return
auto-generated keys and so the call should, in effect, be re-directed to
prepareStatement(String) on behalf of the user.

Yep, and it's also known to be an issue with CTE terms.

Really, this is all made more messy by the fact that we don't have
protocol-level RETURNING lists. The driver should be able to specify a
RETURNING list at the protocol level not hack around with the query text.
It's particularly ugly where the query supplied by the user already has a
RETURNING clause. But we're stuck with that.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

@davecramer

This comment has been minimized.

Member

davecramer commented Apr 26, 2016

Dave Cramer

On 4 April 2016 at 16:54, Igal notifications@github.com wrote:

full SQL parsing is required to identify ? bind placeholders and rewrite
them into PostgreSQL compatible ones like $1.

Is that already implemented?

Yes we do parse to find ? and a few other things.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#488 (comment)

@Ratcreamsoup

This comment has been minimized.

Ratcreamsoup commented Jul 25, 2016

I have just gotten used to patching this functionality out of the JDBC driver, which is a messy hack indeed. But I am stuck with a database abstraction where I cannot prevent this functionality breaking all of my SELECT statements and statements with explicitly set RETURNING clauses. Unfortunately the repackaging of drivers as OSGi packages in Lucee now makes the prodedure for using a custom build driver somewhat much more complicated than before, but that's not actually the issue here and I guess I'll find a way.

I fully acknowledge that the specs don't actually prescribe any specific implementation of this feature, so I assume just blindly tacking on a "RETURNING *" may serve the purpose on paper most of the time. But it does mess with statements and even if it doesn't actually always break the SQL as is the case with SELECTs in ColdFusion/Railo/Lucee, it may bite people with the unexpected effect that all of the result tuple is being returned, inducing unnecessary overhead. This may go unnoticed in many cases, but in some it will introduce performance degradation or maybe even memory issues that may prove to be very hard to pinpoint.

I assume that most developers would find it somewhat unexpected that the statement they send to the server are being altered by the driver in a potentially breaking way. This affects any scenario with dynamically generated queries, in particular any kind of database abstraction layer or other kind of database access encapsulation, both of which I would guess are far from being uncommon use cases.

Dealing with this issue by parsing the statements and then deciding if the RETURNING * clause may or may not be appended seems to me somewhat fragile; taking into account the potential for very complex statements in PostgreSQL, involving CTEs, user defined functions and even rules, I find the benefits of this feature in the current situation are far outweighed by the risk of breaking statements due to the appended RETURNING-clause. Maybe I am wrong and there is a surefire method of parsing the statement for deciding if it's safe to add the RETURNING-clause or not.

But as long as there is no protocol level implementation that would in itself be safe against breaking statements, I do consider this appending of the RETURNING-clause by the JDBC driver as inherently broken, as long as there is no other safeguard against breaking SQL statements.

Kind regards

Markus Wollny

@vlsi vlsi self-assigned this Aug 4, 2016

@vlsi vlsi added this to the 9.4.1210 milestone Aug 4, 2016

@vlsi

This comment has been minimized.

Member

vlsi commented Aug 5, 2016

Any ideas what statement.executeUpdate("insert into tab(x) values(42) returning x"); statement.getGeneratedKeys() should do?

Should it fail with "A result was returned when none was expected"?
Should it just succeed?

@vlsi

This comment has been minimized.

Member

vlsi commented Aug 5, 2016

And a question regarding statement.executeUpdate("insert into tab(x) values(42) returning x", Statement.NO_GENERATED_KEYS); should it fail with "A result was returned when none was expected"?

@isapir

This comment has been minimized.

isapir commented Aug 5, 2016

And a question regarding statement.executeUpdate("insert into tab(x) values(42) returning x", Statement.NO_GENERATED_KEYS); should it fail with "A result was returned when none was expected"?

Definitely not fail. Query should take precedence over configuration. Many times the configuration is set in one place, and the developer has little or no control over it, but has full control over the Query code.

@polobo

This comment has been minimized.

polobo commented Aug 5, 2016

On Fri, Aug 5, 2016 at 2:19 PM, Igal notifications@github.com wrote:

And a question regarding statement.executeUpdate("insert into tab(x)
values(42) returning x", Statement.NO_GENERATED_KEYS); should it fail with
"A result was returned when none was expected"?

Definitely not fail. Query should take precedence over configuration. Many
times the configuration is set in one place, and the developer has little
or no control over it, but has full control over the Query code.

https://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#getGeneratedKeys()

Skimming the above link:

NO_GENERATED_KEY: The constant indicating that generated keys should not be
made available for retrieval.

It says "should", not "must". I'd say we'd be within our rights to make
the ResultSet defined by the returning clause available via the
"getGeneratedKeys()" method.

David J.

@vlsi

This comment has been minimized.

Member

vlsi commented Aug 5, 2016

Definitely not fail. Query should take precedence over configuration. Many times the configuration is set in one place, and the developer has little or no control over it, but has full control over the Query code.

@TwentyOneSolutions why would developer write a query with ...RETURNING... inside the query, execute it with NO_GENERATED_KEYS and expect it to work fine?

Just in case: current pgjdbc version fails with "A result was returned when none was expected" in such cases.

@isapir

This comment has been minimized.

isapir commented Aug 5, 2016

@vlsi The developer who writes the query does not always have full control over the setup or configuration of the environment, which ultimately builds the SQL query and submits it.

You are right that if you write your own Java code then you have full control, but in many tools and application servers the developer can write the query, but there is another environment which builds the Java code and submits it.

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 5, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 5, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)
@vlsi

This comment has been minimized.

Member

vlsi commented Aug 5, 2016

Feel free to review #618

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 6, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 6, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 7, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)
@vlsi

This comment has been minimized.

Member

vlsi commented Aug 8, 2016

Does anybody know of a "open-source application" that can be used as a regression suite?
I mean "add yet another travis job to check that newer pgjdbc works fine for existing applications".

For instance: hibernate regression tests or even full-blown applications.

I think #618 is ready to merge (all tests pass, including "run tests with simpleProtocolOnly=true"), however it is a non-trivial change, so I would love to hear from real-life applications.

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 9, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 9, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 10, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 10, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 10, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

vlsi added a commit to vlsi/pgjdbc that referenced this issue Aug 13, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

@vlsi vlsi closed this in #618 Aug 13, 2016

@isapir

This comment has been minimized.

isapir commented Sep 16, 2016

@vlsi Thanks for patching that! It seems to have solved the issue that we were facing with the Lucee project.

zemian pushed a commit to zemian/pgjdbc that referenced this issue Oct 6, 2016

fix: support cases when user-provided queries have 'returning'
This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes pgjdbc#488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment