Problem/Motivation
Drupal works only on PostgreSQL when it is installed on the default schema called "public". Drupal should also work on other schemas.
Proposed resolution
Change the hardcoded schema name "public" to one set by the connection options from settings.php.
To test that it all works a number of tests need to be added. The drupal testbot has for PostgreSQL an extra schema called "testing_fake". This schema was added for this issue.
What operation the test should do:
- The test should start by creating a cloned database connection with the schema set to "testing_fake";
- Create a table in the new schema;
- Add, change, and delete a primary key;
- Add, change and delete a unique key;
- Add, change and delete a non unique key;
- Add, change and delete a column/field;
- Do a table search;
- Rename a table;
- Insert data in the table;
- Update the data in the table;
- Merge the data in the table;
- Upsert data in the table;
- Delete data from the table;
- Truncate the data in the table;
- Drop the table;
All operations should be tested that do what they should. For instance when you test renaming a table, test that there is no table with the old name and there is a table with the new name.
Remaining tasks
Write the patch with the test.
Review the patch.
Commit the patch.
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
TBD
The original summary
I've tried to install Drupal 7 on 2 separate systems using a PostgreSQL 9.0 database backend, and both times it has failed at the same point.
The error message in the PostgreSQL logs both times is:
ERROR: null value in column "rid" violates not-null constraint
STATEMENT: INSERT INTO role_permission (rid, permission, module)
VALUES (NULL, 'administer blocks', 'block')
I followed the instructions in INSTALL.pgsql.txt, and everything else
installs up to this point.
The actual error message returned on the installation page is:
An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://bison:8089/install.php?profile=standard&locale=en&id=1&op=do
StatusText: OK
ResponseText:
Home | Drupal
@import url("http://bison:8089/modules/system/system.theme.css?0");
@import url("http://bison:8089/modules/system/system.messages.css?0");
@import url("http://bison:8089/modules/system/system.menus.css?0");
@import url("http://bison:8089/modules/system/system.base.css?0");
@import url("http://bison:8089/modules/comment/comment.css?0");
@import url("http://bison:8089/modules/field/theme/field.css?0");
@import url("http://bison:8089/modules/node/node.css?0");
@import url("http://bison:8089/modules/search/search.css?0");
@import url("http://bison:8089/modules/user/user.css?0");
@import url("http://bison:8089/modules/system/system.admin.css?0");
@import url("http://bison:8089/modules/system/system.maintenance.css?0");
@import url("http://bison:8089/themes/seven/reset.css?0");
@import url("http://bison:8089/themes/seven/style.css?0");
Home
Installation tasksChoose profile(done)Choose language(done)Verify
requirements(done)Set up database(done)Install
profile(active)Configure siteFinished
SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current
transaction is aborted, commands ignored until end of transaction
block
Steps to recreate:
Gentoo x64 (2.6.31 kernel)
Apache 2.2.15
PHP 5.2.14
PostgreSQL 9.0.3
1) Run: wget http://ftp.drupal.org/files/projects/drupal-7.0.tar.gz
2) Extract using: tar xvf drupal-7.0.tar.gz
3) Create destination directroy: mkdir /var/www/drupal-7.0
4) Copy files: mv drupal-7.0/* drupal-7.0/.htaccess /var/www/drupal-7.0
3) chgrp -R apache /var/www/drupal-7.0
4) Run: createuser --pwprompt --encrypted --no-createrole --no-createdb drupal
5) Run: createdb --encoding=UTF8 --owner=drupal drupal
6) Run: psql -U postgres -c "CREATE SCHEMA drupal AUTHORIZATION drupal;" drupal
7) Change to drupal dir: cd /var/www/drupal-7.0
8) Change permissions on default sites dir: chmod a+w sites/default
9) Create settings file: cp sites/default/default.settings.php sites/default/settings.php
10) Update settings permissions: chmod a+w sites/default/settings.php
11) Update sites/default/settings.php file as per INSTALL.pgsql.txt with schema name, by adding this line: $db_prefix = 'drupal';
12) Navigate to webpage and follow installation.
13) Error appears as described above.
Screenshots of installation attached.
Installation appears to work fine when using a schema named anything but "drupal", or not using a schema at all.
Comment | File | Size | Author |
---|---|---|---|
#176 | interdiff_173-176.txt | 4.77 KB | Arantxio |
#176 | 1060476-176.patch | 25.42 KB | Arantxio |
#173 | interdiff_171-173.txt | 663 bytes | ravi.shankar |
#173 | 1060476-173.patch | 24.68 KB | ravi.shankar |
#171 | interdiff_167-171.txt | 3.33 KB | ravi.shankar |
Issue fork drupal-1060476
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 106047-issues-pgsql-schema changes, plain diff MR !1401
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the detailed bug report!
It looks like this is happening when the administration role gets created, via this code:
Somehow the role ID is winding up NULL, so the issue must occur in user_role_save(), before it.
If you look in the {watchdog} table in Drupal's database after this occurs, what kind of messages appear at the end? Are there any relevant error messages?
Comment #2
dark_ixion CreditAttribution: dark_ixion commentedI've checked what appears in the watchdog table as you suggested, and there appears to be some issues in the last few rows, although what's specifically wrong I don't know. Output attached.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedHm, I am not really sure how to intrepet that. Normally the "variables" column of that database table would contain something human-readable, but in your case it doesn't seem to....
I'm afraid I don't know a whole lot about PostgreSQL myself, but perhaps moving it to a different place in the issue queue will get some other people's eyes on it?
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedI got exactly the same error during an installation. Debian Squeeze, Apache 2.2.16, php 5.3.3-7 and PostgreSQL 8.4.7.
I know it was possible because I already did an installation on a PostgreSQL server without any problems. I figured out that if you are installing Drupal in a different database schema as the default public, you got this problem. Installing in public works like a charm...
Comment #5
hshana CreditAttribution: hshana commentedAnybody have a work around? This worked fine in 6...
Comment #6
Josh Waihi CreditAttribution: Josh Waihi commentedOk, this makes sense to me now. I'm happy with the commit. Its kinda minor since its more a PostgreSQL specific issue than anything to do with Drupal.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedEr, there is no patch here :) Maybe you posted that in the wrong issue?
Comment #8
Josh Waihi CreditAttribution: Josh Waihi commentedLOL, I fully did, apologies!
Comment #9
dark_ixion CreditAttribution: dark_ixion commentedTo answer the question as to why the variables column is returning hex code, that's because I'm using PostgreSQL 9.0 which, by default, uses hex output for bytea columns. If I change this to "escape", it shows normal text.
Comment #10
agentile CreditAttribution: agentile commentedSame error presented itself when trying to install Drupal in drupal schema.
Environment:
PHP 5.3.6
PostgreSQL 8.4.6
Apache 2.2.11
Solaris 5.11 snv_130
Would be nice for this to be addressed as it breaks Drupal installation and forces installation into a public schema, which depending on the use case of Drupal may not be ideal. Thanks.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedMost likely, the ID generator doesn't play nice with schemas.
From looking at the code, it seems that we are reusing the sequence generator from the
{sequences}.value
serial column. This is an horrible hack, and I bet that the sequence doesn't have the name we expect when schemas are being used.Comment #12
Josh Waihi CreditAttribution: Josh Waihi commentedI just installed Drupal 7 fine on PHP 5.3.2-1ubuntu4.7, PostgreSQL 8.4.7 and Apache 2.2.14 under a PostgreSQL schema called "drupal". Thats the standard build on Ubuntu 10.04 LTS.
I didn't test the user workflow however. I filled out the $databases array manually and set the prefix key to "drupal.".
Can someone else try that and see if it still doesn't work for them?
Comment #13
dark_ixion CreditAttribution: dark_ixion commentedJosh, what PostgreSQL user name are you using for the database? This can make a difference since the default schema search path is $user,public meaning if there is a schema named the same as the user, it gets searched first without the need of a prefix.
Comment #14
tbarreno CreditAttribution: tbarreno commentedHi,
It really looks like a schema/search patch issue.
I've a Drupal 7 default installation and recently created a test site. The default configuration relies in a database schema called 'drupal' (with the same username: 'drupal'). The test site has a new schema and username ('drupaltest'). Every time I tried to install Drupal test site I got the same response (posted by dark_ixion).
The solution was specify the schema with "
prefix
":Then, the installation was successfully.
Best Wishes
Comment #15
Josh Waihi CreditAttribution: Josh Waihi commentedThis seems related to #956942: Use current postgresql schema instead of hardcoded "public". . We should be settings the search path to the correct schema on connection. I recall we were looking at this but were waiting for prefixes per connection to get in first.
See other related issues: http://drupal.org/project/issues/drupal?text=search_path&status=All
Comment #16
dark_ixion CreditAttribution: dark_ixion commentedAre you sure we should be setting the search path at all? First of all the prefix should take care of any problems with things not being found in the search path. And also, the case I'm reporting here is where both the user name and schema name is 'drupal', and since $user appears before 'public' in the schema search path, it should already be in there and therefore not a problem.
But from what I think you're suggesting, the prefix functionality isn't working correctly anyway, so maybe that's making it fail somewhere.
But excluding the prefix anyway, I tried something to rely on it using a schema named after the user, and I get the same error on the web installation interface, and the same error in the PostgreSQL log:
Comment #17
dark_ixion CreditAttribution: dark_ixion commentedI think I know roughly in which area the problem lies:
In includes/database/pgsql/schema.inc there's this bit of code:
That hard-coded public schema value makes its way into the real output. If, using my setup, I replace that with 'drupal.', installation is successful. Obviously that's not the solution, but it does mean that $key is not being assigned the schema-qualified table name.
Comment #18
dark_ixion CreditAttribution: dark_ixion commentedI've also just done a quick check of an installation with a schema name different from the user name (which, as I've previously reported, works fine), and none of the tables are in the schema specified by $db_prefix in my settings file. It is therefore assumed that the INSTALL.pgsql.txt file is misleading in that setting the $db_prefix variable will result in it being used for the schema name. This is clearly not the case.
Drupal really shouldn't assume there is a public schema. It is actually bad practise to keep the public schema around due to security reasons. Ideally, if no schema is specified, it shouldn't prefix the table at all and just follow the schema search path for that user. This would mean that if there was a user named the same as the schema, all tables would automatically go in there by default (assuming the user's search_path setting hadn't been changed from the default of "$user",public). If such a schema didn't exist, they'd go into public anyway.
Comment #19
dark_ixion CreditAttribution: dark_ixion commentedOh, I should have mentioned that PostgreSQL has a current_schema function which returns the name of the first valid schema in the connection's search path. If that could be run and used to write into the prefix field in the settings file, it should always work.
Comment #20
Josh Waihi CreditAttribution: Josh Waihi commentedTo me, this issue lies in how Drupal uses prefixes rather than schemas which is a reflection of how MySQL treats schemas as databases which gives MySQL users no concept of a schema.
What that means is, a connection prefix should be renamed to a connection "schema", giving more reason to support dot syntax within a prefix. With that in place, the search_path can be set per connection based on the schema's outlined default schema if any, otherwise the search_path can be left alone.
With that in place, prefixing tables and keys would become much simpler. However, when schema details were addressed in D7, they were done before prefixes per connections were implemented
Comment #21
catchI marked #956942: Use current postgresql schema instead of hardcoded "public". , #1219674: Postgresql not public schema upgrade error, #1227004: Multiple issues with PostgreSQL driver and public schema all duplicates.
Same search also found http://drupal.org/node/1177322
We really need #1060798: Enable apachsolr 'related posts' block for issues (and forums?).
Comment #22
catchComment #23
mva.name CreditAttribution: mva.name commentedHey guys! As far as I see, it is silence here for about a year. And all isses still in 8.x/7.15.
Nobody wants to fix this?
By the way, I have one more issue related to this topic:
When trying to install d7.15 in non-public schema (when another copy is already installed there) Drupal says "Drupal could not be correctly setup with the existing database. Revise any errors."
as far as I looken in code — error happens when drupal trying to create functions in global namespace. And it seems, that PgSQL server (9.1.4) doesn't allow to do that.
P.S. How can I help to speed up issue fixing? I'm very interested in fixing it asap.
//wbr,
mva
Comment #24
Josh Waihi CreditAttribution: Josh Waihi commentedHey mva.name! By all means, please jump in and fix the issue! I have literally been stuck on a project for the last 18 months using a MySQL database (ewww) and since my effort is focused there, I haven't been able to put in time to fix these PostgreSQL issues. More than happy to review any patches though!
Comment #25
ericsol CreditAttribution: ericsol commentedJumping in to see if I can shed some light on this.
This thread is tagged version 8.x-dev. From the timing of the issue and the description I would assume its not. Since I can reproduce the issue in 7.21 I've changed the thread accordingly.
I've made separate installs with minimal profile and language = English with PostgreSQL 9.2:
I visually compared the two databases and saw no differences (same number of tables, same records in system, menu_router and so on).
In the next step I've made a dump of both schema's:
pg_dump -U sandbox4 --schema= > sandbox4<-db>.psql
Next step was to find/replaced all instances of "Schema: public" and "public." with "Schema: sandbox4" and "sandbox." in sandbox4-db.psql to rule out all differences in the dump caused by the name of the schema.
After that I compared the two dumps: diff sandbox4.psql sandbox4-db.psql > sandboxdiff.txt. I've attached the file for further inspection.
My initial finding is there are no meaningful differences between the dumps; the binary data being shown is probably an artefact of diff.
All this leads me to the assumption we might be looking in the wrong direction: this is not an installation issue but some other bug. The most likely cause is somewhere in menu_router, since paths like /admin produce WSOD and my error log points to line 2127 in system.module:
Any thoughts or comments?
Comment #26
ericsol CreditAttribution: ericsol commentedTo follow up on my post:
All of the reported problems disappeared when I changed the prefix from .
. to . I was experimenting with having user-related tables in one Drupal installation to share users across multiple sites. I now have all of this working through the use of the following database declaration in settings.php: I can now have multiple sites in one PostgreSQL database with shared user credentials! Marking this as closed fixed.Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedI've had the same error with mixed-case schema name where postgres would mangle it to become all lowercase. Making it lowercase in the Drupal settings fixed it.
Comment #28
JimmyAx CreditAttribution: JimmyAx commentedWhy is this issue closed? I'm having issues with my PostgreSQL Drupal site migrated from MySQL. Other sites migrated from MySQL using the schema "public" are not having issues.
One issue I'm having is my tables not dropping when I uninstall modules. I basically tracked it down to
db_table_exists()
which ends up inDatabaseSchema::getPrefixInfo()
that is using the hardcoded valuedefaultSchema
being set to exactly "public". This can not be changed in any way but hacking core.Comment #29
stefan.r CreditAttribution: stefan.r commentedComment #31
stefan.r CreditAttribution: stefan.r commentedComment #32
bzrudi71 CreditAttribution: bzrudi71 commentedPatch fails with:
Fatal error: Call to undefined method Drupal\Core\Database\Schema::construct() in /var/www/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php on line 36
If I remember right, construct() moved over to Connection.php?
Comment #33
BerdirIt means that it should be parent::__construct() :)
Comment #34
jaredsmith CreditAttribution: jaredsmith commentedI tried changing the patch to call parent::__construct(), but that caused other problems. I'll post the patch here just for reference, but I really don't expect it to solve the problem.
Comment #35
bzrudi71 CreditAttribution: bzrudi71 commentedYep, I think there are more hardcoded 'public' vars around and this could help but will not finally make Drupal work outside public schema. Also I don't think that this is required any more to pass tests, at least for the remaining once other than migrate - but that is thing of it's own to fix ;-) Thanks anyway @jaredsmith for the patch.
Comment #36
daffie CreditAttribution: daffie commentedComment #37
daffie CreditAttribution: daffie commentedChanged the hardcoded 'public' vars for the class variable $defaultSchema.
Comment #38
daffie CreditAttribution: daffie commentedOeps. The class variables go first!
Comment #39
bzrudi71 CreditAttribution: bzrudi71 commentedSorry, back to needs work ;-) I just installed D8 outside public schema, it works with, or without the patch. However, I did just a quick test run of the Database test group and there are still fails and exceptions with patch applied.
Comment #42
BORANBURCIN CreditAttribution: BORANBURCIN commentedHi,
I am using Drupal 8.0.2. I cannot install on non-public schema. Have you guys installed?
Comment #43
just_A_boy CreditAttribution: just_A_boy commented@BORANBURCIN Is this still being the case.. Same problem here on 8.0.3
Comment #44
ozden_meren CreditAttribution: ozden_meren commentedHi,
Same problem with drupal-8.0.5. I tried to create a patch file, but I don't have much experience about patch files. I hope it's in the correct format.
Comment #45
andypostPatch looks right, let's see what testing will tell
Comment #47
daffie CreditAttribution: daffie commented@ozden_meren: Your patch looks good. Thanks.
I think that we need to combine the patches from #38 and #44. We also need some tests to make sure that the problems are fixed and stay fixed.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNot sure why this can't be fixed in 8.1.x?
Comment #50
RoSk0Re-roll of patch #38.
Comment #52
RoSk0Improved patch version.
Comment #53
daffie CreditAttribution: daffie commentedThe patch looks good. I do have some remarks:
Can we default to "public" if $prefix_info['schema'] is empty.
Why is this change necessary?
The method Connection:: getFullQualifiedTableName() has a hard coded ‘public’.
We will also need some tests to make sure that all drupal works with a non-public schema.
Comment #55
RenrhafHello, I had the same issue trying to install a D8 8.3-x with a PostGreSQL schema different from the "public" one.
Using the patch in #52 works great, but due to short array notation change, this patch can't be applied automatically anymore.
So here the patch rerolled on D8 8.4-x. and some answers :
1. Default to "public" is already happening because of
protected $defaultSchema = 'public';
inSchema.php
2. Don't really know but there is no "else" statement anymore and it seems a bit strange.
3. I tried to do something in the "drupal-1060476-55-bis.patch". The
getFullQualifiedTableName
method seems to be called in migrations only. It can be called and tested like that\Drupal::service('database')->getFullQualifiedTableName('TABLE');
4. D8 8.4-x fresh install works properly.
Comment #56
RenrhafComment #57
RenrhafI faced some issues with the command "drush entity-updates", on field deletions, and more particularly on index renaming.
The new patch includes a fix for this too.
Comment #58
RenrhafFixing the case when INDEX needs to be dropped in a non-public schema.
Comment #60
RoSk0Re-roll.
Comment #61
RoSk0I think I finally figured it out, updated patch to follow soon...
Comment #62
RoSk0Changes:
Comment #64
RoSk0This one should work.
Comment #66
RoSk0Generated from drupal/drupal instead of drupal/core, same as #62.
Comment #68
RoSk0Added missed MySQL schema.
Comment #69
RoSk0Still on it, looks like we overlooked identifier name quoting in some places...
Comment #70
RoSk0Slightly reworked version.
Main changes from previous:
* Removed strpos() detection of schema where possible, replaced with Schema::getPrefixInfo()
* Used Connection::escapeTable() for identifier quoting
Comment #72
RoSk0Fixed unit test.
Comment #73
RoSk0Added small performance improvement and made unit test a bit more explicit.
Comment #74
AnaSwin CreditAttribution: AnaSwin commentedThanks a lot for the last patch (drupal-1060476-73.patch)!
I applied it on my website (Drupal 4.0 - PostGreSQL 9.4.7 - PHP 5.6). No regression seen yet.
Thanks for your work.
Comment #75
daffie CreditAttribution: daffie commentedThe patch looks good. But what I am missing is tests to make sure that the problem is fixed and to make sure that future patches do break the support for non-public schema's. Kerneltest will be diffecult to do with the current testbot. So my suggestion will be to create unittests. Let the PostgreSQL driver calculate the query string and test if that will work for different schema's (public, temporary and others). Do that for SELECT, UPDATE, DELETE, INSERT and the different ALTER queries. Create the test with provider methods. See: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writ....
Please, create a separate issue for this.
Comment #76
RoSk0Thanks for your review @daffie.
I don't think that writing tests for different query types will help. They all using Connection::prefixTables so I tried to test this. Lets see what testbot will say.
Removed max_identifier_length retrieval change as requested and added prefixTable test.
Comment #77
daffie CreditAttribution: daffie commented@RoSk0: Lets look at this in a different way. Lets say that this patch fixes all non default schema problems for PostgreSQL. What parts of this patch can later be changed back without the testbot generating any fails. The only security we have is the testbot. So we need tests for ALL the changes that make drupal core work on non default schema's.
Comment #78
RoSk0Reverted testComputedConstraintName changes related to #2921665: PostgreSQL: Optimize getting maxIdentifierLength.
@daffie Thanks for the suggestion, but I'm struggling to figure it out.
There is a major issue with testing this patch, at least in my understanding, major changes are in
Schema::queryTableInformation() and Schema::renameTable(). In Schema::queryTableInformation() we fixing logic and to test the outcome we need to actually talk to DB. In Schema::renameTable() we adding schema name to queries and same thing with testing only possible by real DB queries.
So in my opinion we only can test it indirectly by testing Connection::getFullQualifiedTableName() which has hard-coded
"public" schema at the moment and producing "drupal8-contrib.public.drupal.cache" instead of "drupal8-contrib.drupal.cache".
I'm open to suggestions on how to improve test coverage for this.
Comment #79
RoSk0Wake up testbots
Comment #80
RoSk0Comment #81
RoSk0Comment #83
RoSk0Ok, so my assumption that testComputedConstraintName will work was wrong, fixing...
Comment #84
RoSk0Comment #85
RoSk0Re-applied fix to testComputedConstraintName.
Fingers crossed.
Comment #86
RoSk0Comment #88
RoSk0Setting back to "Needs review" as tests passed, removing "Needs test" tag as tests were added.
Comment #90
RenrhafI confirm that the patch works properly.
Patching Drupal 8.5.3 and using a non-public schema (multisite with one schema per site) on PostGreSQL 9.5 works with #85.
Without the patch, the installation fails when using prefixes like "siteA.", "siteB.", etc.
Thanks RoSK0 !
Comment #92
daffie CreditAttribution: daffie commentedThe patch looks good and @Renrhaf has manual tested the patch with a non-public schema. That is great. I do have 2 minor remarks about this patch:
@RoSkO: Great work on this patch!
We do not have a subsystem maintainer any more after @crell left. So changing it to the framework maintainer.
Comment #93
mradcliffeI think that test failure is using the test-only patch and not the real patch in #85, @daffie.
I think that this may get rejected on further review as it's been frowned upon to add in driver-specific options in core _not_ in the driver itself. I think it would be possible to work around this by adding an optional parameter to getFullQualifiedTableName?
I don't think this is related to the patch, @daffie. I think this and some of the changes in PostgresqlSchemaTest are because there are additional calls to fetchField for checking indexes. It is because the old code should pass because the query that checks for indexes is truthy so it "works".
Comment #94
daffie CreditAttribution: daffie commentedThe testbot fixed my first remark from comment #92. For my second remark, I did take the time to have another good look at the test and I understand the changes now, but I do have a new point about the test:
I have a problem with this part. The entire idea of testing with a provider method is that you can add extra testcases to the provider method,
without changing the main test method. Only by adding the above codeblock, that is no longer possible. I do not think we can remove this codeblock from the test method. Therefor my suggestion would be to change this test to a kernel test. There you have a normal database connection and there is no need mocking used classes.
Comment #95
kattekrab CreditAttribution: kattekrab at Catalyst IT commentedLooks like this might be the bug that bit me today.
Comment #96
daffie CreditAttribution: daffie commented@kattekrab: Would you be able to test the patch from comment #85 and see if it fixes your problem. User @Renrhaf already confirmed that the patch worked for him. The only work on this patch is a test. The rest of this patch should work.
Comment #97
beram CreditAttribution: beram as a volunteer and at Ekino commentedHi,
I confirm that the patch from comment #85 is working on a non-public schema with Drupal 8.5.3.
Tested on multiple environments.
Comment #98
mradcliffeI agree that a kernel test would be useful. I confirmed in drupalci_environments that the database user is the database owner, however this may not always be the case for local development / testing environments. So if we're okay with possibly failing test runs on non-drupal.org systems, then that would make sense.
The current unit test could be modified to use the data provider because the test arguments are the same as what we're mocking, @daffie.
Comment #100
RoSk0Back to "Needs review".
I don't see any benefit in conversion to Kernel test unless we actually will test on non-public schema, but as far as I can tell we can't specify what schema to use when testing on PostgreSQL.
Comment #102
RenrhafRerolled against 8.7.x
Comment #104
daffie CreditAttribution: daffie commentedComment #105
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have re-rolled the patch.
Comment #106
daffie CreditAttribution: daffie commented@ravi.shankar: Thanks for the reroll.
Comment #107
daffie CreditAttribution: daffie commentedTo be able to test a patch for this issue a new testbot with a non-public schema is needed. Postponing untill there is such a testbot. See: #3104007: Postgresql 12 DrupalCI environment needed for core testing.
Comment #108
mradcliffeThere's an extra space on the left-hand side of the assignment operator.
Comment #109
ressa CreditAttribution: ressa at Ardea commentedI got this before patching:
Applying the patch 1060476-105.patch fixes it.
Also, if I try to define the prefix with a traling dot during installation (in Lando) like this
drush site:install --db-url=pgsql://postgres:@database/drupal8 --db-prefix=my_postgres_db. -y
I get an error:So I have to run
drush site:install
with--db-prefix=my_postgres_db
and manually add the dot to the prefix insettings.php
afterwards, like this:Comment #110
daffie CreditAttribution: daffie commentedWe have now testbots for PostgreSQL 10 and 12. Both have an extra schema called:
testing_fake
. Because all PostgreSQL testbots must pass the added testing, I move this issue to 9.1.Comment #111
ressa CreditAttribution: ressa at Ardea commentedTo my luck, the table in the database-dump I was importing has since changed from
my_postgres_db
to the genericpublic
, which made defining prefix with'prefix' => 'my_postgres_db.',
no longer necessary.Comment #112
daffie CreditAttribution: daffie commentedComment #113
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x. Kindly review.
Comment #115
RoSk0In reply to #109: I believe your issue is related to bug in Drush described here https://github.com/drush-ops/drush/issues/1523 with fix here https://github.com/drush-ops/drush/pull/2567 .
Comment #116
ressa CreditAttribution: ressa at Ardea commentedThanks for working on supporting non-public schema for PostgreSQL, and sharing the Github issue @RoSk0! I hope it gets committed at some point.
Comment #117
RoSk0Still needs work to address:
Comment #120
intrafusion#113 doesn't apply cleanly to 9.1.8,
lib/Drupal/Core/Database/Driver/pgsql/Schema.php.rej
is attached but I can't see any reason why this doesn't applyComment #121
daffie CreditAttribution: daffie commentedHi @intrafusion, that you for the reroll!. At the moment is your patch not working. A patch file should have the extension ".patch". Also when you are working with multiple patches it helps when the naming of the patch has the issue ID in it followed by its comment ID. See: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa.... With which Drupal core branch are you working?
Comment #122
intrafusion@daffie, it's not my patch, it's the patch from #113
I posted the rejected file that was generated when I tried to apply the patch. I've compared the line by line changes and cannot understand why the patch doesn't apply correctly
Comment #123
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch given for 9.3.x.
Comment #124
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixing 'custom command fails' Issues.
Comment #125
quietone CreditAttribution: quietone as a volunteer commentedReviewing Bug Smash issues. I don't know db drivers well so can not comment on the actual fix or the tests here. I did not try to the steps to reproduce because they are for Drupal 7. The Issue Summary does not provide a list of tasks for a proposed resolution. So, even if I knew more about db drivers it is not possible to know if this patch is implementing the recommended fix. Tagging for issue summary update.
There were several rerolls since #105, none of which provided a diff. Those diffs are added.
@ridhimaabrol24 and @vsujeetkumar It is good practice to always add a interdiff or diff. It makes the task of the reviewer much easier.
Out of scope change
Why this change? Why does mysql have to change?
Out of scope
Out of scope
Line too lone, put the array on multiple lines. And we can avoid the two variables if we use $info['schema'] and $iinfo['table'] in the query string.
When there is an 'if' in a comment there is usually one in the code as well. What happens here when the default schema is used? It is not clear to me.
As above why is this needed?
Not needed, the switch has a default case where it is set.
Comment #126
daffie CreditAttribution: daffie commentedUpdated the IS.
@quietone: Thank you for the feedback!
Comment #127
daffie CreditAttribution: daffie commentedComment #128
mogtofu33 CreditAttribution: mogtofu33 as a volunteer and commentedUpdated patch for last 9.3.x (Test class didn't apply).
Including quietone remarks.
Comment #131
intrafusionIn relation to #7 in #1060476-125: Multiple issues when PostgreSQL is used with non-public schema by not making
getPrefixInfo
public I get the following error when using migrations:Error: Call to protected method Drupal\Core\Database\Schema::getPrefixInfo() from context 'Drupal\migrate\Plugin\migrate\id_map\Sql' in Drupal\migrate\Plugin\migrate\id_map\Sql->getQualifiedMapTableName() (line 288 of core/modules/migrate/src/Plugin/migrate/id_map/Sql.php) #0 modules/contrib/migrate_tools/src/MigrateExecutable.php(128): Drupal\migrate\Plugin\migrate\id_map\Sql->getQualifiedMapTableName()
Changing this function from protected to public fixes this error and allows the migrations to complete
Comment #132
intrafusionPatch attached which adds back #7 from #125, no idea how to create an interdiff
Also re-rolled for 9.3 as it was no longer applying cleanly, no idea about 9.4 yet
Comment #133
daffie CreditAttribution: daffie commentedThe tests from the proposed solution still needs to be added.
Comment #135
intrafusionPatch for 9.4 as the files have been moved into separate modules as per https://www.drupal.org/node/3129492
Comment #137
tostinni CreditAttribution: tostinni at Agence Propal commentedLast patch wasn't applying for 9.4 due to this file that has been moved
core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlSchemaTest.php
.Updating patch
Comment #138
tostinni CreditAttribution: tostinni at Agence Propal commentedFix incorrect patch uploaded for 9.4
Comment #139
daffie CreditAttribution: daffie commented@tostinni: Great to see that you are working on this issue. I shall help by reviewing your work. You can contact me on drupal.slack.com in the #contribute channel. My username there is the same as here.
The first goal is to get this fixed in 10.1.x. Later we maybe able to backport this to earlier Drupal versions.
The testbot for PostgreSQL has a special schema to make this issue testable and the schema name is
testing_fake
.The issue summery has a list of tests that are needed. It is a long list, only they are all very simple test. It is less work than it looks.
I have added the tag "Needs framework manager review" 4 years ago and I added it, because there was no subsystem maintainer for the Database API. As I am now that subsystem maintainer and I added the tag, makes me free to remove it. Not adding back the tag "Needs subsystem maintainer review", because as that subsystem maintainer, I think this issue should be fixed.
Comment #140
jordan.jamous CreditAttribution: jordan.jamous at Eighty Options commentedThanks all. I am getting this error with #138.
Updating the patch for 9.4.x
Comment #141
jordan.jamous CreditAttribution: jordan.jamous at Eighty Options commentedTrying again for 9.4.x and 9.5.x
Comment #142
ArantxioI created a new patch based on the patch from #141 which should be working on 10.1.x dev. This patch contains most or all test in the proposed resolution and is made in collaboration with @daffie.
Comment #143
ArantxioComment #144
daffie CreditAttribution: daffie commentedFirst quick review. The patch look good!
This added line can be removed.
This change is not necessary any more. We have a new function called
getPrefix()
we can use instead.This change can be removed. I am not sure if it is necessary. Therefor remove it for now.
Let the comment run until 80 characters.
Lets use getPrefix() instead of getPrefixInfo(). Please undo this change.
Lets get the schema from the connection $options.
Please remove one of the empty lines and add a comment with: "If the schema is not set in the connection options than schema defaults to public".
This change is unrelated to what we are doing in this patch (out of scope). There for please remove this change.
These 2 empty lines can be removed.
Comment #145
ArantxioCreated a new patch because the other one failed, and added the comments from @daffie in comment #144.
Comment #146
asad_ahmed CreditAttribution: asad_ahmed at OpenSense Labs commentedWorked on points 1, 3, 5, 7, 8 and 9 of #144. Please review and thanks
Comment #147
ArantxioForgot to add :ignore to cSpell for the ignored words.
Comment #148
ArantxioCreated a new patch for the failing tests, also assigned to myself as I will be working on this as much as possible with @daffie.
Comment #149
ArantxioA new patch to try and fix the failing tests.
Comment #150
ArantxioI had a double semicolon on one line, which caused the phpcs test to fail.
Comment #151
daffie CreditAttribution: daffie at Finalist commentedCan we change this to: "Create a connection to the non-public schema."
Can we rename the variable to
$this->testingFakeConnection
This line can be removed.
Can we, before we remove the connection assert that all tables have been dropped. "$this->assertEmpty($this->testingFakeConnection->schema()->findTables('%'));"
You can move the code from this method the setUp(). It is not much code and it removes a method.
Can we change this to "Tests schema API for non-public schema for the PostgreSQL driver"
Can we rename the method to "testExtensionExists()". The pgsql part is not necessary. We are already in the Drupal\pgsql namespace. The same for the other method names.
Can we add the line " * @coversDefaultClass \Drupal\pgsql\Driver\Database\pgsql\Schema"
Can we change this line to "* @covers ::extensionExists". We have already stated add the top of the class which class we are testing with @coversDefaultClass. For more info: https://phpunit.readthedocs.io/en/9.5/annotations.html#coversdefaultclass
This line can be removed.
Can we change the assertion to: "$this->assertSame()".
Please do not use variable names. Use $this->assertFalse($this->connection->schema()->tableExists('some_table'));
Please do this on a single line.
Comment #152
ArantxioChanged getPrefixInfo back to protected, removed unnecessary changes. improved some coding standards.
Adjusted the new test with the review points from @daffie in comment #151.
Comment #153
daffie CreditAttribution: daffie at Finalist commentedCan we do with the variable $index_short_name and use $index->indexname instead?
Why is this change necessary? Does the schema name really need to be added here?
Is this change really necessary? It feels wrong to me to add the schema name to an index. I am also worried that it will break existing Drupal sites.
Comment #154
daffie CreditAttribution: daffie at Finalist commentedCan we change this line to:
$this->connection->query('ALTER INDEX "' . $this->defaultSchema . '"."' . $index->indexname . '" RENAME TO ' . $this->ensureIdentifiersLength($new_name, $index_name, $index_type) . '');
.In this way we always add the schema name. The part with $index full name can than be removed and we can use
$index->indexname
instead of the variable $index_short_name.These changes are not necessary.
Comment #155
ArantxioI've added the changes from comment #154. We still have to check why 1 test is failing in: "Drupal\Tests\pgsql\Kernel\pgsql\SchemaTest", or rather why the last insert in the function "testChangePrimaryKeyToSerial()" is failing in "tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php"
Comment #156
ArantxioThis new patch does not fix the issue, but it has some improvements for better clarity of the code.
Also added both interdiff's as I've forgotten to add it to the past patch.
Comment #157
ArantxioForgot to remove a commented line for the issue I've been working on.
Comment #158
ArantxioSome extra test to see what fails when we change the key to get the table information
Comment #159
ArantxioCreated a new patch that seems to fix the last issue.
Made the diff with #157, as the previous patches were to check where it would fail.
Comment #160
ArantxioComment #161
ArantxioOne space too many on a if statement.
Comment #162
daffie CreditAttribution: daffie at Finalist commentedAll code changes look good to me.
There is testing been added.
After @arantxio talked to @catch on Slack to create a followup for the installer part and the settings.php part.
To me it is the best solution to create CR in the followup.
The followup is #3328215: Add option to change the PostGreSQL Schema during installation.
For me is it RTBC.
Comment #163
larowlanThanks for resurrecting this one folks, here's some observations, disclaimer I haven't used postgres for 15 years, but I think the observations are generic
I think 'of it' should be 'if it'
And probably 'else' should be 'otherwise'
nit: !==
personal preference, but I think this would be easier to read with sprintf so its clear the . is part of the string and not used for string concatenation
than => then
There's a few places this gets used in raw SQL (i.e. via string concatenation), should we sanitize it here or throw an exception if it contains special characters?
should the quoted key be quoted or can we guarantee that the temporary namespace name is safe?
previously we were querying with `public.$key` but now, in the 'public' case we're querying with '"$key"' i.e without the schema?
If we revert to the old logic, we can combine the elseif and else clauses, and in fact we can probably do that first, and then only have the if for the temporary schema (i.e no need for the else/elseif)
Here we're using $this->defaultSchema in raw SQL - should it be parameterized?
because we use parameters here
Comment #164
Arantxio@larowlan thank you for your comment, hereby is a new patch which includes some of the comments made:
163.1 - done
163.2 - done
163.3 - I have not changed this as it currently represents how it was, and I think this is currently out of scope. If we really want this I think we should create a follow up issue which addresses multiple of these type of lines.
163.4 - done
163.5 - I have added the sanitization like we do in "connections->escapeDatabase" but without adding te quotes. I have added this to the schema within the Connections class and the Schema class for pgsql.
163.6 - This is almost exactly how it used to be, the only thing that changed was the name of the parameter. So I won't change this. If we want to change this we could also make a follow up issue for this.
163.7 - I have changed it a little bit back to how it was with some of the changes we have made so it should work.
163.8 - I have not changed this, because when I make it parameterized it adds its own quotes to the query string. which made it fail.
Comment #165
ArantxioComment #166
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedA minor thing - can we remove the empty string concatenation at the end of the query?
It was left here by this commit. If we are touching this query, probably this can be also cleaned-up (as it seems it is not needed anymore).
Comment #167
ArantxioAdded the suggestion from @poker10, thank you for noticing.
Comment #168
daffie CreditAttribution: daffie at Finalist commentedAll points of @larowlan have been fixed or answered.
Back to RTBC.
Comment #170
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #171
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #167 on Drupal 10.1.x.
Comment #172
daffie CreditAttribution: daffie at Finalist commentedReroll failed.
Comment #173
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed Drupal CS issue of patch #171.
Comment #174
daffie CreditAttribution: daffie at Finalist commentedBack to RTBC.
Comment #175
larowlanShould we do the sanitizing of the schema name here (the preg_replace), we're doing it twice (once in ::setPrefix and again in Schema::__construct).
Doing it here would also save any caller of ::getConnectionOptions having to do the same sanitization
this looks like a bad merge, we moved to using str_contains/str_starts_with/str_ends_with in 10.1.x
Comment #176
Arantxio@larowlan I've adjusted the code according to your suggestions and also some changes due to the latest commits.
Comment #177
ArantxioComment #178
daffie CreditAttribution: daffie at Finalist commentedThe points of @larowlan have been addressed.
Back to RTBC.
Comment #179
larowlanIssue credits
Comment #181
larowlanCommitted to 10.1.x
I think this is eligible for backport, but will ask for a second opinion
Setting to patch (To be ported) in the meantime
Comment #182
catchI also think this is eligible but we're now approaching the rc of 10.1 and the last bugfix release of 10.0.x so might be easier to leave it in 10.1.x. It will mostly benefit new installs.
Comment #183
intrafusionIf it's going to be ported, I need for 9.5.x as this is issue is holding me back on 9.4.x
Comment #184
catch@intrafusion can you explain how it's preventing you from updating from 9.4.x to 9.5.x, and also are you able to apply the patch?
Comment #185
intrafusion@catch the last working patch for me is #1060476-138: Multiple issues when PostgreSQL is used with non-public schema and none tagged with 9.5.x, etc. apply cleanly. I haven't had an opportunity to review why
Comment #186
catch@intrafusion so do you mean you have the patch applied to 9.4.x, and will need to re-apply the patch if it's not backported to 9.5.x? In that case I would suggest trying to work from https://www.drupal.org/project/drupal/issues/1060476#comment-14995059 to create a new 9.5-compatible patch, or that version might even apply without changes. Moving back to fixed.
Comment #187
intrafusion@catch but it's not fixed, the patch still needs to be ported
Comment #188
catch@intrafusion it's fixed in Drupal 10.1.x, and the change won't be committed to 10.0.x, or 9.5.x - therefore from the point of view of core development the issue should be 'fixed'.
You are welcome to check if the latest patch applies to 10.0.x and 9.5.x, and to post a backport of that patch to this issue for other people to use, but none of these require changing the issue status.
Comment #190
quietone CreditAttribution: quietone at PreviousNext commentedPublished change record