Add PostgreSQL support

[bernd-wechner]

I have started the research on thus and drafted a well annotated version of the plugin:

owa_db_postgresql.php.txt

I seek some feedback on the notes therein, have set up a PHP dev environment (Exclipse, PHP Development Tools and xdebug - am curious what others use but I'm invested in Eclipse for other projects so ...), and can start trying it out some time soon once it stabilises. Among other things of course support for $db_type == postgresql needs to appear in the code base and I would welcome tips on where all the touch points for that are (if more than one, hopefully only one) - dbFactory() seems robust but the owa_db_type select box on the installer needs win an option somehow and I've not found where that's built yet.

[padams]

Yeah db_type is hard coded into the config file. I'll make a ticket now to change that.

[padams]

Ok it seems that Postgresql doesn't allow indexes to be added during a table create. So, it looks like createTable method needs to be overridden by the db_plugin for sure.

[padams]

getDefinition should definitely not have that hard coded INDEX statement. I'll move that to the DB plugin where it belongs.

[padams]

I believe the auto_increment definition is deprecated replaced with OWA_DTD_SERIAL

[padams]

OWA_DTD_UNIQUE is not used and should probably be removed.

[bernd-wechner]

Thanks for the review and small patches. There were a couple of constants I find used anywhere in the code base. if createTable moves top the plugin file that would certainly make the whole thing more flexible in future for other databases to be added. Lightdb is really popular for example for it's simplicity (a single data file). Not that I use it would write a plugin for it (I use postgresql on my webserver so I'm keen to use that for OWA if I put on the same server). Should be hard to complete it.

Would love a pointer on where the drop down for database format selection is populated in the installer. I'm a tad short on tme, and haven't delved too deeply, but could see it obviously as it's a rather heavy PHP implementation (most of the PHP I've worked with was in the days where it was mostly inserted into HTML files not a solid programming language which it's become).

[bernd-wechner]

I believe the auto_increment definition is deprecated replaced with OWA_DTD_SERIAL

It would be great confirm this as ti removes one hurdle from implementing new db support. The constant OWA_DTD_AUTO_INCREMENT could be removed. Currently an open question mark in Postgresql implementation.

[bernd-wechner]

I could of course make all the changes above and related and put it in a PR? I forked a branch on my forked repo called postgresql.

[padams]

Yes please. That way i can comment on any specific code.

[bernd-wechner]

Cool. Will work on it in a Postgresql branch.

A heads up that INDEXing has to change a little. Primarily because Postgresql doesn't support ALTER TABLE to create and add indexes, both MySQL and Postresql support CREATE INDEX, but MYSQL supports implicit index naming and Postgreql doesn't etc. I'm thinking about minimal impact changes to that now.

[bernd-wechner]

