Support RETURNING clause for MariaDB
#49840
Conversation
fatkodima
force-pushed
the
insert-returning-mariadb
branch
from
e1f1988
to
735262b
Compare
| assert_not_nil record_with_defaults.modified_time | ||
| assert_not_nil record_with_defaults.modified_time_without_precision | ||
| assert_not_nil record_with_defaults.modified_time_function | ||
| if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter, :Mysql2Adapter, :TrilogyAdapter) |
There was a problem hiding this comment.
I'm not used to seeing all of these adapters listed -- is this so that we avoid running the tests against OracleAdapter, etc?
There was a problem hiding this comment.
Yes. I have seen recently (can't find that issue/PR) that someone mentioned they use active record tests as part of their adapter tests.
| if current_adapter?(:PostgreSQLAdapter) && ActiveRecord::Base.connection.supports_identity_columns? | ||
| klass = Class.new(ActiveRecord::Base) do | ||
| self.table_name = "postgresql_identity_table" | ||
| if current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter) |
There was a problem hiding this comment.
I'm finding it difficult to follow this test, since we're asserting different things for different adapters 😅 Maybe we could pull the assertions that are adapter-specific out into separate tests, one per adapter?
There was a problem hiding this comment.
Agreed, extracted as separate test cases.
| def return_value_after_insert?(column) # :nodoc: | ||
| column.auto_populated? | ||
| end |
There was a problem hiding this comment.
Should we make this the default in AbstractAdapter?
There was a problem hiding this comment.
This method is :nodoc: and the same for all adapters, so I think yes. Did that.
|
|
||
| if insert.skip_duplicates? |
There was a problem hiding this comment.
Switching this to INSERT IGNORE 👌
There was a problem hiding this comment.
@adrianna-chang-shopify @fatkodima @nvasilevski Why is this change desirable? I don't see any discussion of why we want to use INSERT IGNORE over a no-op ON DUPLICATE KEY UPDATE. I haven't tested but I think the behaviour may be different.
There was a problem hiding this comment.
INSERT IGNORE generates a warning and ON DUPLICATE KEY UPPDATE does not.
There was a problem hiding this comment.
Ok, so I thought that these are equivalent. Do you mean MariaDB warning?
There was a problem hiding this comment.
Oh, I see from the docs what you mean. Thanks for pinging! I will open a PR with the fix.
There was a problem hiding this comment.
With RETURNING clause INSERT ON DUPLICATE UPDATE no_op always returns all (even not actually inserted) rows in result set.
INSERT IGNORE returns only the actually inserted rows and in this behaviour it coincides with postgresql adapter.
MariaDB [db]> CREATE TABLE tmp_users (
-> id int(11) NOT NULL AUTO_INCREMENT,
-> email varchar(255) NOT NULL,
-> PRIMARY KEY (id),
-> UNIQUE KEY unique_users_email (email)
-> );
Query OK, 0 rows affected (0,010 sec)
MariaDB [db]> insert ignore tmp_users (email) values ('one@example.com'), ('two@examp.com');
Query OK, 2 rows affected (0,002 sec)
Records: 2 Duplicates: 0 Warnings: 0
MariaDB [db]> insert tmp_users (email) values ('one@example.com'), ('two@examp.com') ON DUPLICATE KEY UPDATE id=id returning id, email;
+----+-----------------+
| id | email |
+----+-----------------+
| 1 | one@example.com |
| 2 | two@examp.com |
+----+-----------------+
2 rows in set (0,001 sec)
MariaDB [db]> insert ignore tmp_users (email) values ('one@example.com'), ('two@examp.com') returning id, email;
Empty set, 2 warnings (0,001 sec)After #49890 we won't know which of the rows were inserted.
There was a problem hiding this comment.
@jhawthorn , @fatkodima 👀 ☝🏻
There was a problem hiding this comment.
Can you open a new issue with all of the information describing your problem? Pinging maintainers is not a good way to get a response
There was a problem hiding this comment.
I think this was addressed already: eecd672
Co-authored-by: Nikolay Kondratyev <nkondratyev@yandex.ru>
fatkodima
force-pushed
the
insert-returning-mariadb
branch
from
735262b
to
81eeecd
Compare
This is a rebased #49315 PR with addressed feedback.
I will need these changes for my effort of supporting
uuiddata type for MySQL/MariaDB.I added a test case which tests that dynamic database defaults are populated after the
INSERT.insert_all-like methods also support:returning, but these are already testedrails/activerecord/test/cases/insert_all_test.rb
Line 90 in 820fbd9
Closes #49315.
cc @nvasilevski @adrianna-chang-shopify