Hey all,

I've finally had a chance to port the install system to PostgreSQL. I've attached the files to this post. Of particular note:

  • Added unsigned integer types for smallint, int and bigint types.
  • Modified indexes to match the MySQL KEY statements where substrings are used in the index.
  • Everything(™) that was unsigned in MySQL is now unsigned in PostgreSQL

Hopefully we get some people testing this baby out so we can see how it goes.

Cheers,

--
Sammy Spets
Synerger Pty Ltd
http://www.synerger.com

CommentFileSizeAuthor
#21 system.install.patch_2.txt7.5 KBnuwanda
#19 system.install.patch_1.txt8.17 KBnuwanda
#10 module.install.1.patch.txt17.3 KBsammys
#5 install.pgsql_1.inc5.54 KBsammys
#3 install.pgsql_0.inc5.54 KBsammys
#2 module.install.patch.txt29.38 KBsammys
#1 system.install.pgsql.4.8.patch.txt15.05 KBsammys
install.pgsql.inc5.5 KBsammys
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sammys’s picture

sammys CreditAttribution: sammys commented
FileSize
system.install.pgsql.4.8.patch.txt15.05 KB

Here is the patch for system.install.

sammys’s picture

sammys CreditAttribution: sammys commented
FileSize
module.install.patch.txt29.38 KB

Ok.. i've rolled up all the [module].install patches into the one patch. Now, all core modules that have install files have been updated with PostgreSQL installation code.

sammys’s picture

sammys CreditAttribution: sammys commented
Status: Active » Needs review
FileSize
install.pgsql_0.inc5.54 KB

Found a remaining mysql_error() call in the install.pgsql.inc file. Now this file and the patch are ready for review.

breyten’s picture

breyten CreditAttribution: breyten commented

The doxygen for drupal_test_pgsql still says MySQL ;-)

sammys’s picture

sammys CreditAttribution: sammys commented
FileSize
install.pgsql_1.inc5.54 KB

hehe. nicely spotted. updated.

Dries’s picture

Dries CreditAttribution: Dries commented

I lost track. What patches should I apply? :)

sammys’s picture

sammys CreditAttribution: sammys commented

heh... apply module.install.patch and put install.pgsql_1.inc into the includes directory as install.pgsql.inc. Voila!

Steve Simms’s picture

Steve Simms CreditAttribution: Steve Simms commented

There are a few odd constructions with sequences for a few tables, but this existed in the old code, so I filed a separate issue to deal with that. I'll submit a patch once this one gets committed.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks sammys. Keep 'em coming. :)

sammys’s picture

sammys CreditAttribution: sammys commented
Status: Fixed » Reviewed & tested by the community
FileSize
module.install.1.patch.txt17.3 KB

Changed my mind about the type naming. Making the 'u', for unsigned, a suffix so compatibility code in modules like CCK and core database code can easily use the unsigned types. So, rather than having a smalluint there is a smallint_unsigned. In addition to this the pgsql version of the node_type table has been added to the system.install file. Here's the patch.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Version: » x.y.z
Category: task » bug
Status: Closed (fixed) » Active

there's no update to migrate a 4.7 pgsql DB to this new system. in particular, the stuff to define the int_unsigned type only happens when you first install the system DB. if you're updating an old install, then the int_unsigned type is unavailable to you. this is particularly problematic for update 1005, which is trying (incorrectly, see http://drupal.org/node/82822) to add a {node_type} table with some unsigned fields. ideally, the fix for #82822 would be able to just convert those "integer unsigned" (mysql syntax) into "int_unsigned" (the new drupal-specific pgsql syntax). however, without a proper system update to add these types to the DB, you can't do that. cramming them in via update 1005 seems wrong. however, it seems even worse to fix 1005 to use plain old integer, and then add a new update 1012 that both defines int_unsigned (if not already there from the system install) and then converts the 3 fields in {node_type} to use int_unsigned.

sammys’s picture

sammys CreditAttribution: sammys commented
Status: Active » Closed (fixed)

This issue is closed. See http://drupal.org/node/82822 instead.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Closed (fixed) » Active

i don't want to get into a pushing match, but no, this isn't closed. ;) there are 48 references to "int_unsigned" in system.install. none of them in an update. if someone takes a 4.7 pgsql DB and wants to update to 5.0, their version of all of these fields are going to be signed ints, not unsigned like we're expecting. i believe we really need a new system update that works as an upgrade path to alter all of these fields. it should probably be the one that defines the new int_unsigned types, too. this is a separate (though related) issue to how we fix update 1005's incorrect use of "unsigned" via http://drupal.org/node/82822.

does that make sense?
thanks,
-derek

sammys’s picture

sammys CreditAttribution: sammys commented

Fair enough then. Semantics. :)

I understand your concern for consistency across both upgraded platforms and the new install base. The more I think about it, the more I feel it's a necessary thing to do. There are some reasons why it doesn't make any difference.

  • Drupal doesn't require use of the unsigned types. It's simply a safety net.
  • Future updates can use the new types if desired

Alternatives:

  1. Switch all fields requiring unsigned types over to the new *int_unsigned types
  2. Add check constraints to all fields requiring unsigned types
  3. Do nothing

Any comments?

Cheers,

--
Sammy Spets
Synerger
http://synerger.com

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Title: 4.8+ install system code for PostgreSQL » 5.0.0 install system code for PostgreSQL

(at chx's request)

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Title: 5.0.0 install system code for PostgreSQL » 5.0 install system code for PostgreSQL
Version: x.y.z » 5.x-dev

bump: this is still broken, right? it'd be nice to fix this before 5.0 RC2, wouldn't it? i'm not nearly the pgsql guru as others, but i think i'd vote for option #1 from comment #16. sammys, any reason *not* to take that approach? seems simpler than #2, and for all the reasons already discussed, better than #3.

nuwanda’s picture

nuwanda CreditAttribution: nuwanda commented
Status: Active » Needs review
FileSize
system.install.patch_1.txt8.17 KB

Hi there,

am a Newbie to Drupal, but I think this patch, extending system_update_1005, will bring update and install back in sync, as far as the *int_unsigned types are concerned. Ran the patch against DRUPAL-5. Tested with upgrades from 4.7.3 to 5.2 on postgres 8.1 and 7.4.

Cheers

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Needs work

The first chunk no longer applies since that was fixed separately.

nuwanda’s picture

nuwanda CreditAttribution: nuwanda commented
Assigned: sammys » nuwanda
Status: Needs work » Needs review
FileSize
system.install.patch_2.txt7.5 KB

So it was. Thanks for spotting that drumm!

sammys’s picture

sammys CreditAttribution: sammys commented

Hi,

Thanks for taking ownership of this nuwanda. Hopefully you're still around. I've taken a look at your patch and most of it looks good. Without applying the patch and testing it I can see there are missing {} around table names in a few of the direct db_query() calls. Could you fix that and reroll the patch please?

StevenPatz’s picture

StevenPatz
he/him
Primary language English
Location Alexandria VA
CreditAttribution: StevenPatz commented
Status: Needs review » Needs work
TR’s picture

TR CreditAttribution: TR commented
Status: Needs work » Closed (won't fix)

The part of this issue that is still open has to do with the upgrade path from Drupal 4 to Drupal 5. Neither of those major versions are supported anymore. Closing this as "won't fix".