Skip to content
This repository has been archived by the owner on Apr 23, 2024. It is now read-only.

Doesn't work with PostgreSQL 12 #32

Open
bcalik opened this issue Jun 25, 2020 · 14 comments · Fixed by #37
Open

Doesn't work with PostgreSQL 12 #32

bcalik opened this issue Jun 25, 2020 · 14 comments · Fixed by #37

Comments

@bcalik
Copy link

bcalik commented Jun 25, 2020

I'm using the latest v4.0.1 version of the package on Laravel 7.
I'm also using PostgreSQL 12.

When I try to migrate, I get this error:

Method Illuminate\Database\Schema\Grammars\PostgresGrammar::typeEfficientUuid does not exist.

Its probably because of this line expects the postgres name for driver, but Laravel 7 uses pgsql as driver name.

I tried to change the name to pgsql, but then I get this error:

   Illuminate\Database\QueryException

SQLSTATE[22021]: Character not in repertoire: 7 ERROR:  invalid byte sequence for encoding "UTF8": 0x90
(SQL: insert into "users" ("name", "email", "email_verified_at", "password", "remember_token", "status",
"uuid", "organization_id", "updated_at", "created_at") values (Prof. Johnny Torphy, [email protected],
2020-06-25 12:02:06, $2y$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi, ejdCIBmyNS, 1,
 ��G�|D��*���C�, ?, 2020-06-25 12:02:06, 2020-06-25 12:02:06) returning "id")

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:671
    667|         // If an exception occurs when attempting to run a query, we'll format the error
    668|         // message to include the bindings with SQL, which will make this exception a
    669|         // lot more helpful to the developer instead of just the database's errors.
    670|         catch (Exception $e) {
  > 671|             throw new QueryException(
    672|                 $query, $this->prepareBindings($bindings), $e
    673|             );
    674|         }
    675|

cc @defenestrator

@mfauveau
Copy link

You are right about the fix. The other error is unrelated.

@bcalik
Copy link
Author

bcalik commented Sep 3, 2020

You are right about the fix. The other error is unrelated.

@mfauveau How its unrelated? The error is about uuid column content, its a bytea column and doesn't accept the given content.

@defenestrator
Copy link
Contributor

You are right about the fix. The other error is unrelated.

@mfauveau How its unrelated? The error is about uuid column content, its a bytea column and doesn't accept the given content.

I'm wondering if you have a configuration problem. Did you manually register the service provider? There's no package discovery on this package, intentionally. That error message seems like a config issue, to me. It says the method doesn't exist in Illuminate\Database\Schema\Grammars\PostgresGrammar::typeEfficientUuid so it seems like maybe you didn't add it to the providers array.

'providers' => [
    ...
    Dyrynda\Database\LaravelEfficientUuidServiceProvider::class,
],

Forgive me if I'm wrong, but I'd like to help support this in any way I can.

@bcalik
Copy link
Author

bcalik commented Sep 4, 2020

I'm wondering if you have a configuration problem. Did you manually register the service provider? There's no package discovery on this package, intentionally. That error message seems like a config issue, to me. It says the method doesn't exist in Illuminate\Database\Schema\Grammars\PostgresGrammar::typeEfficientUuid so it seems like maybe you didn't add it to the providers array.

'providers' => [
    ...
    Dyrynda\Database\LaravelEfficientUuidServiceProvider::class,
],

Forgive me if I'm wrong, but I'd like to help support this in any way I can.

Yes, I have registered the service provider. It works for MySQL without any issue.

Error says the method doesn't exist, because it doesn't exists.
The whole purpose of this package is to provide this new method by extending the PostgresGrammar.
The extended class is registered/resolved in here by driver name.
Laravel uses pgsql driver name, not postgres. So It only works when you change it to pgsql.
This solves the issue number 1.

The second error is another issue, which is what I'm actually asking for help.
So please focus on the second error if you would like to help.

@defenestrator
Copy link
Contributor

I'm actually sending a pull request for the first error in the next few minutes.

I'm aware of the whole purpose of this package, which you have misstated somewhat.

I added the pg support, so I feel some responsibility for this.

Error two requires more information about your system to solve, since changing postgres to pgsql apparently does not resolve it.

