Closed (won't fix)
Project:
Drupal core
Version:
8.9.x-dev
Component:
mysql db driver
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
12 Apr 2014 at 03:13 UTC
Updated:
16 Jul 2020 at 14:05 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
danblack commentedComment #2
damien tournoud commentedNeither
REPLACEnorINSERT ... ON DUPLICATE KEY ...are alternatives forMERGE, as they only consider actual table constraints (primary keys or unique keys), not the keys that you specified.See #715108: Make Merge queries more consistent and robust for the full discussion.
Also, I saw you wondering why we don't have a
FOR UPDATEon theSELECT. This not because we implicitly assume we are running underREAD REPEATABLE, but it is because we don't care which concurrent modification will succeed. See #715108-114: Make Merge queries more consistent and robust for the discussion that lead us there.Comment #3
damien tournoud commentedComment #4
danblack commentedmeans if the keys align and a bunch of other stuff mentioned in the other issues mentioned.
Comment #5
damien tournoud commentedHow do you expect to detect if things align? By introspecting the schema? This very unlikely to lead to performance improvements, but will certainly increase the complexity of the code.
REPLACEwas invented as an atomic operation at one point where MySQL didn't have a transactional storage. It is a thing of the past, not the way of the future.Comment #6
danblack commenteddefinitely agree with code complexity.
http://www.mysqlperformanceblog.com/2013/12/12/one-more-innodb-gap-lock-... indicated it might help.
I'll write a concurrent test next with a varying percentage of insert vs update and see how the two implementations compare.
Comment #7
damien tournoud commentedAt a glance, this is not remotely equivalent to the current code.
Please see #715108-4: Make Merge queries more consistent and robust for a description of all the cases you need to handle. In a nutshell, you will need to introspect the schema (at runtime), to extract the list of constraints for table, and you will only be able to use a
REPLACEor aINSERT ... ON DUPLICATE KEY ...when both of the following are true:(1) There is only one key (because in case of multiple keys, you are going to trigger an error if *any* key is duplicate, not if *all* key are duplicate)
(2) The key is a constraint of the table (primary key or unique key)
Comment #8
damien tournoud commentedI can see that you are starting doing this, but:
This is Drupal's conceptual schema, you cannot use this in the database layer. You will have to introspect the schema from MySQL's really slow
information_schema.Comment #10
danblack commented> At a glance, this is not remotely equivalent to the current code.
I'm sorry you feel anguished at covering something you believe is right and fixed 4 years ago. Can we let the test cases including performance tests to be decider of the value or not of this patch?
Because of the failures outside of MergeTest there are test case and code deficiencies here still.
> (1) There is only one key (because in case of multiple keys, you are going to trigger an error if *any* key is duplicate, not if *all* key are duplicate)
I believe I covered this (and its implementation in keys) but I guess more explicit tests are required.
the other criteria:
(3) The insert fields covers all non-serial columns
Note to self: a keys() column that isn't part of a uniqueKey needs to trigger the Query/Merge implementation as otherwise it isn't a distinguisher of insert vs update fields.
While a variation between drupal's conceptual schema and the real database schema can obviously occur due to manipulation outside of Drupal's install/update why isn't this a fair assumption?
I totally agree that introspection of the information schema isn't appropriate.
Comment #11
mikeytown2 commentedThere is a module that checks for schema discrepancies.
In general I would assume that at most people would have added indexes and not written an hook_schema_alter to update the schema. As long as we assume indexes aren't perfect I think we can use the schema provided by Drupal.
Comment #12
damien tournoud commentedNo, you just cannot use the conceptual schema in the database layer. The conceptual schema depends on at least (1) Drupal being bootstrapped, (2) the module system and (3) the cache system. Any of those can (and do) depend on the database layer.
The only option you have in the database layer is runtime introspection, which is what is done in PostgreSQL.
Comment #13
morgantocker commentedIt's worth pointing out that
REPLACEis always semantically aDELETEfollowed by anINSERT. In the case that a merge doesn't have many rows to actually modify, the REPLACE operation will be more expensive on the MySQL-side.Comment #14
danblack commented> As long as we assume indexes aren't perfect I think we can use the schema provided by Drupal.
It probably a problem only if a unique index is removed and the schema says its still there.
If the schema returns false/empty then it falls back to the Query/Merge implementation.
obviously, but why is that important?
> In the case that a merge doesn't have many rows to actually modify
Hence #1499738: db_merge does not allow multivalue inserts..
> the REPLACE operation will be more expensive on the MySQL-side.
more expensive than what? Expensive in terms of lock, latency or something else? Do you have a test case that proves this? Because that's my next effort, to see what performs better.
Comment #15
mikeytown2 commentedDamien Tournoud is right about this. Using the drupal schema in its current form wouldn't be smart as it has the potential to greatly increase the complexity of the database layer due to interdependencies. The only way around this would be to store the schema in file storage but with this issue #2161591: Change default active config from file storage to DB storage I don't see that happening any time soon.
Is there a way to make this functionality be possible in some contrib code for experimentation purposes in the future?
Comment #16
danblack commentedI tried following the code paths there to see when schema was available because a FALSE return from drupal_get_schema is still workable as there is a fallback. Getting a schema that has a unique key before the database does however is a problem.
#1314214-16: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) seems to indicate also striving for independence (not sure why - there's other DB abstraction layers like Doctrine doing the same thing).
Comment #17
mikeytown2 commented#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities might change things
Comment #18
danblack commentedComment #27
amateescu commentedComment #28
amateescu commentedPer #7, I think this is a won't fix.