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

INSERT/UPDATE ... RETURNING #971

Closed
cowwoc opened this issue Sep 30, 2014 · 12 comments
Closed

INSERT/UPDATE ... RETURNING #971

cowwoc opened this issue Sep 30, 2014 · 12 comments

Comments

@cowwoc
Copy link
Contributor

cowwoc commented Sep 30, 2014

INSERT: http://www.postgresql.org/docs/9.3/static/sql-insert.html
UPDATE: http://www.postgresql.org/docs/9.3/static/sql-update.html

support returning values that were inserted or updated.

In the case of INSERT, one could insert a new row and use RETURNING to retrieve the value that was inserted into an AUTOINCREMENT column.

In the case of UPDATE, one could UPDATE ... SET ... FROM and use RETURNING to retrieve the new values.

This syntax is specific to PostgreSQL. It's not clear whether it would be possible to add it to QueryDSL in a database-agnostic manner. Many Stackoverflow answers favor the use of RETURNING over CURRVAL() or LASTVAL() because the latter are not thread-safe on some databases and return the wrong values if triggers are used. I mention this because there might not be any good alternatives under PostgreSQL.

@brunomanzo
Copy link

any thoughs on this?

@timowest
Copy link
Member

@brunomanzo Contributions are welcome. I haven't had time to look into this.

@brunomanzo
Copy link

@timowest
I did some research about because I really need to audit batch updates/inserts.
The Hibernate don't support the function, because the getResultList method doesn't allow DML operations.

After some tests I did the following:
1 - Extends HibernateUpdateClause
2 - Override the execute method
3 - Get the HQL using the "serializer.toString()"
4 - Get the Session from the hibernate and use the method doWork/doReturningWork to get a Connection managed by the hibernate transaction

Controller for allowing users to perform JDBC related work using the Connection managed by this Session.

5 - Get the entity that will be updated and find the name of the id field (sadly here not all tables have a "id" field, sometimes its "id_tableName").
6 - Use the org.hibernate.hql.spi.QueryTranslatorFactory to translate the HQL->SQL
7 - Append the returning clause with the id field on the translated SQL
8 - Make a prepared statement with the connection managed by the hibernate
9 - Take the parameters and inject them on the PS
10 - execute que query and get the result set

In my case the id field was enough. I will open a PR later with the code so you can take a look, but what you think about the idea?

@Shredder121
Copy link
Member

INSERT/UPDATE .. RETURNING is only relevant for Querydsl SQL.
For Querydsl JPA we only follow the JPA spec (and only apply minor differences for JPA Providers - for example Hibernate - to work around bugs in them).

@brunomanzo
Copy link

@Shredder121 I see, but for my case the application is built in top of hibernate, so i had to do that work around, it's working fine for me. But makes sense what you said :)

@Shredder121
Copy link
Member

Yes and I'm not saying that workarounds shouldn't be there (it's a risk of the trade).
I'm only making a point that including it in Querydsl JPA is probably not such a good idea.
I figured it's better to let you know upfront, before you start making a commit on top of Querydsl and propose a pull request, to save you (and us) time.

@timowest
Copy link
Member

I agree with @Shredder121, as a Querydsl contribution we would accept it only in the scope of Querydsl SQL.

@mwillema
Copy link

In QueryDSL SQL with PostgreSQL an insert with a call to executeWithKey(...) leads to a org.postgresql.util.PSQLException: ERROR: column "MY_ID" does not exist where MY_ID is the name of the auto incremented primary key.

I guess this is because the only way to get the generated primary key with PostgreSQL is by adding the 'returning MY_ID' at the end of the insert clause.

Do you have a workaround for this? Or a pointer on how to fix it?

@coder-hugo
Copy link
Contributor

TL;DR it's not a bug in querydsl but a misconfiguration of the code generation plugin. If you are using an in-memory database for the code generation, you should ensure that this database doesn't change the case of your column (and table) names. For an H2 database this can be done as follows: jdbc:h2:mem:db;DATABASE_TO_UPPER=FALSE;INIT=RUNSCRIPT FROM <your-schema-sql-file>

We had exactly the same issue with querydsl-sql in combination with PostgresSQL. After several hours of debugging we've found the reason for this behavior. querydsl-sql is doing everything properly to generate a valid SQL statement for returning the generated keys. There was just one thing wrong: the name of the column for our primary key within the statement was ID (upper case) whereas it's id (lower case) in the PostgresSQL schema. After recognizing this difference we looked for the cause why querydsl-sql uses upper case column names instead of lower case ones. Since they are already upper cased within the generated classes and the code for generating the query classes doesn't contain anything that changes the case, there was only one piece left in the chain that have to cause this issue. In our case we use an in-memory database (H2) that provides the information_schema for generating the query classes. By default the H2 upper cases all table and column names but there is a setting that can be passed to the jdbcUrl to disable this: DATABASE_TO_UPPER=FALSE. After changing this and regenerating the query classes everything worked as expected.

@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 3, 2021
@stale stale bot closed this as completed Jun 10, 2021
@vbartacek
Copy link

I've solved this issue by configuring the QueryDSL Configuration class to convert all identifiers to lower-case:

import com.querydsl.sql.Configuration;
import com.querydsl.sql.PostgreSQLTemplates;
import com.querydsl.sql.namemapping.ChangeLetterCaseNameMapping;
...
final Configuration configuration = new Configuration(new PostgreSQLTemplates());
configuration.setDynamicNameMapping(new ChangeLetterCaseNameMapping(ChangeLetterCaseNameMapping.LetterCase.LOWER, Locale.ENGLISH));

@rdicroce
Copy link
Contributor

I just spent most of the day investigating whether it's possible to implement this in a database-agnostic way, and I've decided the answer is no. But in case anyone else wants to take a stab at this, I figured I'd summarize my research.

At first, it doesn't look so bad. Many databases have some variant of this functionality. But, as always, the devil is in the details:

  • There is no standard syntax, so it varies widely between databases.
    • SQLite, Postgres, and Firebird all use RETURNING at the end of the query.
    • Oracle uses RETURNING INTO at the end of the query.
    • SQL Server uses OUTPUT in the middle of the query.
  • The returned data goes to different places.
    • SQLite and Postgres can only return the data directly, as if the query were a SELECT.
    • Oracle can only put the data into some previously defined variable.
    • Firebird and SQL Server can do either of the above.
  • All of them look like they allow returning expressions based on the columns, but Oracle additionally allows top-level aggregates. (e.g. RETURNING SUM(foo) INTO bar)
  • Different databases have different rules for what data is returned by an UPDATE.
    • SQLite, Postgres, and Oracle always return the modified values.
    • Firebird and SQL Server let you pick whether you want the original or modified values by prefixing the column names.
      • Firebird uses the prefixes OLD. and NEW., and the prefix is optional (defaults to new).
      • SQL Server uses the prefixes DELETED. and INSERTED., and the prefix is mandatory. In fact, the prefix is mandatory even for INSERT and DELETE statements, even though there is no ambiguity in those cases.

On top of all that, the JDBC spec creates more headaches if you want to do batched statements. The spec specifically bans executeBatch() from returning any ResultSets. So a batch INSERT with return would have to use batchToBulk = true, and I don't see any generic solution for batch UPDATE/DELETE. For SQL Server, Firebird, and Oracle, there could be a workaround since they can output the data into a variable or temp table. So the batch itself could have no ResultSets and then the actual ResultSet could be read by a separate query. For SQL Server specifically, I've requested the addition of a method in the JDBC driver to allow batch queries to return ResultSets - see microsoft/mssql-jdbc#2156.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants