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.

#1497374: Switch from Field-based storage to Entity-based storage

Files: 
CommentFileSizeAuthor
#45 interdiff.txt1.87 KBchx
#45 2056133_45.patch8.82 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,839 pass(es).
[ View ]
#42 db-2056133-42.patch9.56 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,086 pass(es).
[ View ]
#42 interdiff.txt4.7 KBdawehner
#40 2056133_40.patch8.75 KBplach
PASSED: [[SimpleTest]]: [MySQL] 57,635 pass(es).
[ View ]
#35 2056133_35.patch8.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,626 pass(es).
[ View ]
#35 34-35-interdiff.txt2.62 KBalexpott
#34 2056133_34.interdiff.do_not_test.patch434 bytesplach
#34 2056133_34.patch8.56 KBplach
PASSED: [[SimpleTest]]: [MySQL] 57,962 pass(es).
[ View ]
#32 interdiff.txt628 byteschx
#32 2056133_32.patch8.56 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,862 pass(es).
[ View ]
#29 db-2056133-29.patch9.03 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es).
[ View ]
#29 interdiff.txt2.51 KBdawehner
#28 2056133_28.patch7.66 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]
#28 interdiff.txt1.63 KBchx
#24 interdiff_6_24.txt4.27 KBchx
#24 interdiff.txt4.08 KBchx
#24 2056133_24.patch7.53 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es).
[ View ]
#19 2056133_19.patch6.56 KBchx
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Database/Schema.php.
[ View ]
#19 interdiff.txt1.47 KBchx
#16 2056133_15.patch6.45 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,626 pass(es).
[ View ]
#16 interdiff.txt1.47 KBchx
#10 db-2056133-10.patch6.55 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,798 pass(es).
[ View ]
#10 interdiff.txt6.75 KBdawehner
#9 db-2056133-9.patch6.43 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,370 pass(es), 24 fail(s), and 6 exception(s).
[ View ]
#9 interdiff.txt6.63 KBdawehner
#6 2056133_6.patch4.98 KBplach
PASSED: [[SimpleTest]]: [MySQL] 57,797 pass(es).
[ View ]
#6 2056133_6interdiff.do_not_test.patch3.42 KBplach
#4 2056133_4.patch4.1 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,779 pass(es).
[ View ]
#4 interdiff.txt3.77 KBchx
#1 2056133_1.patch4.09 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

Assigned:chx» Damien Tournoud
Status:Active» Needs review
StatusFileSize
new4.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,471 pass(es).
[ View ]

Here 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.

StatusFileSize
new3.77 KB
new4.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,779 pass(es).
[ View ]

