laravel-efficient-uuid icon indicating copy to clipboard operation
laravel-efficient-uuid copied to clipboard

Doesn't work with PostgreSQL 12

Open bcalik opened this issue 4 years ago • 14 comments

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

bcalik avatar Jun 25 '20 09:06 bcalik

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

mfauveau avatar Aug 10 '20 20:08 mfauveau

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.

bcalik avatar Sep 03 '20 05:09 bcalik

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.

defenestrator avatar Sep 04 '20 14:09 defenestrator

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.

bcalik avatar Sep 04 '20 14:09 bcalik

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?

defenestrator avatar Sep 04 '20 14:09 defenestrator

@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.

bcalik avatar Sep 04 '20 15:09 bcalik

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.

defenestrator avatar Sep 04 '20 16:09 defenestrator

@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.

bcalik avatar Dec 05 '20 08:12 bcalik

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

michaeldyrynda avatar Dec 05 '20 08:12 michaeldyrynda

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.

bcalik avatar Dec 05 '20 08:12 bcalik

I'll handle this later today.

defenestrator avatar Dec 05 '20 12:12 defenestrator

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)

tpetry avatar Dec 10 '21 12:12 tpetry

@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?

michaeldyrynda avatar Dec 10 '21 20:12 michaeldyrynda

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.

tpetry avatar Dec 12 '21 14:12 tpetry