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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Assigned: chx » Damien Tournoud
Status: Active » Needs review
FileSize
4.09 KB

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.

chx’s picture

FileSize
3.77 KB
4.1 KB

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

dawehner’s picture

+++ 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?

plach’s picture

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.

yched’s picture

+++ 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" ?

dawehner’s picture

Working on a test

dawehner’s picture

FileSize
6.63 KB
6.43 KB

There we go.

dawehner’s picture

FileSize
6.75 KB
6.55 KB

ups, this was broken.

plach’s picture

@dawehner:

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

Crell’s picture

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?

chx’s picture

Issue tags: -API addition
FileSize
1.47 KB
6.45 KB

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

chx’s picture

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.

plach’s picture

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.

chx’s picture

FileSize
1.47 KB
6.56 KB

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.

Damien Tournoud’s picture

--- 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’s picture

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

Damien Tournoud’s picture

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

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.53 KB
4.08 KB
4.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.

plach’s picture

+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.
chx’s picture

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

Dries’s picture

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.

chx’s picture

FileSize
1.63 KB
7.66 KB

Sure.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.51 KB
9.03 KB

Here are some tests for the exceptions.

dawehner’s picture

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

plach’s picture

+++ 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? ;)

chx’s picture

FileSize
8.56 KB
628 bytes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great. I consider this as RTBC now.

plach’s picture

Another bogus character I missed before, sorry.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.62 KB
8.75 KB

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.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sure.

Crell’s picture

Thumbs up from me as well.

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned

Thumbs up from me too.

catch’s picture

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.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.75 KB

Fixed the patch directly, no interdiff, sorry :)

catch’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.7 KB
9.56 KB

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

thanks, my bad!

webchick’s picture

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.

chx’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
1.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: 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.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

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.

chx’s picture

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

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC

catch’s picture

Title: Add db_copy_table » Change 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.

chx’s picture

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

Issue tags: -Needs change record

remove tag

yched’s picture

Issue summary: View changes

Updated issue summary.

  • catch committed 0e129e5 on 8.3.x
    Issue #2056133 by chx, dawehner, plach, alexpott: Add db_copy_table().
    

  • catch committed 0e129e5 on 8.3.x
    Issue #2056133 by chx, dawehner, plach, alexpott: Add db_copy_table().
    

  • catch committed 0e129e5 on 8.4.x
    Issue #2056133 by chx, dawehner, plach, alexpott: Add db_copy_table().
    

  • catch committed 0e129e5 on 8.4.x
    Issue #2056133 by chx, dawehner, plach, alexpott: Add db_copy_table().