The typeEfficientUuid() method definitely does exist, given that the package is configured correctly, so I'm not sure what the remaining issue would be caused by. Can you give us some log output and/or more information?

@bcalik
Copy link
Author

bcalik commented Sep 4, 2020

@defenestrator

I added the pg support, so I feel some responsibility for this.

Obviously PostgreSQL integration is not tested and its not complete, the driver name shouldn't be postgres at the first place.

...so I'm not sure what the remaining issue would be caused by...

bytea may not be the correct column type for this, or you may need to use pg_escape_bytea() and pg_unescape_bytea() methods to convert it properly.

@defenestrator
Copy link
Contributor

defenestrator commented Sep 4, 2020

Feel free to make a PR and write some tests for it. I may get to that someday.

It works dandy on my systems, and once my PR from today is merged it will follow the Laravel default configuration. I admit I made a mistake, one which I have now fixed.

@bcalik
Copy link
Author

bcalik commented Dec 5, 2020

@michaeldyrynda It does not fix the issue. You still need to use pg_escape_bytea() and pg_unescape_bytea() methods to convert it properly otherwise you will get errors.

So please reopen this so everyone can see that package has problems with pgsql.

@michaeldyrynda
Copy link
Owner

I'm happy to accept a PR for this, but as I don't use PG myself, I've no mechanism to test it.

@bcalik
Copy link
Author

bcalik commented Dec 5, 2020

I'm happy to accept a PR for this, but as I don't use PG myself, I've no mechanism to test it.

I didn't say "fix this", I said reopen this so someone else can fix it.

@michaeldyrynda michaeldyrynda reopened this Dec 5, 2020
@defenestrator
Copy link
Contributor

defenestrator commented Dec 5, 2020

I'll handle this later today.

@tpetry
Copy link

tpetry commented Dec 10, 2021

You don't need this for PostgreSQL. The uuid type already used by Laravel is space-efficient! Internally, everything is stored as 16 byte data, just the representation in sql is a string for ease of use.

postgres=# SELECT
    pg_column_size('7bf18353-fa4a-4ec8-99ce-de81586e4b3f'::text) as text_size,
    pg_column_size('7bf18353-fa4a-4ec8-99ce-de81586e4b3f'::uuid) as uuid_size;
 text_size | uuid_size
-----------+-----------
        40 |        16
(1 row)

@michaeldyrynda
Copy link
Owner

michaeldyrynda commented Dec 10, 2021

@tpetry Is the right path forward here that we:

  • Just document how to use the appropriate field type with existing migration methods,
  • Update the typeEfficientUuid macro to use uuid,
  • Recommend using only the package’s cat class,
  • Suggest against using the package at all with Postgres as it always does what is needed under the hood, or
  • Something else?

@tpetry
Copy link

tpetry commented Dec 12, 2021

It's a complicated question and i can't answer it for you. It's about how you want to handle compatibility.

Just document how to use the appropriate field type with existing migration methods,
A good approach 👍

Update the typeEfficientUuid macro to use uuid,
Won't this break backward compatibility? Any application using the package already will then use the uuid type but the casts are still used.

Recommend using only the package’s cat class,
Casts would not be required for PostgreSQL too, you can use just what laravel provides.

Suggest against using the package at all with Postgres as it always does what is needed under the hood, or
I think it's the best approach for version 4. In version 5 you could remove PostgreSQL support and state the reason. For the moment i would just:

  • Add a note to the readme that PostgreSQL uuids are already optimized and this package is not needed for PostgreSQL but still available for anyone still using it
  • Trigger a notice/warning on migrations for PostgreSQL to inform anyone of the behaviour and that PostgreSQL support will be removed in the future. You could also make it for v5 use the standard uuid type and just be a no operation package for PostgreSQL but still do all the work for everyone else. So the package can be used for all database and the best approach is used specific for every database. Meaning the current logic is kept for everyone except PostgreSQL.

By the way will MariaDB 10.7 will get a native uuid type too. I asked on twitter about efficient storage. And following their internal discussion seems to indicate that it will be space-efficient too.

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

Successfully merging a pull request may close this issue.

5 participants