Make the postgresql driver independant of the schema

There's already an issue for this, which asks to put this functionality into the schema itself. #301038: Add a cross-compatible database schema introspection API IMO that's where it belongs, not in the connection object.

Working to get that issue taken care of, though, would be awesome. :-)

Who's kidding? As of right now we do not support ANY non-local databases (local = where Drupal is installed) aside from MySQL and SQLite, because of the requirement of the schema for field-type checking. If we make the schema object able to detect the schema as needed, even if it requires some DB-specific logic, we have enabled remote-DB access for all supported database types. This patch works only for Postgres, and puts information about the schema in the connection object rather than the schema object. That's the wrong place for it.

The schema object should be the abstraction through which all information about a given connection's tables are accessed so that we can do whatever weirdness we need to do. Note, I'm talking about the DatabaseSchema class, not the drupal_get_schema() mega-array. IMO that mega array should be split up by table and moved behind the schema object to begin with.

Also, we care for UpdateQuery, too, not just InsertQuery.

subscribing. chx pointed me to this issue after I posted #351002: Cannot update D6 to D7 on pgsql. this patch fixes that issue and in light of that i'm marking this as critical.

Just to follow up. I gave the patch a good look and it looked okay to me but I'm not comfortable with the affected code to mark it RTBC.

Considering that #301038: Add a cross-compatible database schema introspection API has no patch at this point I'd say we should fix this and then consider a heavier solution like that.

Strictly just reading the code comments, "informations" has an extra "s".

Let's fix the typo identified by Keith.

I'd like to get another update from Crell too.

Here's a re-roll that fixed the typo.

I am still not convinced that this belongs in the connection object. Information about the DB's schema belongs in the schema object. If the schema object is too clunky right now, then let's make it less clunky.

Cross posted. Setting back to CNR for the bot, even though I still do not agree with the approach.

it'd be great to get it in even if it's imperfect because without it you're unable to run updates on HEAD.

I don't think @see tags are complete sentences and therefore shouldn't end with a period.

Tried to do a clean install after applying the patch and got the following error:

PDOException: UPDATE batch SET token=:db_update_placeholder_0, batch=:db_update_placeholder_1 WHERE (bid = :db_condition_placeholder_13) - Array ( [target] => default [return] => 2 [already_prepared] => 1 ) SQLSTATE[08P01]: <<Unknown error>>: 7 ERROR: bind message supplies 2 parameters, but prepared statement "pdo_pgsql_stmt_0203b6ac" requires 3 in batch_process() (line 2633 of /Users/amorton/Sites/dh/includes/form.inc).

something like this will help satisfy both parties - I agree with Crell, I see no reason why that extra schema stuff should be in the connection. This patch attempts to put it in Schema though this is untested and most likey won't work. but surely we can do something like this?

Just how much of a performance hit is there really to loading the schema.inc file? It's not an especially complicated class, so it should parse quickly, and loading the class itself doesn't automatically load the full schema out of the cache. (Side note: We really do need to break that up still. Bah.)

Sorry, I was under the impression that the schema.inc had already been included. I thought it was the drupal_get_schema() function calls you wanted to avoid. I still think that if its schema related then it should reside in the schema.inc. I'm still not convinced that queryTableInfomation() belongs in Database_pgsql.

I'm not sure how my suggestion loads all the table information:
$this->connection->schema()->queryTableInformation($this->table);
I assumed this did the same as $this->connection->queryTableInformation($this->table); but stored the function in the schema.

in regards to my comment above, when schema() is called is mearly initializes an instance of DatabaseSchema_pgsql which in turn assigns the connection to a variable within the instance. When schema() is called, it also assumes that schema.inc has been included. So I'm still a little confused as to how moving queryTableInformation to the schema.inc will slow things down.

ok fair enough, I'm convinced. This is Damien's patch with a few small changes and error clean-ups. am currently testing it on a PostgreSQL testbed. will post results when done

forgot to change status

also test results on postgres: Failed: 8138 passes, 5 fails, 0 exceptions <-- no different to CVS HEAD

stupid status defaults!!!

I still disagree that this belongs in the connection object rather than the schema object. At absolute minimum, if the performance difference is enough to justify it we should document why we're doing this lookup in the "wrong" place.

faulty slave, retesting

ok. Time trials, I selected the database unit tests (717 tests) and ran them through the web interface. Results:

HEAD 1/30/09: The tests finished in 14 min 34 sec.
Patch applied: The tests finished in 14 min 22 sec.
Patch Below applied:The tests finished in 14 min 45 sec. (logic was stored in schema as Crell suggested)

In this instance we are talking seconds of difference but there is no doubt that the patch above in #24 is the optimal.

Josh - I know it's annoying, but we probably need those run through again, I'm sure there's a few seconds margin of error running tests on most machines anyway. Either that or can you run ab on a database heavy page?

@catch, this was only over 717 tests out of 8000+ I'd imagine the results would amplify and become clearer if all 8000+ tests were run. Time is the issue here. wish boombatower would get those postgres testbeds running ;)

This patch seems to work to fix the problem with installation on pgsql identified by #361683: Field API initial patch. I did not review the code other than to try the patch.

#24 fixes #375264: Field API failing on PostgreSQL

Crell, can we get this into head? Cause HEAD is broken on PostgreSQL

Hm. I want to commit this, I really do, because every second that we can't install D7 on pgsql means we are likely introducing more pgsql bugs in subsequent patches since we are effectively unable to test.

However, I still can't tell if Crell's concerns were addressed. :(

Crell, are you out there? Would you support committing this even as an interim fix before we re-work schema.inc in the way that you want?

I'll try to have a look at this patch tonight US time. Sorry, the past week and a half have been totally insane. :-(

I still disagree that this information belongs in the connection object at all. Putting it there instead of in the schema object is a hack and I'm still not convinced that it's a necessary hack. However, it's a single-database-specific hack and we do need to make simpletest work in postgres. So for now RTBC, and let's try really hard to block out time to clean up schema so we can move this logic there later.

Some minor issues:

+   * We introspect the database to collect that information required by insert
+   * and update queries.

Probably "the" information required...

+   *   An object with two member variables:
+   *     * 'blob_fields' that lists all the blob fields in the table.
+   *     * 'sequences' that lists the sequences used in that table.

use - rather than * for the bulleted list.

+          $table_information->sequences[] = $matches[1];

Just a question.. why are these not keyed like the blob_fields are above?

+          $blobs[$blob_cnt] = fopen('php://memory', 'a');

We don't abbreviate variable names. $blob_count. Also consistent with the lines at the bottom of the patch.

+          ++$blob_cnt;

$blob_count++ would be the norm. Could you place a comment there to explain why we pre-increment?

Pre-increment is marginally faster than post-increment in PHP, for deep engine reasons I don't fully understand but have seen benchmarks for. In cases where either work equally well logically, pre-increment is therefore a (rather) slight performance benefit.

I don't know if intricacies of PHP engine semantics like that are worth a comment, or if they cross into the "we expect you to already know this" territory (as we do for a lot of other micro-optimizations).

Committed to HEAD. Thanks!

ah crap, the patch I posted were fixes made to the schema version. @Crell, I guess you got your wish

closing -