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

Update broken on PGSQL #5909

Closed
LukasReschke opened this Issue Jul 27, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@LukasReschke
Member

LukasReschke commented Jul 27, 2017

From @BernhardPosselt on IRC:

"\OC\Updater::failure: Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'SELECT "f"."fileid", "f"."path", "p"."path" as "parent_path", "f"."name", "f"."parent", "f"."storage", "p"."storage" as "parent_storage" FROM "oc_filecache" f INNER JOIN "oc_filecache" p ON ("f"."parent" = "p"."fileid") AND ("p"."name" <> ?) WHERE "f"."path"
18:01:55 " || ? || "f"."name" LIMIT 1000' with params ["", "\/"]:\n\nSQLSTATE[42804]: Datatype mismatch: 7 ERROR: argument of WHERE must be type boolean, not type text\nLINE 1: ...ent" = "p"."fileid") AND ("p"."name" <> $1) WHERE "f"."path"...\n

@icewind1991 This seems like your repair step. Can you look into this? This is blocking 12.0.1. THX!

@LukasReschke LukasReschke added this to the Nextcloud 12.0.1 milestone Jul 27, 2017

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member

psql (PostgreSQL) 9.6.3 (latest Debian stable)

Member

BernhardPosselt commented Jul 27, 2017

psql (PostgreSQL) 9.6.3 (latest Debian stable)

@icewind1991

This comment has been minimized.

Show comment
Hide comment
@icewind1991

icewind1991 Jul 27, 2017

Member

No clue why the query builder is fucking up the query

