Fix regression to menu serialization in upgrade path that causes thousands of errors in PostgreSQL

looks like entity schema updates are not applied properly, so conversion from varchar to blob could be broken

Just tested manually on "9.4.4" test fails

Yes the --die-on-fail has to do with a weird hack to simpletest that doesn't actually work with --die-on-fail. Sadly not related to this.

I remember the same issue was in d7 when we converted cache tables to blobs, "bytea" has issues with convertion
http://www.postgresql.org/docs/9.1/static/datatype-binary.html

EDIT http://www.postgresql.org/docs/9.1/static/ddl-alter.html#AEN2793

5.5.6. Changing a Column's Data Type

To convert a column to a different data type, use a command like:

ALTER TABLE products ALTER COLUMN price TYPE numeric(10,2);

This will succeed only if each existing entry in the column can be converted to the new type by an implicit cast. If a more complex conversion is needed, you can add a USING clause that specifies how to compute the new values from the old.

PostgreSQL will attempt to convert the column's default value (if any) to the new type, as well as any constraints that involve the column. But these conversions might fail, or might produce surprising results. It's often best to drop any constraints on the column before altering its type, and then add back suitably modified constraints afterwards.

So it seems like there is a more fundamental bug in the pgsql driver in terms of schema changes? or is there some other step we need to take?

Since we agreed to support postgres for 8.0.0, and this is a critical postgres failure, bumping to critical.

However one way to 'fix' this would be to change the database dump to be based on beta14 instead of beta12.

We might have to add some extra test coverage for entity updates, but then this particular update would never need to run and the problem goes away.

@mradcliffe if the cache clear happens after the updates have run, then why would it be failing?

We shouldn't be clearing caches before updates have run at all.

Oh I see https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Tests%...

Opened #2558247: Remove rebuildAll() and module install from UpdatePathTestBase.

Just to let peoples' heart rate down a touch. ;)

called to re-test pg after #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state

Postponing on #2558247: Remove rebuildAll() and module install from UpdatePathTestBase, turns out we were hiding this error in the update path tests with a hack to literally remove the errors from the assertion checking, but in a way that only works for MySQL. That's not really a postgres issue at all.

If #2558247: Remove rebuildAll() and module install from UpdatePathTestBase gets done there might be nothing to do here.

Discussed with @catch, @webchick, @alexpott, and @effulgentsia. If #2558247: Remove rebuildAll() and module install from UpdatePathTestBase resolves all of this, we can close it as a duplicate. If it doesn't, we should evaluate the criticality of any remaining issues for postgres at that time (so marking for later discussion).

So, #2558247: Remove rebuildAll() and module install from UpdatePathTestBase now has a green patch, I just did a postgresql test run there: https://www.drupal.org/pift-ci-job/24511

So, that seems to fix about 50% of the failing tests, but there are still a few that have those notices in menu storage. can someone with a working postgresql environment test that and post a full backtrace of those notices and when they happen exactly?

Ok, looks like that issue does not fix this.

But we assume that the reason is quite different now. It is likely that the update functions aren't running through and then we get those errors after an imcomplete update.php run.

So, still unpostponed but we can then look into those real problems once that other issue is in.

For me those tests work locally (I mean they just produce notices):

