Ubercart

I think it would be very good to get Ubercart working with PostgreSQL!

I'm not the expert you're looking for, but I think the first thing to do is to inspect and fix all the .install files for all the Ubercart modules you have under sites/all/modules/ubercart. This is where the database tables are created and dropped when the modules are installed or uninstalled.

After a quick look, some look fine to me, but some have invalid PostgreSQL syntax and some have just left the case 'pgsql': sections blank - no way they're going to work with PostgreSQL! I'm guessing you're the first person to try it.

After fixing the .install files, I'm sure there are also SQL statements embedded in the .module code that need to be cleaned up.

Yes, KEY is MySQL for CREATE INDEX. No, that db_query statement is not MySQL specific, but is the format Drupal uses - tables names are enclosed in {}, the SQL is parsed and the {} are removed before sending the statement to the database. A good place to look for examples is in the core Drupal modules, which are known to work with PostgreSQL - see the .install files under the modules directory in your Drupal install directory.

Aye, we don't test or use Postgres in house, so we don't have a good way to make our code compatible. We've accepted working patches for this in the past. Hopefully you'll just need to change the .install files for the pgsql cases... let me know if some particular query in the code breaks a page besides those.

Sorry we're not much more help on this one.

Yeah, I'm cool with taking this on since I want to run it with PostgreSQL.

I've done plenty of Drupal and my main question regarding the "sequences" part was why are you using that convention it's not in my Drupal install and I can't find where it's created.

If they are trying to use "sequences" built in to the DB like "auto increment" or for pgsql "serial" then it's a different format.

Ryan,

I'll lead this one but I hope someone can answer any questions I post in a reasonable time frame. I hate to lose momentum once digging into these things

Can someone give me the fast track for submitting the patches?

The easiest thing will be to use the code from the Bazaar repository and generate the patches with bzr diff. If you want to submit a patch for a particular module, go to the ubercart directory and do
bzr diff uc_product > product_patch.diff
Then just post the file in this thread and we'll get it committed.

Regarding sequences, that's actually a DB table created by Drupal core to handle any sort of incremental sequence. As TR mentioned, the braces there always go around table names in Drupal, b/c Drupal parses queries and does automatic DB prefixing based on the brackets. So, it's not DB specific. The line you mentioned just deletes the row from the sequences table pertaining to the order zone ID sequence.

Ryan,

I'm very clear on what those are used for in Drupal core.

In regards to the "sequences" table this must be a larger bug because I don't have that table and I'm running Drupal 5.2.

Cheers,
-p

May be that they have another solution for Postgres. Built-in support perhaps?

Ryan,

I get the following error:

Ryan,

You are correct it is in the Drupal core but only for MySQL.

/modules/system/system.install:

I think UbertCart needs to correct it's code in order to support the proper use.

Aye, agreed. It's all a part of us never testing w/ Postgres... things like this would be taken care of w/ a switch, but these problems will also disappear once we upgrade to Drupal 6.

EDIT: I'm also not sure what that Bazaar error is from... haven't seen that before.

The sequences table *should* be totally hidden by Drupal's db abstraction layer. It is used only in the MySQL implementation of the db_next_id() function (see below):

The PostgreSQL implementation of this function looks like this:

I would think that user code like Ubercart would only need to deal with the abstraction layer, not the implementation tables. So I guess the question is why does Ubercart manipulate the sequences table in the first place, and can that be replaced by db neutral code? Hmm, I think I can answer my own question, at least for uc_orders.module - it looks like a sequence id is used for order number, and needs to be set to a starting value so as not to overwrite previous order numbers...

Okay, I have all the install files from the Ubercart directory fixed excluding the sequence issues.

Once I can checkout the files system I will provide the patch files. Until then I may just use the diff function built into SVN to get the info.

Let me know if you have any other ideas.

-p