private function getInvalidEntries() {

Member

icewind1991 commented Jul 27, 2017

No clue why the query builder is fucking up the query

private function getInvalidEntries() {

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member

This is $computedPath at https://github.com/nextcloud/server/blob/stable12/lib/private/Repair/NC13/RepairInvalidPaths.php#L57

`p`.`path` || :dcValue1 || `f`.`name`

That query won't work, you will need to expand it.

Member

BernhardPosselt commented Jul 27, 2017

This is $computedPath at https://github.com/nextcloud/server/blob/stable12/lib/private/Repair/NC13/RepairInvalidPaths.php#L57

`p`.`path` || :dcValue1 || `f`.`name`

That query won't work, you will need to expand it.

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member
owncloud=# select name from oc_news_folders where name <> 'open' || 'source';
ERROR:  argument of WHERE must be type boolean, not type text
LINE 1: select name from oc_news_folders where name <> 'open' || 'so...
owncloud=# select name from oc_news_folders where name <> ('open' || 'source');
    name     
-------------
 Open Source
 Media
 IT
(3 rows)
Member

BernhardPosselt commented Jul 27, 2017

owncloud=# select name from oc_news_folders where name <> 'open' || 'source';
ERROR:  argument of WHERE must be type boolean, not type text
LINE 1: select name from oc_news_folders where name <> 'open' || 'so...
owncloud=# select name from oc_news_folders where name <> ('open' || 'source');
    name     
-------------
 Open Source
 Media
 IT
(3 rows)
@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member

The error is the missing parentheses

Member

BernhardPosselt commented Jul 27, 2017

The error is the missing parentheses

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member

https://github.com/nextcloud/server/blob/master/lib/private/DB/QueryBuilder/FunctionBuilder/PgSqlFunctionBuilder.php#L28

return new QueryFunction($this->helper->quoteColumnName($x) . ' || ' . $this->helper->quoteColumnName($y));

should be

return new QueryFunction('(' . $this->helper->quoteColumnName($x) . ' || ' . $this->helper->quoteColumnName($y) . ')');

No idea why you overwrite this method because postgres has CONCAT(). Also I have no idea why you are not reusing doctrine's query builder

Member

BernhardPosselt commented Jul 27, 2017

https://github.com/nextcloud/server/blob/master/lib/private/DB/QueryBuilder/FunctionBuilder/PgSqlFunctionBuilder.php#L28

return new QueryFunction($this->helper->quoteColumnName($x) . ' || ' . $this->helper->quoteColumnName($y));

should be

return new QueryFunction('(' . $this->helper->quoteColumnName($x) . ' || ' . $this->helper->quoteColumnName($y) . ')');

No idea why you overwrite this method because postgres has CONCAT(). Also I have no idea why you are not reusing doctrine's query builder

@enoch85

This comment has been minimized.

Show comment
Hide comment
@enoch85

enoch85 Jul 27, 2017

Member

A little bit off topic here, but I wish PostgreSQL in general was better supported. In MariaDB/MySQL you have to mess around with UTF8mb4 if you want 4-byte support, which is default in PostgreSQL UTF8.

PostgreSQL feels like a better DB in general. Would be nice if Nextcloud decided to go down that path instead of recommending a less advanced (and IMHO) worse DB.

I'm writing this after I have been messing with MariaDB for hours to activate 4 byte support, which only took a few minutes in PostgreSQL (aka apt install postgresql). According to the IRC channel (#posgresql) UTF8mb4 is just something that MariaDB/MySQL came up with becuase they messed up the original code and had to make a quick fix to solve it. That doesn't sound good to me. Also UTFmb4 is considered "experimental" according to docs which isn't the case with PostgreSQL as it just works out of the box.

I think it was @MorrisJobke that said that MariaDB/MySQL was tested in a larger (EDIT: clustered enviroments, nextcloud/vm#277 (comment)) environment and proved to be better, but my guess is that that test was some time ago. Maybe you should consider making PostgreSQL the recomended DB after all?

Also, this is not just about UTF8mb4, I'm talking generally. There are a lot to gain in using PSQL any other DB IMHO. According to several hours of reading I did comparing and testing the alternatives.

Member

enoch85 commented Jul 27, 2017

A little bit off topic here, but I wish PostgreSQL in general was better supported. In MariaDB/MySQL you have to mess around with UTF8mb4 if you want 4-byte support, which is default in PostgreSQL UTF8.

PostgreSQL feels like a better DB in general. Would be nice if Nextcloud decided to go down that path instead of recommending a less advanced (and IMHO) worse DB.

I'm writing this after I have been messing with MariaDB for hours to activate 4 byte support, which only took a few minutes in PostgreSQL (aka apt install postgresql). According to the IRC channel (#posgresql) UTF8mb4 is just something that MariaDB/MySQL came up with becuase they messed up the original code and had to make a quick fix to solve it. That doesn't sound good to me. Also UTFmb4 is considered "experimental" according to docs which isn't the case with PostgreSQL as it just works out of the box.

I think it was @MorrisJobke that said that MariaDB/MySQL was tested in a larger (EDIT: clustered enviroments, nextcloud/vm#277 (comment)) environment and proved to be better, but my guess is that that test was some time ago. Maybe you should consider making PostgreSQL the recomended DB after all?

Also, this is not just about UTF8mb4, I'm talking generally. There are a lot to gain in using PSQL any other DB IMHO. According to several hours of reading I did comparing and testing the alternatives.

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 27, 2017

Member

Hi @enoch85

Please use a new topic ;) short answer: I think postgres and mysql are both equally well supported and there is no bias.

Member

BernhardPosselt commented Jul 27, 2017

Hi @enoch85

Please use a new topic ;) short answer: I think postgres and mysql are both equally well supported and there is no bias.

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Jul 28, 2017

Member

@BernhardPosselt

Also I have no idea why you are not reusing doctrine's query builder

We are, just have 2 manipulations in it:

  1. automatic adding of *PREFIX* on table names
  2. automatic quoting of table/column names for oracle support

However that doctrine query builder has no "FunctionBuilder" like we have, so we are not overwriting a doctrine method with some wrong.

Also as per https://www.postgresql.org/docs/9.1/static/functions-string.html || is also valid...

But thanks for finding the issue and feel free to send a PR? ;)

Member

nickvergessen commented Jul 28, 2017

@BernhardPosselt

Also I have no idea why you are not reusing doctrine's query builder

We are, just have 2 manipulations in it:

  1. automatic adding of *PREFIX* on table names
  2. automatic quoting of table/column names for oracle support

However that doctrine query builder has no "FunctionBuilder" like we have, so we are not overwriting a doctrine method with some wrong.

Also as per https://www.postgresql.org/docs/9.1/static/functions-string.html || is also valid...

But thanks for finding the issue and feel free to send a PR? ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment