Allow table names with forUpdate/forShare (tgriesser/knex#2834) #2835
Conversation
| @@ -67,12 +67,35 @@ assign(QueryCompiler_PG.prototype, { | |||
| return value ? ` returning ${this.formatter.columnize(value)}` : ''; | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
How difficult would it be to also implement this functionality for at least MySQL?
There was a problem hiding this comment.
I don't see what you mean by the code fragment. If you mean the table names for locks, I am not sure if that is possible with MySQL at all, however I am not too familiar with that.
Maybe someone else can comment on that?
There was a problem hiding this comment.
I mean not code fragment per se but the new functionality in general.
MySQL also has SELECT FOR UPDATE functionality:
https://dev.mysql.com/doc/refman/5.7/en/select.html
There was a problem hiding this comment.
MySQL has support for SELECT … FOR UPDATE (which is already supported by Knex) but it does not seem to have support for specifying which tables to lock and always locks rows in all tables involved in the SELECT query.
| for (let i = 0; i < tables.length; i++) { | ||
| let tableName = tables[i]; | ||
|
|
||
| if (tableName && schemaName) tableName = `${schemaName}.${tableName}`; |
There was a problem hiding this comment.
It is typically recommended to wrap then clause in {} always.
There was a problem hiding this comment.
I copied that fragment verbatim from src/query/compiler.js, definition of property tableName (at the end of the file). Do you think both should be adjusted to conform to current code styles?
There was a problem hiding this comment.
There is plenty of sub-optimal existing code :). Personally yeah, I think that would make sense.
| const sql = []; | ||
|
|
||
| for (let i = 0; i < tables.length; i++) { | ||
| let tableName = tables[i]; |
There was a problem hiding this comment.
this probably could be simplified by using ternary operator
There was a problem hiding this comment.
I am curious. How would that work?
There was a problem hiding this comment.
One a second look, it's a bit more complicated than that, but I think that this is still simpler:
let tableName = tables[i];
if (!tableName) {
continue;
}
if (schemaName) {
tableName = `${schemaName}.${tableName}`;
}
sql.push(this.formatter.wrap(tableName));
There was a problem hiding this comment.
I simplified the code as suggested. I did not touch the other code fragment, however, that seems outside the scope of this pull request and should be handled in a separate issue, IMHO.
| @@ -6023,6 +6023,34 @@ describe('QueryBuilder', function() { | |||
| ); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
also needs integration test
There was a problem hiding this comment.
I went ahead and created an integration test that uses two tables, locking only rows from one of them.
There was a problem hiding this comment.
@kibertoad In the future version of knex I'm working on, we're going to move away from requiring as many integration tests outside of basic connection management stuff. It's kind of silly to be testing the database functionality, that's outside of the scope of knex.
There was a problem hiding this comment.
@tgriesser problem of not having integration tests is that no one ever tests that given queries are actually doing what they are supposed to do. There has been many bugs because of that. So integration tests are not testing that DB works correctly, but it makes sure that created query does what it is supposed to do and keep on doing the correct thing in future, when someone decides to change the query.
|
Cool, can you just add a note in the CHANGELOG.md and I'll merge this. |
|
@tgriesser I added a note to CHANGELOG.md. Thanks for merging this so quickly. |
|
I would have preferred of throwing an error instead of silent ignore, if one tries to pass table names with dialects which doesn't support it. |
This implements optional table names to select locks that get applied in
SELECT … FOR UPDATEandSELECT … FOR SHAREqueries for PostgreSQL dialect. See #2834 for details and rationale.