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

[dbal] DBAL transport no longer works on PostgreSQL as of 0.9.0 #670

Closed
msheakoski opened this Issue Dec 5, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@msheakoski
Copy link
Contributor

msheakoski commented Dec 5, 2018

There is an issue of PostgreSQL not being able to work with UUID byte sequences produced by ramsey/uuid (see ramsey/uuid#179 for more details). The UUID type in PostgreSQL only accepts UUIDs in the hex string format (see https://www.postgresql.org/docs/current/datatype-uuid.html).

Additionally, according to this comment, OrderedTimeCodec only works with UUIDv1 and you are using UUIDv4 which is random, so I do not think that the codec is actually doing anything useful here.

Temporary solution:
I made a hotfix which switches binary back to hex string in case anybody needs to get PostgreSQL working while this issue is being examined.

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 5, 2018

It's time to add postgreSQL to our test stack.

@makasim makasim added bug 0.9 labels Dec 5, 2018

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 5, 2018

We used v1 but there were collisions (I still wonder why?) and there are no ones with v4 so we left it.
It might have had a relation to the bug #665 but I am not sure. We should definitely give v1 one more chance.

The UUID type in PostgreSQL only accepts UUIDs in the hex string format (see https://www.postgresql.org/docs/current/datatype-uuid.html).

This is something I expect Doctrine should handle, isn't it?

@msheakoski

This comment has been minimized.

Copy link
Contributor

msheakoski commented Dec 5, 2018

This is something I expect Doctrine should handle, isn't it?

I believe the ramsey/uuid-doctrine package may be able to handle it: https://github.com/ramsey/uuid-doctrine#binary-database-columns

@msheakoski

This comment has been minimized.

Copy link
Contributor

msheakoski commented Dec 5, 2018

We used v1 but there were collisions (I still wonder why?) and there are no ones with v4 so we left it.

It seems highly improbably to have a collision on a UUID v1 so it must have been something else causing them.

Quoted from Stack Overflow:

The timestamp field in the UUID is measured in 100-nanosecond intervals, so you'd have to generate two UUIDs within a tenth of a microsecond to get a collision. That corresponds to a rate of ten million transactions per second.

In addition to the timestamp there's also the clock-sequence, which seems to be seeded randomly; this is incremented every time a UUID is generated. This is effectively 10-bits, meaning you'd have to generate thousands of UUIDs per 100 nanosecond interval before you're in danger of a collision.

@Nommyde

This comment has been minimized.

Copy link
Contributor

Nommyde commented Dec 9, 2018

Default time generator in ramsey library uses gettimeofday function. This function is not high performance timer. It can only get microseconds. So collisions may occurs.

image

And you can not use binary encoded UUIDs in doctrine's UUID type. (because it is db abstraction layer)

In MairaDB I've got an error too. Because uuid mapped to char type, and DB expects valid unicode sequence.

@msheakoski

This comment has been minimized.

Copy link
Contributor

msheakoski commented Dec 9, 2018

@Nommyde that makes sense. In that case, it is better to stick to V4 UUIDs.

According to this comment in #280, it seems like there is not much of a real-world performance gain from using binary UUIDs. It is probably better to switch back to strings, which have better cross-platform compatibility. If the highest performance is desired, choose one of the AMQP transports instead.

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 10, 2018

What should be done:

  • Add PostgreSQS to testing setup
  • Remove OrderedTimeCodec
  • Use string instead of binary, let doctrine manage it.
@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 10, 2018

Is there someone willing to take it?

@jverdeyen

This comment has been minimized.

Copy link
Contributor

jverdeyen commented Dec 18, 2018

What should be done:

* Add PostgreSQS to testing setup

* Remove [OrderedTimeCodec](https://github.com/php-enqueue/enqueue-dev/blob/master/pkg/dbal/DbalContext.php#L157)

* Use string instead of binary, let doctrine manage it.

I've had an issue with the binary formatted id's and using a minor tweaked fork for now. I'm just using the string rep of the uuid.

jverdeyen/dbal@ba0b452

It used to be like this, no?

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 18, 2018

@jverdeyen yeap, it should be the way you've done it.

@jverdeyen

This comment has been minimized.

Copy link
Contributor

jverdeyen commented Dec 18, 2018

@jverdeyen yeap, it should be the way you've done it.

Ok, I'll clean it up and create a PR for this change.

@makasim

This comment has been minimized.

Copy link
Member

makasim commented Dec 21, 2018

fixed in 0.9.5

@makasim makasim closed this Dec 21, 2018

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