Piece of advice needed. Because explicit index names are not currently used in your code base and because implicit index names are not supported by SQL the best approach I see is to drop OWA_SQL_ADD_INDEX altogether (it's MySQL only) and use OWA_SQL_CREATE_INDEX which is consistent between the two (but maybe not for future databases mind you).

OWA_SQL_CREATE_INDEX however requires an explcit index name, and it could easily be standardised to something like idx_{$table_name}_{$column_name} and work. The problem is that this won't work well with sprintf() in PHP but can work well with strstr() whch you only use very sparingly the code base currently and not with a hash array at all. But for example:

    define('OWA_SQL_CREATE_INDEX', 'CREATE INDEX {index_name} ON {table_name} ({icolumn_name})');
    define('OWA_SQL_DROP_INDEX', 'DROP INDEX {index_name}');

    function addIndex($table_name, $column_name) {
        $index_name = sprintf('idx_%s_%s', $table_name, $column_name);
        return $this->query(strstr(OWA_SQL_ADD_INDEX,     
                                       ['{index_name}' => $index_name,
                                        '{table_name}' => $table_name,
                                        '{column_name}' => $column_name]);
    }

    function dropIndex($table_name, $column_name) {
        $index_name = sprintf('idx_%s_%s', $table_name, $column_name);
        return $this->query(strstr(OWA_SQL_DROP_INDEX, ['{index_name}'=>$index_name]);
    }

Would work fine without requiring any code changes to current users of addIndex and dropIndex in the code base (i.e. concentrates the change in owa_db.php and the (soon to be 2) database plugin files (where the format string changes).

This could form part of a later generalisation of all those format strings to strstr, named field use rather than sprintf ordered field use allowing greater flexibility for future database support.

[bernd-wechner]

If I don't hear back soon I may just implement it. I'd consider a clean sweep converting all the constants to named field format. Seems much more sensible to me and easier to adapt to other databases in future. What I'm asking before doing that is if PHP experience beyond mine (which is long but modest) sees a problem with this approach I don't.

[padams]

Agreed but maybe just do it for these index methods to start and we can do the rest later. Less potential for bugs with something new like this.

Also, we still have to modify the owa_db::createTable to loop through the columns looking for the index flag and then add the indexes for those columns via separate queries after the table is created yes?

[dkegle]

Has there been any progress on this issue in recent weeks?

[bernd-wechner]

I have this almost working now. A significant PR: #634

paves the way for it (tidying up coding issues that were not apparent when one database type could be assumed - namely accessing the data type definitions before the database type is defined in the install sequence). A proposed fix sits in that PR.

I further code changes implemented in a postgresql branch (not pushed yet) that create the database quite successfully.

I'm presently stuck on mapping MySQL BLOB to postgresql. I've tried BYTEA but alas when the serialized configuration is written it is not read back intact with the existing code. Meaning pg BYTEA behaves differently to ms BLOB. Researching this at present.

I can't see how the current code base was working with MySQL even mind you as autoincrementing columns were not supported at all. OWA only uses one on the user table, but still it was falling over in postgresql because the INSERT was built including the autoincrement column! Which postgresql barfs on and I have to assume if it was working on MySQL that MySQL just ignores it politely.

[bernd-wechner]

It is tentatively fully functional with a mapping of ms BLOB to ps TEXT.

Pending some research into all used contexts. These are the only uses of OWA_DTD_BLOB in the code base:

• configuration.settings
• site.settings
• session.latest_attributions
• queue_item.event
• domstream.events

The first is fully functional with a mapping to TEXT. The rest to be tested.

[bernd-wechner]

There is now a functioning installable branch that supports Postgresql here:

https://github.com/bernd-wechner/Open-Web-Analytics/tree/Postgresql

I will make this live on a site I manage for testing in situ. I encourage you to consider same. Once considered stable enough I'll submit a PR back to upstream master.

[jsundquist]

@bernd-wechner I'm testing out your branch with a new installation and noticed that the dropdown to select which database type only includes mysql. How do I ensure I can install it against a postgresql database instead?

[padams]

I believe the change for that is in master. #599

[bernd-wechner]

@bernd-wechner I'm testing out your branch with a new installation and noticed that the dropdown to select which database type only includes mysql. How do I ensure I can install it against a postgresql database instead?

Are you sure you cloned the postgresql branch?

here's the file that contains the config I believe that puts the options onto the drop down:

https://github.com/bernd-wechner/Open-Web-Analytics/blob/Postgresql/modules/base/classes/settings.php

Search in that for: db_supported_types
Line 728.

The same file in the master branch:

https://github.com/bernd-wechner/Open-Web-Analytics/blob/master/modules/base/classes/settings.php

lacks that config. I've only been working on the postgresql branch.

Haven't got around to trying it in production yet myself. Distracted with some server changes and a desktop upgrade for now.

[er1c]

One additional motivation for postgres support is using CockroachDB: https://www.cockroachlabs.com/. I'd be interested knowing how the performance works out.

[bernd-wechner]

I've dropped the ball on this in the past year. But would still like to see it progressed. I've done the ground work and just need to get around to setting up a test bed and testing MySQL and Postgresql instances.

[er1c]

@bernd-wechner I don't have any skin in the game, since I've never used anything other than Google Analytics, but would be happy to do a pair programming session if you are interested to push this forward.