Notice: unserialize(): Error at offset 61 of 64 bytes in Drupal\Core\Menu\MenuTreeStorage->loadFullMultiple() (line 741 of core/lib/Drupal/Core/Menu/MenuTreeStorage.php).
Drupal\Core\Menu\MenuTreeStorage->loadFullMultiple(Array)
Drupal\Core\Menu\MenuTreeStorage->loadFull('standard.front_page')
Drupal\Core\Menu\MenuTreeStorage->doSave(Array)
Drupal\Core\Menu\MenuTreeStorage->saveRecursive('standard.front_page', Array, Array)
Drupal\Core\Menu\MenuTreeStorage->rebuild(Array)
Drupal\Core\Menu\MenuLinkManager->rebuild()
Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->menuLinksRebuild()
Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->onRouterRebuild(Object, 'routing.route_finished', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('routing.route_finished', Object)
Drupal\Core\Routing\RouteBuilder->rebuild()
Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()
drupal_flush_all_caches()
Drupal\system\Controller\DbUpdateController::batchFinished(1, Array, Array, '29 sec')
call_user_func_array(Array, Array)
_batch_finished()
_batch_page(Object)
Drupal\system\Controller\DbUpdateController->handle('start', Object)
call_user_func_array(Array, Array)
Drupal\Core\Update\UpdateKernel->handleRaw(Object)
Drupal\Core\Update\UpdateKernel->handle(Object)

Some more interesting result: These are the entries which are stored in the database after the menu rebuild:

>>> $b = new \Drupal\Core\StringTranslation\TranslationWrapper('Books');
=> Drupal\Core\StringTranslation\TranslationWrapper {#564}
>>> serialize($b);
=> "O:48:"Drupal\Core\StringTranslation\TranslationWrapper":3:{s:9:"\x00*\x00string";s:5:"Books";s:12:"\x00*\x00arguments";a:0:{}s:10:"\x00*\x00options";a:0:{}}"

For some reasons though, these null bytes cause issues in pgsql, see https://php.net/manual/de/function.serialize.php#96504 as one example
People seem to suggest to use something like https://secure.php.net/manual/en/function.pg-escape-bytea.php#89036
Do we need to care about that in our DB layer?

@dawehner: I'm sure we serialize objects with protected properties already, e.g. in the cache tables? Also, shouldn't all tests be failing with those if it would be a more generic problem?

Well sure, I hope that the trait does not make sense worse ... but well you see this unserialize call which is triggered as part the drupal_flush_caches() at the end of update.php
At that point the entries should be fine already. Yeah in other words no clue.

Here is a try to fix some of the test failures which are caused by the fact that we don't convert some exceptions properly

Looking at the PDO docs, is seems the way we cal execute, all params are just treated as strings:
https://secure.php.net/manual/en/pdostatement.execute.php

Would it make sense so have a way to to bind the values so we can specify that it's binary?

https://secure.php.net/manual/en/pdostatement.bindvalue.php

Never mind, looks like we are already doing that in the postgres driver.

Could this relate to caching of the table schema? I see in the postgres driver we are checking the schema info to decide which fields are blob to handle them correctly using bindParam() with type \PDO::PARAM_LOB.

#2558247: Remove rebuildAll() and module install from UpdatePathTestBase has landed

Running #28 past the bot.

If #28 comes back green, let's split it off as a quick fix. The less postgres fails we have, the less easy it is to introduce new ones at least.

Just ran the other tests:

fail: [Completion check] Line 54 of core/modules/aggregator/src/Tests/FeedLanguageTest.php:
The test did not complete due to a fatal error.


exception: [Uncaught exception] Line 868 of core/vendor/symfony/dependency-injection/ContainerBuilder.php:
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "renderer" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 868 of /var/www/html/core/vendor/symfony/dependency-injection/ContainerBuilder.php). Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('renderer')
Symfony\Component\DependencyInjection\ContainerBuilder->get('renderer')
Drupal::service('renderer')
install_database_errors(Array, './sites/simpletest/207534/settings.php')
install_verify_database_settings('sites/simpletest/207534')
install_begin_request(Object, Array)
install_drupal(Object, Array)
Drupal\simpletest\WebTestBase->doInstall(Array)
Drupal\simpletest\WebTestBase->setUp()
Drupal\aggregator\Tests\AggregatorTestBase->setUp()
Drupal\aggregator\Tests\FeedLanguageTest->setUp()
Drupal\simpletest\TestBase->run(Array)
simpletest_script_run_one_test('15', 'Drupal\aggregator\Tests\FeedLanguageTest')

And well of course, those tests work fine on my local pgsql installation, :(

Well #28 fixes half of the fails vs. HEAD so still worth getting in on its own?

Committed and pushed #28, setting back to active for the rest.

Just an alternative idea for an solution:

Don't store the translation wrapper but rather an array: ['title' => 'meh', 'context' => 't_context', 'translatable' => TRUE] and special case some behaviour.

Here is a patch just to run some pgsql test.

just some ordinary tagging

Ok, here's a possible approach now that the early rebuild is removed. Seems to fix the bug for me in a local install with pgsql - possibly it just can't work there to convert UTF-8 varchar to bytea and then try to read it back and serialize it.

note that this is similar to an approach dawehner suggested originally of creating columns with new names, but the renaming only lasts during the update function so we don't have to change any code.

#41 looks good - both the patch itself and the test results.

All the other test failures are unrelated to this one - we'll need a new postgres critical to track them though.

We are removing test failures, this is great!

Not sure where, but it'd be good to document this workaround centrally for other upgrade paths to emulate.

Is there any way we could make the postgres driver handle some of this?

I'm thinking if changeField() did the following:

- rename old field
- create new field
- copy old field data to new field with an INSERT INTO ..SELECT FROM.

in this case we'd have to then take the copied data and update it again to the correct format.

CNR for that question, if the answer is 'no' then I think this is right.

Also since this requires changing the update that runs for MySQL too, bumping to beta target.

INSERT INTO ... SELECT FROM won't work if the field is NOT NULL or has a default, since then it'll have data as soon as it's created.

I did find this though: http://www.postgresql.org/message-id/25980.1358264400@sss.pgh.pa.us

ALTER TABLE test ALTER COLUMN val TYPE varchar(255) USING convert(val);

with whatever is appropriate in place of convert().

That seems worth looking into.

@catch - this update was particularly problematic since we wanted to take the existing data and serialize it inside the TranslationWrapper and re-use the field name. That's not a very common pattern for updates.

the postgres driver already has some special handling for bytea fields, but possibly this would be a better general strategy. I don't think it's in scope for this issue, and it's unclear if it would have prevented the problem.

Let's make a follow-up for that: #2560691: Consider more robust alternatives for PostgreSQL when changing fields to a blob type

@catch - this update was particularly problematic since we wanted to take the existing data and serialize it inside the TranslationWrapper and re-use the field name.

Do we know that this is the actual problem though?

Or is it that the data is corrupted when we do the conversion of the type, then that corruption persists through the actual data update? If it's that, then just fixing the initial type conversion would fix this - and that's data loss just from a type change, not from the extra conversion.

I looked to see where our bytea special casing was added. It does the exact opposite of what everyone on the internet says you should do with a bytea column (i.e. cast to it).

I get commit hash 9166c436a054187feb29dbf89f9100dbd60c220b

Which was #2443681: PostgreSQL: Fix user\Tests\UserAccountLinksTest, which never had a patch with that code in.

Trying to just nuke that to see what happens.

I don't think that's right. The current code was added in #1031122: postgres changeField() is unable to convert to bytea column type correctly specifically to avoid this problem

This is now a re-roll of #1031122: postgres changeField() is unable to convert to bytea column type correctly.

We committed a bad patch, then neither reverted nor fixed it.

I sill think that's wrong since standard_conforming_strings in ON by default, but we are adding quoting as if it is off.

Guess let's see what the result is of the test, but I can also try it locally.

Uploading stefan_r's patch from https://www.drupal.org/node/1031122#comment-8836081

If one of these passes, we should probably bump that issue back to critical and mark this one as duplicate.

Patch #56 fails for me locally.

Let's see if this combination of parts is better.

Nice one! 25 fails down from 60+.

Going to do what I said in #56 and mark this duplicate of #1031122: postgres changeField() is unable to convert to bytea column type correctly. See you over there.

re-opening per catch to just fix the update function.

Just a typo fix in code comment and whitespace fix compared to #41.

This issue has been RTBC already

So yes I think #1031122: postgres changeField() is unable to convert to bytea column type correctly is a 'critical postgresql' issue that this rediscovered.

However there are good arguments for doing the update like this regardless of that issue.

Once we commit this (regardless of which issue it goes in for), there is no implicit test coverage for the bytea issue, and that issue also needs more time to sort out the proper fix.

So I think it makes sense to commit this now - to get the total number of postgres test faiures down by over half to around 25. I've just split out the remaining failures to dedicated issues and doubled the criticals list - most of these are new test coverage added by other critical issues afaict. The less, and more consistent, postgres fails we have, the harder it is to introduce new ones because we have at least a chance of spotting them.

Will just let the bot run to get final numbers on remaining test fails, then I'll commit this one.

I only uploaded other people's patches here, so I think I'm good to commit this.

Committed/pushed to 8.0.x, thanks!