Eh, I forgot to search-replace @source / @destination.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.phpundefined
@@ -327,6 +327,17 @@ public function dropTable($table) {
+    if (!$this->tableExists($source)) {
+      throw new SchemaObjectDoesNotExistException(t("Cannot copy @source to @destination: table @source doesn't exist.", array('@source' => $source, '@destination' => $destination)));
+    }
+    if ($this->tableExists($destination)) {
+      throw new SchemaObjectExistsException(t("Cannot copy @source to @destination: table @destination already exists.", array('@source' => $source, '@destination' => $destination)));
+    }

So this code is the same for all three implementations. What about moving this to the schema base class?

StatusFileSize
new3.42 KB
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,797 pass(es).
[ View ]

So this code is the same for all three implementations. What about moving this to the schema base class?

That 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.

+++ b/core/lib/Drupal/Core/Database/Schema.phpundefined
@@ -408,6 +408,19 @@ public function fieldExists($table, $column) {
   /**
+   * Copies a table.
(...)
+   */
+  abstract public function copyTable($source, $destination);

Nitpick: "Copies the structure of a table" ?

Working on a test

StatusFileSize
new6.63 KB
new6.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,370 pass(es), 24 fail(s), and 6 exception(s).
[ View ]

There we go.

StatusFileSize
new6.75 KB
new6.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,798 pass(es).
[ View ]

ups, this was broken.

@dawehner:

A word about why #6 has been disregarded would be nice :)

Issue tags:+API addition

This 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)

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SchemaTest.php
@@ -154,9 +167,9 @@ function testSchema() {
     try {
-       db_insert($table)
-         ->fields(array('id' => mt_rand(10, 20)))
-         ->execute();
+      db_insert($table)
+        ->fields(array('id' => mt_rand(10, 20)))
+        ->execute();
       return TRUE;
     }
     catch (\Exception $e) {
@@ -227,8 +240,8 @@ function testUnsignedColumns() {
@@ -227,8 +240,8 @@ function testUnsignedColumns() {
   function tryUnsignedInsert($table_name, $column_name) {
     try {
       db_insert($table_name)
-         ->fields(array($column_name => -1))
-         ->execute();
+        ->fields(array($column_name => -1))
+        ->execute();
       return TRUE;
     }

These are just unrelated whitespace changes, it looks like?

Issue tags:-API addition
StatusFileSize
new1.47 KB
new6.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,626 pass(es).
[ View ]

#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.

Issue tags:+API addition

Crosspost (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.

I 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.

StatusFileSize
new1.47 KB
new6.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Database/Schema.php.
[ View ]

Crell 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.

Status:Needs review» Needs work

The last submitted patch, 2056133_19.patch, failed testing.

<?php
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -348,6 +348,15 @@ function renameTable($table, $new_name) {
    
$this->connection->query('ALTER TABLE {' . $table . '} RENAME TO ' . $prefixInfo['table']);
   }
/**
+   * {@inheritdoc}
+   */
+  public function copyTable($source, $destination) {
+   
parent::copyTable($source, $destination);
+   
$info = $this->getPrefixInfo($destination);
+    return
$this->connection->query('CREATE TABLE `' . $info['table'] . '` (LIKE {' . $source . '} INCLUDING ALL)');
+  }
?>

Sadly, 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.

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

I'm really sorry but in #6 almost the whole comment is about the abstract stuff so I didn't think I could cause confusion :)

Clarifying on #21: let's add the TODO and fix the abstract method, and this is RTBC on my end.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es).
[ View ]
new4.08 KB
new4.27 KB

Added 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.

+1 to RTBC. There's only the minor stuff below but I guess it can be fixed on commit.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -348,6 +348,23 @@ function renameTable($table, $new_name) {
+    // @TODO The server is likely going to rename indexes and constraints
+    // during the copy process, and it will not match our
+    // table_name + constraint name convention anymore. Fix this.

I think this should be:

    // @todo The server is likely going to rename indexes and constraints during
    //   the copy process, and it will not match our table_name + constraint
    //   name convention anymore. Fix this.

Moving 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...

This 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.

StatusFileSize
new1.63 KB
new7.66 KB
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]

Sure.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.51 KB
new9.03 KB
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es).
[ View ]

Here are some tests for the exceptions.

Regarding the docs, I opened a different issue: #2057809: Correct the @param and @return statements on dbtng

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -446,7 +446,7 @@ public function fieldExists($table, $column) {
+   * @throws Drupal\Core\Database\SchemaObjectDoesNotExistExceptionV

Trailing extra V? ;)

StatusFileSize
new8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,862 pass(es).
[ View ]
new628 bytes

Status:Needs review» Reviewed & tested by the community

Great. I consider this as RTBC now.

StatusFileSize
new8.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,962 pass(es).
[ View ]
new434 bytes

Another bogus character I missed before, sorry.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.62 KB
new8.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,626 pass(es).
[ View ]

If 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.

Status:Needs review» Reviewed & tested by the community

Sure.

Thumbs up from me as well.

Assigned:Damien Tournoud» Unassigned

Thumbs up from me too.

Status:Reviewed & tested by the community» Needs work

This looks handy. Extremely minor nits;

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SchemaTest.phpundefined
@@ -87,6 +89,38 @@ function testSchema() {
+    }
+    $this->assertTrue($fail, 'Ensure that db_copy_table_schema throws an exception when the destination table does already exists.');

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new8.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,635 pass(es).
[ View ]

Fixed the patch directly, no interdiff, sorry :)

Status:Reviewed & tested by the community» Needs work

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.phpundefined
@@ -272,6 +272,20 @@ public function renameTable($table, $new_name) {
+    if ($this->tableExists($destination)) {
+      throw new SchemaObjectExistsException(t("Cannot copy @source to @destination: table @destination already exists.", array('@source' => $source, '@destination' => $destination)));
+    }

Sorry 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.

Status:Needs work» Needs review
StatusFileSize
new4.7 KB
new9.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,086 pass(es).
[ View ]

Good catch! I have seen that in other places in the schema classes but at least don't add it here.

Status:Needs review» Reviewed & tested by the community

thanks, my bad!

Status:Reviewed & tested by the community» Needs work

+++ b/core/includes/database.inc
@@ -680,6 +680,23 @@ function db_rename_table($table, $new_name) {
+function db_copy_table_schema($source, $destination) {
+  return Database::getConnection()->schema()->copyTable($source, $destination);

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?

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -408,6 +408,24 @@ public function fieldExists($table, $column) {
+   * Copies the structure of a table without copying the content.
...
+  abstract public function copyTable($source, $destination);

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.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
@@ -348,6 +349,25 @@ function renameTable($table, $new_name) {
+  public function copyTable($source, $destination) {
+    if (!$this->tableExists($source)) {
+      throw new SchemaObjectDoesNotExistException(String::format("Cannot copy @source to @destination: table @source doesn't exist.", array('@source' => $source, '@destination' => $destination)));
+    }
+    if ($this->tableExists($destination)) {
+      throw new SchemaObjectExistsException(String::format("Cannot copy @source to @destination: table @destination already exists.", array('@source' => $source, '@destination' => $destination)));
+    }
+    // @TODO The server is likely going to rename indexes and constraints
+    //   during the copy process, and it will not match our
+    //   table_name + constraint name convention anymore.
+    throw new \Exception('Not implemented, see https://drupal.org/node/2056133');

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.

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -408,6 +408,24 @@ public function fieldExists($table, $column) {
+   * @throws \Drupal\Core\Database\SchemaObjectExistsException
+   *   Thrown when the source table does not exist.
+   * @throws \Drupal\Core\Database\SchemaObjectDoesNotExistException
+   *   Thrown when the destination table already exists.

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.

Status:Needs work» Needs review
StatusFileSize
new8.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,839 pass(es).
[ View ]
new1.87 KB

> 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: Implement pgsql 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.

Status:Needs review» Reviewed & tested by the community

Cool, that seems fine to me as long as bot comes back happy.

I 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.

Status:Reviewed & tested by the community» Needs work
Issue tags:-API addition

The last submitted patch, 2056133_45.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API addition

#45: 2056133_45.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

back to RTBC

Title:Add db_copy_tableChange notice: Add db_copy_table
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

Could use a change notice.

Title:Change notice: Add db_copy_tableAdd db_copy_table_schema
Version:8.x-dev» 7.x-dev
Priority:Critical» Normal
Status:Active» Patch (to be ported)

Issue tags:-Needs change record

remove tag

Issue summary:View changes

Updated issue summary.