#2477853: PostgreSQL: Add support for reserved field/column names added support for reserved column names, fixing the postgres driver, and adding tests insert/update/delete for all databases.
From the issue:
Some migrate tables use reserved keywords as column name like OFFSET currently causing exceptions on PostgreSQL because such names need quoting to work. As this is somehow not a problem for MySQL we decided to add support for PostgreSQL also by adding quotes where needed.
What we didn't notice is that OFFSET is reserved in postgres (and SQL-2008+), but not in mysql (sql-92).
This means that our tests are lying, since they aren't testing a keyword that's actually reserved on all databases.
The solution is to fix the test, then fix our mysql driver.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2560965-1-reserved-columns-test-fix.patch | 3.25 KB | bojanz |
Comments
Comment #2
bojanz commentedLet's start with the tests.
Question: Do we want to quote all identifiers, or just the ones that are reserved keywords? The postgres driver did #2 for undocumented reasons possibly related to case sensitivity. I'm not sure the same reasoning applies here.
I am also wondering why there's no coverage for select queries.
Berdir indicated he is against that so it needs further discussion.
EDIT: Reference for case sensitivity issues: http://www.alberton.info/dbms_identifiers_and_case_sensitivity.html
As far as I can see, MySQL identifiers are always compared according to other settings (lower_case_tablenames), quoting doesn't change that.
Comment #4
bojanz commentedComment #5
Crell commentedOh this issue again...
We went around on the reserved words question for several months, years ago. Check out the nid on this issue :-): #371: resolve ANSI SQL-92/99/2003 reserved words conflict in query statements
Short version: Supporting reserved words as column names is hard, buggy, sucks, and has negative performance implications because it involves string parsing. Therefore, Don't Do That(tm). Just don't use reserved words, and it's the module's job to make sure it avoids them. See:
https://www.drupal.org/node/141051
So if Migrate is using a reserved word like OFFSET and breaking, the solution is: Don't use reserved words like OFFSET.
With that in mind, I'm looking at the patch and thinking, "wait, so despite the previous statement someone managed to sneak *TEST* code into the codebase that uses reserved words??? Why? That's a mistake in the first place."
Comment #6
bojanz commented@Crell
In the issue I linked people decided to fix this, fixed it for postgres only, added a test which accidentally passes under mysql.
So we'd need to revert the entire fix, then open a major-to-critical migrate issue.
Look at the committed patch, I don't see any string parsing. Looks doable to make a similar fix for mysql.
Comment #7
Crell commentedUgh. That's a stealth API change, as it means we now claim to support reserved word columns. That's a major change of direction, and one that needs further discussion, because now we're quasi-committing to supporting reserved word columns across all supported databases. That is NOT something I am even remotely willing to commit to, especially with only 6 criticals to go until RC.
Also, the patch there looks like it only works for dynamic queries, those using a query builder. It would not work for a query literal in query() or a where() method without string parsing. That is, the patch in the referenced issue is buggy and incomplete, as it translates to "we sometimes support reserved word columns, but we're not documenting that, and oh yeah only on postgres".
So yes, my stance at the moment is that we should back out that patch as incomplete and misleading, then just fix migrate. "Don't use reserved word columns, yo" has been the de facto policy since before I even touched the DB layer. :-) Only supporting it for built queries and then leaving it up to each driver to figure out what is reserved is far too error prone.
Comment #8
bojanz commented@Crell
Okay, I'll leave you to make that case on the original issue.
I'm fine with the reasoning, just wanted to address the current inconsistency.
Comment #9
Crell commentedComment #15
alexpottWell MySQL have kinda forced our hand end because MySQL8 has a load of new reserved keywords that we have usages of in core - for example -
functionis a column in the simpletest table. I'm going to mark this issue as a duplicate of #2966523: MySQL 8 Support as that has a complete fix for this.