Conversation
|
I'm trying to investigate in the code. (I'm far from being a good PHP developer :-D) I see this: I'm wondering if this will solve the issue about the wp_e_events SQL table for example, because the SQL code sent to PostgreSQL at this time, as seen in my audit logs, is: As you can see, the line about the id column has an auto_increment directive, but there is no NOT NULL directive inside. So maybe this line will not match the $pattern PHP line of code because it seems to me this line of code is looking for the NOT NULL directive, which potentially doesn't exist depending of how SQL tables are created. Finally, what I mean is that in the $pattern PHP line of code, the presence of NOT NULL directive is optionnal. If you can make it optionnal, this should match this kind of cases. Any way, in PostgreSQL, the simple fact to create a primay key on a column implies the fact the column is NOT NULL automatically. See below on my example, where I create a test table without specifying the NOT NULL directive, but when displaying the description of the table, the NOT NULL is automatically there on the column because of the existence of the primary key: What do you think ? |
|
@hahnn I switched to bigserial instead. it looks like the sequences still get autocreated. |
|
Ran into an interesting issue, testing this by hand, not sure if the same issue will occur when deployed Running the following query CREATE TABLE wp_itsec_vulnerabilities (
id varchar(128) NOT NULL,
software_type varchar(20) NOT NULL,
software_slug varchar(255) NOT NULL,
first_seen timestamp NOT NULL,
last_seen timestamp NOT NULL,
resolved_at timestamp default NULL,
resolved_by bigint NOT NULL default 0,
resolution varchar(20) NOT NULL default '',
details text NOT NULL,
PRIMARY KEY (id)
);
CREATE INDEX wp_itsec_vulnerabilities_resolution ON wp_itsec_vulnerabilities (resolution);
CREATE INDEX wp_itsec_vulnerabilities_software_type ON wp_itsec_vulnerabilities (software_type);
CREATE INDEX wp_itsec_vulnerabilities_last_seen ON wp_itsec_vulnerabilities (last_seen);
which is valid SQL gives Running first the create, then the index 1 by 1, works fine. It may be possible that the rewrite SQL commands should take an array of $SQL instead of a string, and then append to this array so that each command can be run 1 by 1 without exploding on ";" or something else fragile |
About that, I made the test in my lab, and the table has been created without any issue: So I cannot explain why you get a syntax error on your side... I'm think about a typo, or bad invisble characters you might have inside the code that generates a syntax error on DB side??? |
|
I've this issue: To solve it, I've added this inside private $stringReplacements: In the same order of idea, I see several SQL tables cannot be created because they have int(10) columns. So to solve this, I've also added: A suggestion here could be to implement something more robust like for any size of smallint(NB), int(NB) or bigint(NB), then convert to smallint, int or bigint. |
|
I've to say that with all the changes you made in your source code, lot more SQL tables can be created without issues when installing/enabling plugins. There are still issues I'm seeing. Here is another one below, showing a KEY line composed of several columns cannot be converted as an index creation (or maybe it's because of the single quotes?). And there will be potentially another issue with this table, because it will try to create a column called date whereas date is a reserved keyword and as such it should be between double quotes : So in fact the converted result you should get should be this one: |
|
I would also suggest you to keep the CREATE TABLE IF NOT EXISTS instead of converting it to just CREATE TABLE: that will avoid errors on DB side because the table already exist. Better: it would be probably nice to add the IF NOT EXISTS clause if it's not detected first in the CREATE TABLE command... In order to do that, here below is what I did in the PHP code (added the reverse replacement with the last str_ireplace line of code): |
|
Another issue as you can see below: In the converted result, the column referred text CHARACTER SET utf8 NOT NULL is the issue: in PostgreSQL, CHARACTER SET utf8 must be removed, and the result for this column should be referred text NOT NULL. to this: BTW, I see there are BIGINT(48) that have not been converted, so as usual, I've added this below to private $stringReplacements: And again, this table has column names using reserved keywords: like timestamp or date. Then it shoud be between double quotes. |
|
Another issue about a type converted from tinyint(1) to a type called tinysmallint which is not an existing type in PostgreSQL: I think this issue is existing because of the content of private $stringReplacements block of code: is before the line below: So when conversion is made, tinyint(1) matches in fact the first line which is about int(1), and that's why the converted result is wrong and become tinysmallint. An easy fix would probably be to reverse the order of PHP lines in private $stringReplacements block of code, but something more robust would be probably needed... |
|
Another one: DEFAULT CHARSET=utf8 should not be there in the converted result. |
|
I should have some time to look at this later this week and will incorporate tests for the cases you've brought up, thanks. I think I'll just add a space before int(1) so that it only matches " int(1)" |
|
@hahnn I believe I've fixed all the reported issues in this PR and added tests for each of them |
|
Fixed a couple more bugs, but this still isn't ready to merge. It seems like this PR changes the way the sequences get generated in a way that the code doesn't expect. Here's the list of currently broken sequences when installing |
|
It's not possible to change the name of sequences created by use of bigserial and serial, I don't want to move away from using those types so that requires changing the emulation of mysqli_insert_id found here: postgresql-for-wordpress/pg4wp/driver_pgsql.php Line 1060 in 94259e4 and the drop table rewriter here: My current thought is to select the list of sequences from postgres and then get the one that matches our table |
|
Interesting idea, We could append We could then cache that ID instead of having to query it later. to select the primary key before doing insertion and this can be cached |

Closes: #73