Bug 7365

I created a Wiki page to comment on the ongoing work to make Koha run happily with PostgreSQL: http://wiki.koha-community.org/wiki/PostgreSQL

Created attachment 6852 [details]
A list of mysqlisms in C4

An (incomplete) list of MySQLisms in C4.

You have a perplexing change in the patch:
+        $isbn = $isbn->as_isbn10 if $isbn->tHOW COLUMNS FROMype eq 'ISBN13';

Perhaps a typo? Please submit a revised patch, obsolete the existing patch, and change the status to "Needs signoff."

(In reply to comment #5)
> Created attachment 8428 [details] [review]
> Add DBQ, a small module to generate DB specific SQL code
> 
> To eliminate database specific SQL idoms (aka MySQLisms) it is necessary to
> either use a standard SQL form like 'CAST(now() as date)' instead of the
> non-standard MySQLism 'CURDATE()', or to generate a database specific idiom
> where no such standard form exists (e.g. to get the list of column names
> from a table).
> 
> The new DBQ module is a generator for DB specific SQL idioms and comes with
> an implementatio for both MySQL and PostgreSQL.  Generating the SQL idioms
> this way is the least invasive form of getting DB independence, as no large
> changes to the database layer are required.  Please note that DBQ->dbq
> returns a singleton.
> 
> DBQ stands for Database Query.  Name, usage patterns, and implementation
> discussed and agreed upon with many during the Marseille Hackfest 2012.

Why don't we just use one of the existing database abstraction layers available for Perl? Then someone could use a completely different database, if they so desired.

(In reply to comment #7)
> Why don't we just use one of the existing database abstraction layers
> available for Perl? Then someone could use a completely different database,
> if they so desired.

I share this concern. I did a lot of work on the SQL ages ago and still feel that much of it should be replaced by a database-agnostic abstraction layer (if only I had the funding to do it). I don't feel that creating a Koha-specific multi-database SQL generator is a sustainable approach.

Wwe are aiming at a minimally invasive solution that does not
require us to make obstrusive changes.  And this solution does not mean
to go back to the minimal common available SQL subset, but still be able
to use the full power of the underlying DB.  And it does not prevent us
from changing to a whole other approach in the future.  For now, this is
the bare minimum that is neede to enable Koha to run on different DBs.

Switching do a different DB layer right now does help, because even if we did, Koha still expects MySQL result sets, C4::DBQ takes care of this.

Signed off the diff.

(In reply to comment #10)
> Signed off the diff.

Huh? Are we allowed to sign off our own enhancements now?

Also, has this approach been discussed on koha-devel?
I remember discussion of DBIx::Class in the past and the related RFC http://wiki.koha-community.org/wiki/RFC_for_using_DBIx_Class_in_Koha but I didn't find similar discussion of C4::DBQ.

(In reply to comment #11)
> (In reply to comment #10)
> > Signed off the diff.
> 
> Huh? Are we allowed to sign off our own enhancements now?
no, but the patch was made by me

Okay, so this patch uses a bolt-on approach.  Three new modules are added (along with a unit test!), along with a new Perl dependency, and then C4/ImportExportFramework.pm is modified to use those modules instead of direct MySQL "SHOW COLUMNS FROM".

It seems like it'd be easier to use $dbh->column_info(undef, undef, $table, '%') or some modification thereof to get this information.  We'd then be using a standardized, packaged database abstraction layer, instead of having to make our own, and it'd give us more than just MySQL and Postgres.  The resulting change would likely be fewer lines of code than what we've got here currently.

Moving to In Discussion, as we've likely still got a lot to talk about in terms of an implementation plan for adding database agnosticity.

SHOW COLUMNS FROM is only one example of a MySQLism, that we merely used to demonstrate the technique being used.  What we - and we do it on purpose - is to not change the way how the database is accessed, but the SQL being used.  For SHOW COLUMNS FROM there is a function in DBH, but did you check what it returns what the surrounding code in Koha expects?

Well, we did check that and our goals was to minimally invasive, that is why we only change the way the SQL is generated.

How will you address idoms for which no DBH equivalent, no standard SQL equivalent exists, i.e. where different SQL HAS to be used for MySQL or PostgreSQL?  Our solution adresses exactly this problem.  It is small, overseeable, and works.

Created attachment 8534 [details] [review]
proposed patch (updated)

as suggested by Ian Walls the patch replace "SHOW COLMUNS" in ImportExportFramework.pm with $dbh->column_info(undef, undef, $table, "%");
now DBQ implement 2 subs : 
- dateSub (Subtract a time value (interval) from a date)
- ifNull (Returns $a if not null, else return $b)

they are use in sub GetLateOrders in C4/Acquisition.pm .

If db_scheme is mysql, the $query created by trhis sub is :
    SELECT aqbasket.basketno,
        aqorders.ordernumber,
        DATE(aqbasket.closedate)  AS orderdate,
        aqorders.rrp              AS unitpricesupplier,
        aqorders.ecost            AS unitpricelib,
        aqorders.claims_count     AS claims_count,
        aqorders.claimed_date     AS claimed_date,
        aqbudgets.budget_name     AS budget,
        borrowers.branchcode      AS branch,
        aqbooksellers.name        AS supplier,
        aqbooksellers.id          AS supplierid,
        biblio.author, biblio.title,
        biblioitems.publishercode AS publisher,
        biblioitems.publicationyear,
    
    aqorders.quantity - IFNULL(aqorders.quantityreceived, 0) AS quantity,
    (aqorders.quantity - IFNULL(aqorders.quantityreceived, 0)) * aqorders.rrp AS subtotal,
    (CAST(now() AS date) - closedate) AS latesince
     
    FROM
        aqorders LEFT JOIN biblio     ON biblio.biblionumber         = aqorders.biblionumber
        LEFT JOIN biblioitems         ON biblioitems.biblionumber    = biblio.biblionumber
        LEFT JOIN aqbudgets           ON aqorders.budget_id          = aqbudgets.budget_id,
        aqbasket LEFT JOIN borrowers  ON aqbasket.authorisedby       = borrowers.borrowernumber
        LEFT JOIN aqbooksellers       ON aqbasket.booksellerid       = aqbooksellers.id
        WHERE aqorders.basketno = aqbasket.basketno
        AND ( datereceived = ''
            OR datereceived IS NULL
            OR aqorders.quantityreceived < aqorders.quantity
        )
        AND (aqorders.datecancellationprinted IS NULL OR aqorders.datecancellationprinted='0000-00-00')
     AND (closedate <= DATE_SUB(CAST(now() AS date), INTERVAL ? DAY))  AND aqbasket.booksellerid = ?  AND borrowers.branchcode LIKE ?  
    HAVING quantity          <> 0
        AND unitpricesupplier <> 0
        AND unitpricelib      <> 0
    
ORDER BY latesince, basketno, borrowers.branchcode, supplier


If db_scheme is pg, the $query created by trhis sub is :
SELECT aqbasket.basketno,
        aqorders.ordernumber,
        DATE(aqbasket.closedate)  AS orderdate,
        aqorders.rrp              AS unitpricesupplier,
        aqorders.ecost            AS unitpricelib,
        aqorders.claims_count     AS claims_count,
        aqorders.claimed_date     AS claimed_date,
        aqbudgets.budget_name     AS budget,
        borrowers.branchcode      AS branch,
        aqbooksellers.name        AS supplier,
        aqbooksellers.id          AS supplierid,
        biblio.author, biblio.title,
        biblioitems.publishercode AS publisher,
        biblioitems.publicationyear,
    
    aqorders.quantity - COALESCE(aqorders.quantityreceived, 0) AS quantity,
    (aqorders.quantity - COALESCE(aqorders.quantityreceived, 0)) * aqorders.rrp AS subtotal,
    (CAST(now() AS date) - closedate) AS latesince
     
    FROM
        aqorders LEFT JOIN biblio     ON biblio.biblionumber         = aqorders.biblionumber
        LEFT JOIN biblioitems         ON biblioitems.biblionumber    = biblio.biblionumber
        LEFT JOIN aqbudgets           ON aqorders.budget_id          = aqbudgets.budget_id,
        aqbasket LEFT JOIN borrowers  ON aqbasket.authorisedby       = borrowers.borrowernumber
        LEFT JOIN aqbooksellers       ON aqbasket.booksellerid       = aqbooksellers.id
        WHERE aqorders.basketno = aqbasket.basketno
        AND ( datereceived = ''
            OR datereceived IS NULL
            OR aqorders.quantityreceived < aqorders.quantity
        )
        AND (aqorders.datecancellationprinted IS NULL OR aqorders.datecancellationprinted='0000-00-00')
     AND (closedate <= CAST(now() AS date) - interval '? DAY')  AND aqbasket.booksellerid = ?  AND borrowers.branchcode LIKE ?  
    HAVING quantity          <> 0
        AND unitpricesupplier <> 0
        AND unitpricelib      <> 0
    
ORDER BY latesince, basketno, borrowers.branchcode, supplier

IIRC, the QAM has the prerogative to put bugs "in discussion." This is one which should be discussed regardless of the "my plan works fine" declaration. Database agnosticism is bigger than just Pg. Furthermore, the last time an entity undertook to quickly push "custom" database agnostic work we got stuck with C4:SQLHelper which has its own set of problems in spite of the declaration that it worked fine.

Moving back to "In Discussion"

 * For hash refs, use $myhash->{mykey}, not $$myhash{mykey}


From http://wiki.koha-community.org/wiki/Coding_Guidelines

Since we are making people resubmit patches for mysqlism, then this should be resubmitted for style violations

After http://article.gmane.org/gmane.education.libraries.koha.devel/7566 and reviewing the patch, I think the current patch also fails coding guideline SQL9 because it will result in ways to avoid the placeholder system and inject SQL, particularly in date fields.

The proposed patch does not introduce new vulnerabilities or possibilities for SQL injection and you can not prevent the placeholder system.  It does a mere text replacement, so when you feed '?' to one of the (date) functions, the resulting SQL code will contain the '?' at the right place and $dbh->prepare() will do it's job like before.

The right flow of things is as follows:

$sql = $dbq->dateDiff('?', '?')
$qth = $dbh->prepare($sql)
$res = $qth->execute()

Bugzilla has done some great work on cross-database compatibility, and their guide can be referenced here: http://www.bugzilla.org/docs/developer.html#sql-crossdb

Well, bug 7785 is listed as blocking this. I have provided a patch which removes the MySQLism from C4::Members->column. Though, if there is a RDBMS specific LIMIT-like clause, the function will still need modifications for optimization purposes, not out of necessity.

GPML...

Given the new use of DBIx::Class in Koha, I'm taking this opportunity to change the title of the bug to a positive sentence: Koha should support PostgreSQL.

Hi all,

I'm evaluating the use of Koha, but our platform uses PostgreSQL.

I read your very interesting discussion and I'm interested to know the current PostgreSQL support status.

Seems like this patch is not included in the main branch yet, right? In that case, hope Koha supports PostgreSQL officially soon.

If Koha don't support PostgreSQL yet, do you know if there are a "mantained" patch? I mean, I want know if Stephane or somebody else is updating an unofficial patch to support PostgreSQL on each new release of Koha. If so, is the patch used on  production installations?

Thanks in advance.

Best Regards,
Leandro

"InnoDB allows a foreign key constraint to reference a non-unique key. This is an InnoDB extension to standard SQL."

This is possibly also worth noting. I'm not sure how PostgeSQL handles foreign key constraints on non-unique keys, but I know we have at least two of these in kohastructure.sql at the moment.

Hi,

I would like to use postgres/Oracle with koha, and be willing to implement it. Has anybody started this - and/or would be willing to give me hints on the structure needed

cheers 
Mark

(In reply to Mark Hofstetter from comment #25)
> Hi,
> 
> I would like to use postgres/Oracle with koha, and be willing to implement
> it. Has anybody started this - and/or would be willing to give me hints on
> the structure needed
> 
> cheers 
> Mark

I think BibLibre did work on this in the past, but I can't imagine this work ever being successful, since there are many MySQLisms in Koha.

While Koha does use an ORM in many places now, there are still queries written by hand, and the database creation scripts all use MySQL-specific syntax. Plus there is that old comment of mine about the MySQL extension to InnoDB. 

It would be easier to adapt one's own system to use MySQL than to adapt Koha to use a different database, I reckon.