When installing properties_sql using a PostgreSQL database, (I haven't checked any other), the installation fails with the following error:

PDOException: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near "Array"
LINE 5: PRIMARY KEY (Array, Array)
^: CREATE TABLE {properties_category_attribute} (

When I change the primary key in the schema and remove the length constraint, it works. Is this way of defining a primary key allowed? If so, it might be a bug in the pgsql driver.

// MARIA Storage engine key length must not be longer than 1024 bytes.
'primary key'     => array(array('category_name', 100), array('attribute_name', 100)),

Comments

berdir’s picture

Hm, well, it did work on MySQL and it seems to work for all others too so far. You are probably the first to try on PostgreSQL (ar at least to *report* this ;))

It is possible that this isn't really valid (and could lead to issues when you have category/attribute names which identical in the first 100 chars.

Maybe we could simply limit those to be no more than 100 characters long (It's also possible that it can be a bit more, I haven't actually calculated the resulting lenght, that was just the first try).

xatoo’s picture

Status: Active » Closed (won't fix)

Just reporting back that this behaviour is due to a bug in the PgSQL driver and therefore needs no fix in this particular module.

#1148856: Postgres schema doesn't support keylength on a unique index

berdir’s picture

Status: Closed (won't fix) » Active

Ok, let's keep this open because it's probably not really a good idea to do this for the primary key.

xatoo’s picture

Ok, so apart from it being a bug in core we shouldn't put such constraints on the primary key. I suggest removing the constraints from the .install file and then just limiting the length of the accepted machine names in the form definitions. The maximum key length in MySQL (don't known about Maria) depends on the used character set. MySQL uses up to 3 bytes per character for UTF8. The maximum key length in MySQL is 1000 bytes, so with 500 bytes for each machine name we'll have to limit the length to about 166 characters. Would you agree?

By the way, is there a reason to use machine names as keys and to expose them to users as opposed to using artificial keys?

berdir’s picture

Yes, removing that key limitation and instead adding a restriction on the column + form is definitly better, patches are welcome :)

Instead of 166, which is a pretty arbitrary number, we could also use something like 128.

They are used as keys because of the abstraction layer that only cares about machine names. And to connect them, the easiest way was to use the machine_name as key. But might be worth thinking about improving that in a new issue, if it's possible. I remember that there were issues with that, but don't remember what exactly.

xatoo’s picture

Status: Active » Needs review
StatusFileSize
new4.5 KB

Here is a patch which limits the machine name lengths to 128 characters. Maybe someday I will try to run a test with artificial keys but to be quite honest with you I'm quite happy with it being machine names right now. Using varchars as keys might be a bad practice in database design, it however makes data synchronisation a bit easier.

berdir’s picture

Status: Needs review » Fixed

Looks good, commited!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.