@TR: The sequences table is accessed directly for setting values, as you noticed. It's only done in select cases... namely in the importer, in update functions, and in uninstall functions. Since there is no function to manually set a sequence ID or remove it altogether (in the case of an uninstall), we've just added the queries in ourselves. I don't see any reason to remove these queries from update functions, since it will only matter for people using MySQL anyways... no one could be updating from a previous version w/ Postgres since it didn't work before. For deletes, it doesn't really matter if the query fails, but I suppose to be forward thinking the appropriate query for Postgres should get executed at uninstall. I don't know what to think about the importer module, but I'll have Lyle take a look at this issue and see what he thinks.

You don't want unnecessary error messages, though. Fixing the uninstall functions would be a good idea, I think.

The importer uses the sequence table because db_next_id() actually increments the id value, and that's not good when we want to insert a new node or taxonomy term. I guess the same thing could be done with a SELECT MAX(nid) query, but I thought at the time the sequences table was used by everyone as a separate solution to the auto-increment problem.

Yes, I agree that the use of the sequences table in the .install files under the mysql case block is appropriate and doesn't need to be changed. The code using sequences in uc_store.install and uc_product.install does need to be moved into the mysql case block, however. We're at my limit of PostgreSQL knowledge here - last time I used PostgreSQL was when it was still a Berkeley project - but I think the PostgreSQL replacement code for uc_product.install would look something like this:

MySQL:

PostgreSQL:

Plus a similar mod for uc_store.install. Instead of:

use:

and instead of:

becomes:

(BTW, uc_product.install isn't currently deleting the sequence upon uninstall - that should be fixed!)

The many uses of db_next_id() in the Ubercart code would then remain unchanged.

There are also similar uses in uc_credit.install and uc_payment.install that need to be fixed. The only thing left that I see would be uc_importer.module, and from Lyle's post it sounds like he has an idea of an alternative way to do it...

What do you think, patkins? I don't have a way to test these changes with PostgreSQL...

Simply subscribing ... in need of a successful installation with Postgres as well.

The class field sequence is supposed to be removed in uc_product_update_4(), when those tables are dropped in favor of different node types for product classes, and not in the uninstall function. Anybody who installed the product module before that was done will have that extra row in the sequences table, but it's not a big deal, I think. For professionalism's sake, we can put the query in the next schema update.

I'll be glad when Drupal 6 does all this thinking for me.

I still get this error and upon a google search it seems that others had this issues as well.

https://bugs.launchpad.net/bzr/+bug/39542

Is there another branch to try? Or another protocal like ssh or sftp.

-p

That bug report's from last year, and the last comment says it was fixed in version .11. I don't know what version Bazaar is up to now, but we started using version .14 with Ãœbercart.

You might try the other Bazaar commands like get or pull. Those still ought to work over HTTP. That might be better for everyone anyway, since no one else really needs the full revision history.

And if all else fails, just point your browser to http://bazaar.ubercart.org/ubercart/alpha/ubercart. The working tree there gets updated every hour, so it's always a mirror of the repository.

I looked in this queue just to check if postgresql was supported, and 'lo and behold it isn't, as I'm very well used to. This looks like a really promising framework and I'm looking forward to a time (soon, hopefully!) when I can try it out.

I sent Ryan an email with the mods. There are some other changes that I will complete by next monday.

I hope we see them in the source soon.

-p

How are we looking on these patches?

-p

Honestly, I thought they were included in this update. Can you check the beta code to verify?

My angle into this is from the drupal.org site where I started this issue:

http://drupal.org/node/208443

Are patkins' changes in the beta1?

Have the .install patches been merged yet? I started working on such a patch today until I found this thread... based on the SQL errors I received during installation of beta 4 I assume this has not yet happened.

If patkins's patch for the install queries can be applied to the development branch, I am willing to take a stab at auditing all the queries for postgresql compatibility. I realize this is a substantial bit of work but I would very much like to run ubercart on postgresql.

Unfortunately, I'm not entirely sure if the patch made it in or not. I was under the impression that it had, but I didn't get solid confirmation through this issue. I guess it can just be compared against the latest release.

We'd love to have help getting the cart up to snuff for Postgres, so just let us know what needs to change/post up a patch and we'll get it in.

Attached is a patch that should make Ubercart work with PostgreSQL. The patch is against a checkout of revision 811.

Most of the changes were simple: fixing typos in the pgsql installation queries, removing `ticks` around column names, and things like that. Some of the queries required more substantial changes. In these cases I added conditional logic to define the query based on the database type (mysql[i] or pgsql). This way the more-tested support for mysql should not be changed at all because of this patch.

I tested most of the functionality of most of the modules, with some exceptions. I did not test the 2Checkout, Cybersource, Importer, and Repeater modules at all, though based on a code review they should not have problems. I only performed limited testing on the Shipping and Shipping Quote modules.

If this patch can be applied for an upcoming release, I would love to hear feedback from anybody who can test ubercart with postgres. If you come across any errors please post them here (or is there a better place?), and I will be happy to fix them.

You are a hero. I'll get someone to apply this in the morning.

I applied the patch to the livetest, and it was mostly successful. There was a hunk that failed around line 232 of uc_stock.module, but it was easy to fix manually.

I was looking through the patch and noticed a change I don't understand. Line 343 of the patch file, or the first change of uc_cart.install. I'm pretty sure SQL doesn't like commas after the final parameter of a function. Maybe you forgot to add the PRIMARY KEY after the "data text"?

I would like for you to reroll the patch because it introduces a lot of leading tabs to create whitespace. This goes against the Drupal coding standards, which we're trying to uphold. I just made a couple of commits to fix the uc_file and uc_stock modules. Set your favorite text editor to emulate tabs with two spaces, and it'll reduce the size of your patch by getting rid of superfluous lines.

Thanks.

I didn't notice the error in uc_stock.mocule:232, but I'm sure whatever your fix was is fine.

You are right about the extra comma in uc_cart.install; I removed it in this new patch. I mistakenly had a primary key there at first, and when I removed it the comma stayed behind.

I've gone through my changes and used the 2 spaces setting instead of tabs everywhere. I'm not sure if the uc_file and uc_stock files might still look bad, because the copies I worked off had tabs; I left these except for the lines I edited, where I replaced them with the spaces. I'm not sure how that will look after merging with your changes to those files.

I ran vim's retab command (changes tabs to spaces) on the files I edited; the only place this seems to have introduced changes (other than on lines having to do with the pgsql fixes) is uc_reports.module.

Let me know if anything else needs to change.

I tried applying it to the latest version, and several parts of it failed. I figured there had been too many changes, including the removal of the leading tabs, so I tried applying the previous version to revision 811. That didn't work either. I'm sorry, but I think I have to ask you to reroll the patch against the latest version.

This new patch is against revision 832. I tested the patch against a fresh checkout and then deployed it and things seem to be fine, so hopefully this one will work out.

Patched and committed. Revision 834 is ready to roll.

I tested an install on Postgres yesterday, and it mostly worked. The only error I got was from the use of CONCAT_WS() in uc_catalog.install. It turns out that the only string concatenation that Postgres has is the operator ||. That's an easy fix since it's only in the .install file once, so once that gets committed (in the next 5 minutes), we should have a working Postgres implementation. Right up until we make a mistake with a new query.

I'm glad to hear that we are close if not done with the current version. I hope my first run through of the modules helped get the ball rolling.

I'll download and try again this weekend.

-p

There's something wrong with the $sql_count query in theme_uc_catalog_browse. If I run it in MySQL, it gives me a row for each product, which is not the purpose of it. It should return one row and one column: the count of number of products in the catalog category. I remember hearing that there was some reason we couldn't do COUNT(DISTINCT(n.nid)) like the MySQL query, but I don't remember why not. Need some input on this.