Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostgreSQL support #2875

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

iWantToKeepAnon
Copy link

I pulled as much database specific code as I found into "includes/class-<mysql|pgsql>.php". I removed the mysql specific backtick syntax everywhere I could (mysql will work w/o it and psql won't work with it). I hacked "docker-entrypoint.sh" over on the docker project (another PR there).

To install pgsql and create the database if pgsql is selected (see "stack.yml" and "stack-pgsql.yml" and "start.bash").
There needs to be a good database selector mechanism, for now I run:

  • cd users && ln -s ../includes/class-pgsql.php ./db.php

and that selects postgress. I have not touched the test cases.

.gitignore Outdated Show resolved Hide resolved
admin/index.php Outdated
@@ -61,14 +61,14 @@
$search_text = $search;
$search = str_replace( '*', '%', '*' . $search . '*' );
if( $search_in == 'all' ) {
$where['sql'] .= " AND CONCAT_WS('',`keyword`,`url`,`title`,`ip`) LIKE (:search)";
$where['sql'] .= " AND CONCAT_WS('',keyword,url,title,ip) LIKE (:search)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason for removing the backticks?
They have a concrete security feature for SQL.

'short URL charset' => sprintf('ALTER TABLE %s CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_bin;', YOURLS_DB_TABLE_URL),
'short URL varchar' => sprintf("ALTER TABLE %s CHANGE keyword keyword VARCHAR(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '';", YOURLS_DB_TABLE_URL),
'short URL type' => sprintf("ALTER TABLE %s CHANGE url url TEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL;", YOURLS_DB_TABLE_URL),
'short URL type' => sprintf("ALTER TABLE %s CHANGE title title TEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", YOURLS_DB_TABLE_URL),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment but here the backtick protect variables.
Big warning

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backticks are too specific to mysql, it is either remove them or isolate 100% of the SQL. No other db engine recognizes them so for postgress and maybe eventually oracle, sqlite, etc... they need to go.

Another solution would be to have a global quoting variable ($q) that could be set to ticks or quotes as needed.

@dgw dgw changed the title Postgress PR PostgreSQL support Mar 18, 2021
@ozh
Copy link
Member

ozh commented Mar 20, 2021

Regarding postgresql : the goal here should be to make things compatible with a PGSQL plugin (or other DB engine as you mention) but I don't want to include PGSQL code.
Two simple reasons:

  • 95% of people don't care about pgsql
  • I don't use psql so I won't maintain this code :)

Removing the backticks wrapping column names shouldn't be problematic (it's all hard coded), however we need them around table names : MySQL needs backticks for table names with a dash (eg joe-yourls), see for instance #2844. Maybe a helper function, similar to your dt_year(), with something like:

public function get_tablename($table) {
    return yourls_apply_filter('get_tablename', "`$table`");
}

so it's easy for a PSQL/SQLITE/etc plugin to hook into this and remove backticks

@ozh ozh mentioned this pull request Mar 30, 2021
@ozh
Copy link
Member

ozh commented Apr 8, 2021

Other things to consider : as far as I understand it the upgrade functions too would need complete rewriting

@iWantToKeepAnon
Copy link
Author

Other things to consider : as far as I understand it the upgrade functions too would need complete rewriting

Yes, or just kept separate like the db code is. Chose which upgrade module to use on whatever DB flag gets worked out. And since pgsql is brand new, no old migration code support necessary.

@iWantToKeepAnon
Copy link
Author

MySQL needs backticks for table names with a dash (eg joe-yourls)

Ah, ok. The yourls containers for mysql work with this PR but I doubt I had dashes in table names.

@FrankDupree
Copy link

Regarding postgresql : the goal here should be to make things compatible with a PGSQL plugin (or other DB engine as you mention) but I don't want to include PGSQL code.
Two simple reasons:

  • 95% of people don't care about pgsql
  • I don't use psql so I won't maintain this code :)

@ozh I really don't agree with point number 1. Postgres really is a robust and popular database. its a shame its not supported by default. and saying something like "I dont use psql so I wont maintain this code" is discouraging. its open source bro, everyone is invited and everyone can help maintain it. @iWantToKeepAnon well done for the effort. you still the man @ozh 💯

@iWantToKeepAnon
Copy link
Author

  • 95% of people don't care about pgsql
  • I don't use psql so I won't maintain this code :)

@FrankDupree, thanks for commenting. And sorry I haven't had time to resolve the conflict put on the pr.

@ozh:

#1: they don't care b/c it isn't available
#2: I didn't either until the pain of switching from mysql was greater than the pain of mysql. pgsql is a much more competent and consistent db engine.

I wrote the pr b/c I didn't want to maintain 2 db servers and my main project was experiencing too much mysql pain to stay put.

@FrankDupree
Copy link

@iWantToKeepAnon since you have lots of experience with this service, what other alternatives could you recommend, im looking at polr or shlink which would you go for?

@iWantToKeepAnon
Copy link
Author

@iWantToKeepAnon since you have lots of experience with this service, what other alternatives could you recommend, im looking at polr or shlink which would you go for?

I am far from an expert on url shorteners. But looking at the 2:

a) polr only lists mysql as a database, so no change there
b) shlink lists pgsql so that seems to meet your desired db platform; I like the command line interface idea too

I am satisfied with my pgsql plugin/patch for the moment. It would be nice for it to be in the core product to stay current.

@ozh ozh marked this pull request as draft September 7, 2021 08:42
@ozh
Copy link
Member

ozh commented May 8, 2022

Instead of rewriting things and pulling hair to make queries compatible with pgsql, I think a much simpler option would be to use a query builder, such as https://github.com/auraphp/Aura.SqlQuery/ , to write queries independently from the SQL engine. Then, a one liner plugin would be enough to use pgsql, sqlite or whatever.

@iWantToKeepAnon
Copy link
Author

... to write queries independently from the SQL engine. Then, a one liner plugin would be enough to use pgsql, sqlite or whatever.

True, that's a big update but I think the freedom of choice payoff would be bigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgress code supplied, PR? PostgreSQL support with the now used PDO library
4 participants