Problem/Motivation
#1497374: Switch from Field-based storage to Entity-based storage needs a db_copy_table
method: when we are splitting the field data tables per entity type in the D7->D8 upgrade then there is nothing to create the D8 tables from except the existing tables and so we need to copy them.
Proposed resolution
Introduce a db_copy_table function, and a copyTable on Schema.
Remaining tasks
Implementation is per DB:
In MySQL this is CREATE TABLE dst LIKE src
In SQLite we could read the definition of src by SELECT sql FROM sqlite_master WHERE tbl_name='src'
and then massage it. We need to deal with CREATE TABLE and CREATE INDEX separately but it is definitely doable. For CREATE TABLE, just change the tablename, for CREATE INDEX search-replace the indexname if it didn't change, prefix with dst_, and search-replace the tablename.
In PostgreSQL we can do CREATE TABLE dst (LIKE src INCLUDING ALL)
MSSQL is a huge problem but I talked to Damien and he said go ahead and he will deal with it. (Note that INSERT ... SELECT doesn't copy indexes, contraints and the like.)
Oracle has an SQLite-like SELECT dbms_metadata.get_ddl( 'TABLE', 'src' ) FROM DUAL;
so I am less worried than MSSQL.
User interface changes
None.
API changes
We are adding a new function / method. Even backportable if we want.
Related Issues
#1497374: Switch from Field-based storage to Entity-based storage
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff.txt | 1.87 KB | chx |
#45 | 2056133_45.patch | 8.82 KB | chx |
#42 | db-2056133-42.patch | 9.56 KB | dawehner |
#42 | interdiff.txt | 4.7 KB | dawehner |
#40 | 2056133_40.patch | 8.75 KB | plach |
Comments
Comment #0.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #1
chx CreditAttribution: chx commentedHere we go. This has the same amount of tests as renameTable, namely none -- and you can't really check for existence of index, constraints and such in a cross-database way anyways, so I somewhat fail to see what to test with this. The field upgrade will test it amply... I mention renameTable cos I have heavily ahem borrowed from renameTable when writing this. I particularly like the sqlite implementation... assigning to Damien for review cos he is both pgsql and sqlite maintainer.
Comment #4
chx CreditAttribution: chx commentedEh, I forgot to search-replace @source / @destination.
Comment #5
dawehnerSo this code is the same for all three implementations. What about moving this to the schema base class?
Comment #6
plachThat would force us the define a concrete base method which actually would do nothing but throwing exceptions because we have no generic implementation. Instead we need to force drivers to provide their own implementation, so we need an abstract method in the base class. Added that and some missing PHP docs.
Comment #7
yched CreditAttribution: yched commentedNitpick: "Copies the structure of a table" ?
Comment #8
dawehnerWorking on a test
Comment #9
dawehnerThere we go.
Comment #10
dawehnerups, this was broken.
Comment #11
plach@dawehner:
A word about why #6 has been disregarded would be nice :)
Comment #15
Crell CreditAttribution: Crell commentedThis is nominally an API change, but just an addition. It doesn't break any existing code. Given that it's needed for the upgrade path I think it is fine to include. I think we talked about doing this a long time ago but never really got around to it. :-)
The code looks reasonable to me, although I cannot speak for the SQLite or Postgres implementations. Comments:
1) We SHOULD use {@inheritdoc}. New code should be using that. We can fix the rest of the class later, certainly, but new code follows current standards.
2)
These are just unrelated whitespace changes, it looks like?
Comment #16
chx CreditAttribution: chx commented#6 was not disregard at all, I have reviewed the interdiff in that comment and all of that is there -- however, I have removed the inheritdoc. There's none of that in Schema.php and we will not add now to a single method introducing an inconsistency. I know there's a coding standard but DBTNG doxygen is a broken mess anyways (have you seen all the @return statements?) and I would rather keep it a consistent broken mess.
Comment #17
chx CreditAttribution: chx commentedCrosspost (note that Crell was not clairvoyant: he reviewed a patch I posted hastily which was a broken merge so I deleted it). And I still disagree with the @inheritdoc : it'll be much easier to script that if there is none of the functions have @inheritdoc in Schema.
Yeah @ unrelated whitespace change, doesn't matter IMO.
Comment #18
plachI don't care about the
{@inheritdoc}
at all :)I was referring to the fact that we are introducing a base implementation that does nothing: this theoretically will allow broken implementations to be around, while an abstract method would force them to provide an implementation or at least knowingly ignore this method. Not sure the tradeoff is worth, honestly.
Comment #19
chx CreditAttribution: chx commentedCrell points out we will likely farm it out as novice tasks and not manually fix so ok, here's {@inheritdoc} back and here's abstract as well.
plach I would've *really* appreciated if instead of vague "A word about why #6 has been disregarded would be nice" you would've told straight what your problem was -- that you miss an abstract on the newly added copyTable, that's all, that's a trivial fix. I have tried to merge #6 in and made a gigantic mess (which I since deleted) because I presumed based on your comment that a crosspost happened. Anyways, here's the patch with inheritdoc and abstract.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedSadly, this is not going to be enough, because indexes and constraints are globally-named on PostgreSQL. The server is likely going to rename those during the copy process, and it will not match our
table_name + constraint name
convention anymore. PostgreSQL support is critically broken due to several other issues, so let's not hold off this patch with PostgreSQL... but please add a TODO here.And of course PHP doesn't allow bodies to abstract methods. We are not in Python here :) Let's just duplicate the sanity checks, that's what we do everywhere else anyway.
Comment #22
plachI'm really sorry but in #6 almost the whole comment is about the abstract stuff so I didn't think I could cause confusion :)
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedClarifying on #21: let's add the TODO and fix the abstract method, and this is RTBC on my end.
Comment #24
chx CreditAttribution: chx commentedAdded todo, fixed abstract method to not have body, left in inheritdoc -- this answers the concerns of both Crell and Damien, so RTBC. I have attached an interdiff against plach's #6 -- there are no code changes since, only tests were added thanks to dawehner, so I believe both plach and dawehner will fine too. Of course there's an interdiff against #19 too.
Comment #25
plach+1 to RTBC. There's only the minor stuff below but I guess it can be fixed on commit.
I think this should be:
Comment #26
chx CreditAttribution: chx commentedMoving during makes it longer than 80 chars. "table_name + constraint" despite the space I wanted to keep on one line. Yes, if the core committer feels the urge, I will let them add two spaces times two, it's not worth rolling a new one...
Comment #27
Dries CreditAttribution: Dries commentedThis looks good to me.
Instead of just a @todo, do we also want to throw a "not implemented" exception when the method is called? That seems a bit more helpful for developers that attempt to use PostgreSQL. Failing silently is never a good option, even in temporary code.
Comment #28
chx CreditAttribution: chx commentedSure.
Comment #29
dawehnerHere are some tests for the exceptions.
Comment #30
dawehnerRegarding the docs, I opened a different issue: #2057809: Correct the @param and @return statements on dbtng
Comment #31
plachTrailing extra V? ;)
Comment #32
chx CreditAttribution: chx commentedComment #33
dawehnerGreat. I consider this as RTBC now.
Comment #34
plachAnother bogus character I missed before, sorry.
Comment #35
alexpottIf I see a function called db_copy_table I would expect it to copy the schema and data so let's rename it to db_copy_table_schema as this is what it is doing. I also fixed up the function docs to match our standards by adding the return value and added a helpful @see
I think the copyTable() method on Drupal\Core\Database\Schema is okay because the class is directly concerned with schema.
Comment #36
chx CreditAttribution: chx commentedSure.
Comment #37
Crell CreditAttribution: Crell commentedThumbs up from me as well.
Comment #38
Damien Tournoud CreditAttribution: Damien Tournoud commentedThumbs up from me too.
Comment #39
catchThis looks handy. Extremely minor nits;
Should be 'already exists', not 'does already exists'. Also db_copy_table_schema() rather than db_copy_table_schema.
On postgres not working right, we still have #1013034: PostgreSQL constraints do not get renamed by db_rename_table() open so yeah shouldn't hold this up.
Comment #40
plachFixed the patch directly, no interdiff, sorry :)
Comment #41
catchSorry I missed this the first time.
The exception message shouldn't use t() - at least format_string() (or whatever the new version of that is). No need to add a dependency on that to what's a very low-level method.
Also no need for double quotes here.
Comment #42
dawehnerGood catch! I have seen that in other places in the schema classes but at least don't add it here.
Comment #43
chx CreditAttribution: chx commentedthanks, my bad!
Comment #44
webchickDespite what alexpott said (which I read after the fact), I didn't understand why the procedural function is called db_copy_table_schema(), but the method it's calling is only called "copyTable", despite the fact that it in fact seems to be only copying the table schema, and not the entire table. Shouldn't these match?
That would also make this PHPDoc summary much less awkward; it would instead just be "Copies the schema of a table."
I get that $schema->copyTableSchema() might be redundant, though, I guess.
If we can't do any of this anyway, why bother with all these shenanigans with checking if the tables exist? Why not just throw the exception at the top of the method (really, as the only thing in the method) and be done?
Also, let's get that URL pointing to a follow-up issue specifically for adding this to pgsql, rather than burying the reason for it in a 50+ reply issue. It can include the code below that (as well as an explanation as to why this isn't implemented) in the issue summary.
This also apparently throws an exception when not implemented, so we should document that here, too. Should this be an explicit FeatureNotImplemented exception (or whatever), rather than a generic one?
---
Other than that, I couldn't find much to complain about here; tests are great, docs look good, etc. And I think catch/alex's feedback has been taken into consideration so this is probably good to go after a few quick tweaks.
Comment #45
chx CreditAttribution: chx commented> Despite what alexpott said (which I read after the fact), I didn't understand why the procedural function is called db_copy_table_schema(), but the method it's calling is only called "copyTable", despite the fact that it in fact seems to be only copying the table schema, and not the entire table. Shouldn't these match?
Discussed with msonnabaum, no, schema is implied.
> That would also make this PHPDoc summary much less awkward; it would instead just be "Copies the schema of a table."
Done
#2061879: Remove Schema::copyTable filed, code moved there.
> This also apparently throws an exception when not implemented, so we should document that here, too. Should this be an explicit FeatureNotImplemented exception (or whatever), rather than a generic one?
This pgsql thing is very temporary, a generic will do. Let's not overdo it.
Comment #46
webchickCool, that seems fine to me as long as bot comes back happy.
Comment #47
dawehnerI guess noone will call this method without catching all kind of exceptions ... I can't imagine to have a different code path for non existing source then for already existing destination anyway ... so we have a follow up.
Comment #49
chx CreditAttribution: chx commented#45: 2056133_45.patch queued for re-testing.
Comment #50
yched CreditAttribution: yched commentedback to RTBC
Comment #51
catchCommitted/pushed to 8.x, thanks!
Could use a change notice.
Comment #52
chx CreditAttribution: chx commentedhttps://drupal.org/node/2062137 written
Comment #53
yched CreditAttribution: yched commentedremove tag
Comment #53.0
yched CreditAttribution: yched commentedUpdated issue summary.