INSERT INTO table SELECT * FROM ... not supported [#2056363]

Our support for "INSERT ... SELECT" queries only works with "SELECT (an explicit list of fields)".
Queries like "INSERT INTO new SELECT * FROM old WHERE..." cannot be written, although they are supported, in that exact syntax, in MySQL, pgSQL & SQLite.

All our current uses of db_insert($table)->from($select)->execute(); in current core are done with a $select query that has an explicit list of fields - i.e. built with $select->fields('table', array('some', 'specific', 'fields')), not $select->fields('table'). The latter fails with the exception below.

From /core/lib/Drupal/Core/Database/Query/Insert.php:

    // Don't execute query without fields.
    if (count($this->insertFields) + count($this->defaultFields) == 0) {
      throw new NoFieldsException('There are no fields available to insert with.');
    }

From /core/lib/Drupal/Core/Database/Query/Insert.php,
and present exactly as is in /core/lib/Drupal/Core/Database/Driver/*/Insert.php:

     if (!empty($this->fromQuery)) {
      return $comments . 'INSERT INTO {' . $this->table . '} (' . implode(', ', $insert_fields) . ') ' . $this->fromQuery;
    }

Reading through #481288: Add support for INSERT INTO ... SELECT FROM ..., though, I can't really convince myself that it's a deliberate restriction rather than simply an omission.
Supporting "copy rows as is, whatever the columns are", is needed for the upgrade path in #1497374: Switch from Field-based storage to Entity-based storage.

Comment	File	Size	Author
#15	drupal-2056363.patch	6.32 KB	Sweetchuck
#15
#9	insert-select-all-2056363-9.patch	7.52 KB	yched
#9
#8	insert-select-all-2056363-8.patch	35.41 KB	yched
#8
#8	interdiff.txt	1.68 KB	yched
#4	insert-select-all-2056363-4.patch	6.99 KB	yched
#4
#4	interdiff.txt	3.09 KB	yched
#2	2056363_2.patch	7.41 KB	chx
#2
#2	interdiff.txt	1.37 KB	chx
#1	insert-select-all-2056363-1.patch	6.99 KB	yched
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 2 August 2013 at 12:40

Status:

Active

» Needs review

File	Size
insert-select-all-2056363-1.patch	6.99 KB

Temptative patch.

Comment #1.0

yched CreditAttribution: yched commented 2 August 2013 at 13:28

Issue summary:

View changes

rephrase

Comment #2

chx CreditAttribution: chx commented 3 August 2013 at 09:37

Assigned:

Unassigned

» Crell

File	Size
interdiff.txt	1.37 KB
2056363_2.patch	7.41 KB

Minimal formatting fixes, assigning to Crell for review.

Comment #2.0

chx CreditAttribution: chx commented 3 August 2013 at 09:37

Issue summary:

View changes

rephrasse

Comment #2.1

yched CreditAttribution: yched commented 3 August 2013 at 13:22

Issue summary:

View changes

clarification

Comment #3

yched CreditAttribution: yched commented 3 August 2013 at 13:27

Priority:

Normal

» Critical

Bumping to 'critical', since it blocks #1497374: Switch from Field-based storage to Entity-based storage

Comment #4

yched CreditAttribution: yched commented 3 August 2013 at 13:37

File	Size
interdiff.txt	3.09 KB
insert-select-all-2056363-4.patch	6.99 KB

Minor - fix double spacing in the generated query if there is a list of fields.

Comment #5

Crell CreditAttribution: Crell commented 3 August 2013 at 18:55

How does this block #1497374: Switch from Field-based storage to Entity-based storage? I haven't looked at that patch. Readers' Digest version, please. :-)

Comment #6

yched CreditAttribution: yched commented 3 August 2013 at 19:08

@Crell:
The upgrade path for #1497374: Switch from Field-based storage to Entity-based storage needs to reshuffle field table storage, and move data rows from old to new tables.
But it doesn't know the column names in those tables, because those vary with the field type, and the info lies in a method in the various "field type" plugins, and plugins are out of reach during updates (#2055605: Disallow use of plugins during update).
We don't really need that info anyway, all we need is "take whatever's in here, put it over there" - aka "INSERT INTO new SELECT * FROM old WHERE ..."

Comment #7

Crell CreditAttribution: Crell commented 4 August 2013 at 00:26

Hm, I see. Fair enough. One comment on the code.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/InsertTest.php
@@ -165,5 +165,21 @@ function testInsertSelect() {
+    // Test the "INSERT INTO ... SELECT * FROM" syntax.
+    $query = db_select('test_people', 'tp')
+      ->fields('tp')
+      ->condition('tp.name', 'Meredith');
+    // The resulting query should be equivalent to:
+    // INSERT INTO test_people_copy
+    // SELECT *
+    // FROM test_people tp
+    // WHERE tp.name = 'Meredith'
+    db_insert('test_people_copy')
+      ->from($query)
+      ->execute();
+
+    $saved_age = db_query('SELECT age FROM {test_people_copy} WHERE name = :name', array(':name' => 'Meredith'))->fetchField();

This belongs in a separate test method from the previous test. It's testing a different thing (explicit vs. implicit field specification).

Comment #8

yched CreditAttribution: yched commented 4 August 2013 at 00:49

File	Size
interdiff.txt	1.68 KB
insert-select-all-2056363-8.patch	35.41 KB

Works for me.

Comment #9

yched CreditAttribution: yched commented 4 August 2013 at 00:56

File	Size
insert-select-all-2056363-9.patch	7.52 KB

Whoops. Did not merge HEAD before diffing.
Correct patch attached, interdiff is the same.

Comment #10

Crell CreditAttribution: Crell commented 4 August 2013 at 02:11

Status:

Needs review

» Reviewed & tested by the community

Bot can object if it wants.

Comment #11

chx CreditAttribution: chx commented 4 August 2013 at 03:09

#6 is not correct, the situation is MUCH worse: the information about field tables are only available in Drupal 7 -- or by introspection. For example, file fields now use target_id and they used fid before. Someone might have added custom indexes to their field tables. And so on.

Comment #12

yched CreditAttribution: yched commented 4 August 2013 at 03:43

Well, #6 is correct, but right, on top of that, asking for the field schema through the API would return an info that is not yet in sync with the state of the db at the time this specific update runs.
So field schema is doubly unusable here, and we do need to just SELECT *