UPDATE foo SET bar=(SELECT...) is not supported

How I ended up following this without ever seeing it I do not know. Does d.o auto-follow now when an issue is assigned to someone?

Either way, looks sane.

@Crell: Yes.

We need to prepare this sub-query like we prepare all the others, and make sure that the arguments bubble up properly. It works in the test because the sub-query doesn't use any placeholders.

Right.

I hope @chx wasn't already working on this, but here's an update that seems to work. Not 100% sure if this is implemented correctly. For some reason that I don't quite understand are we calling compile() on conditions both in execute() and in __toString(). Confirmed with expressions that compile() needs to be called before arguments() but I don't think it needs to be called twice.

I think converting all these database tests, or those that don't require hooks to unit tests or at least DrupalUnitTests would be a nice task, should speed up our tests quite a bit, there's a huge amount of test methods in there...

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/UpdateComplexTest.php
@@ -132,4 +132,18 @@ function testUpdateOnlyExpression() {
+    $results = array_map('intval', db_query('SELECT priority FROM {test_task}')->fetchCol());
+    $this->assertIdentical(array(29), array_unique($results));

Now that I look more carefully (bad Crell!), why the array_unique() here? That changes what's coming back in ways we cannot detect, so you may get back more than one row and not realize it. If we have to do this it should be commented, because otherwise it seems wrong to me.

I am planning to convert the DB tests to UnitTests post-feature-freeze when I have time. Actually I'll only be converting one of them and setting up the rest as Novice tasks. :-) I've not done that yet because there's other issues messing with the DB, and that's totally not a feature-freeze-sensitive issue so I'm prioritizing my time.

Well, it looks sane, the db maintainers think it looks sane and their concerns have been addressed with tests. Lets do it.

@Crell: Had to do something where I was actually able to make progress: #1839134: Convert database tests to (Drupal)UnitTestBase

I hope @chx wasn't already working on this, but here's an update that seems to work. Not 100% sure if this is implemented correctly. For some reason that I don't quite understand are we calling compile() on conditions both in execute() and in __toString(). Confirmed with expressions that compile() needs to be called before arguments() but I don't think it needs to be called twice.

Right. It's because this grew organically. The way it is supposed to work(TM) is:

• compile() should compile the main query and all its dependent objects
• arguments() should return the arguments of the main query and all its dependent objects
• execute() should be a simple:
  

  $this->preExecute();
  $this->compile($this->connection, $this);
  $this->connection->query((string) $this, $this->arguments(), $this->queryOptions);
  

• __toString() should also call ->compile() so we support casting the query to a string outside a context (the (string) $query syntax)

Select is about implemented correctly (getArguments() is totally unnecessary and should go away, but that's it).

Also, we need to reimplement ->compiled() as ->compiled($queryPlaceholder) because we support multiple compilation on the same object now.

So, all that to say that we do need the compile in __toString().

Like this?

We need a need for the addition to the __toString() functionality.

#15 looks great, it's addressed concerns from #11 and #7... and we've got full test coverage for all changed lines of code.

We're good to go here!

Approved here.

Committed and pushed to 8.x. Thanks!

Patch for D7. Also updated PostgreSQL driver for D7 with appropriate fix.

+++ b/includes/database/pgsql/query.inc
@@ -174,6 +174,13 @@ class UpdateQuery_pgsql extends UpdateQuery {
+        $selectQueryArguments = $data['expression']->arguments();

No camelCase for non-object-member variables.

This should be good then. Thanks for rerolling it @chx.

+        $select_query_arguments = $data['expression']->arguments();
+        foreach ($selectQueryArguments as $placeholder => $argument ){

This can't possibly work since the variable doesn't exist - perhaps it's the typo @chx intended to fix in #30?

More to the point, these PostgreSQL fixes aren't in Drupal 8... Let's get the issue fixed there first (and tested to make sure it actually works with the relevant database drivers), then backported.

UNTESTED, but presumably this is what it should look like for Drupal 8. I fixed the issue above (as well as a minor code style issue).

The code looks like it could also be simplified a fair amount (get rid of $select_query_arguments entirely; just use $argument) but this is consistent with the way the code above it is written, so I left that alone.

Refiling for the people who know Postgres, if any still pay attention to core...

PostgreSQL test bot found this in the failing test: ComplexUpdateTest::testSubSelectUpdate().

Just ran Database tests on PG testbot. Unfortunately there is still an exception, in Drupal\system\Tests\Database\UpdateComplexTest even with patch from #35 applied, see attached results.txt for a full test log.

SelectQueryInterface is not in drupal 8. Should be SelectInterface. I needed to include the interface for the test to pass.

Forgot to rebase like a scrub. Sorry, Crell.

If I understand #53 correctly PG is already broken and this doesn't make it worse. However, the latest patch is missing the tests from earlier issues.

I don't totally understand how binding works, so RTBC if tests pass and it has all tests to test that ...

Closing as fixed in Drupal 8 per Crell.

I will create a follow-up issue specifically for PostgreSQL Drupal 8 and Drupal 7. It also looks like this patch needs to be backported to Drupal 7 in general so I will create a follow-up issue on that.

- Hiding files that were added after issue was closed to be less confusing.

What is the process for PG SQL then?

It was not yet removed from core as much as I can see and we can ship it if:

a) tests pass

and

b) we have PG SQL test bots

So why closing that issue?

I created #2350461: Fix issues with UpdateComplexTest in PostgreSQL driver for postgresql in D8 and #2350465: UPDATE foo SET bar=(SELECT...) is not supported in D7? per request by Crell.

Okay, thanks. This is a duplicate then.

Sorry for playing ping-pong, but would like to know why: Closed (fixed) if there are other issues that are duplicates of this one?

Ah, something was fixed in this issue already - and committed. Makes more sense now.

Leaving as closed fixed then.