search

sfackler / rust-postgres

Native PostgreSQL driver for the Rust programming language
Apache License 2.0
3.44k stars 436 forks source link

i128 support #933

Closed vporton closed 2 years ago

vporton commented 2 years ago
vitaly-burovoy commented 2 years ago

Why do you think it can work?

Where are checks for fractional part (which should lead to failures)?

sfackler commented 2 years ago

NUMERIC is an arbitrary precision decimal type, so I'm not sure this conversion is correct. Minimally it needs to handle the fractional part in some way.

vporton commented 2 years ago

Where are checks for fractional part (which should lead to failures)?

Doesn't the below code from my patch fail in the case of a fractional part?

    if !buf.is_empty() {
        return Err("invalid buffer size".into());
    }
vitaly-burovoy commented 2 years ago

No. It is better to read how NUMERIC is sent/receive at PostgreSQL sources[1] and read existing implementations[2].

Tests for unknown (for you) behavior and how data is encoded is «must have».

— [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/adt/numeric.c;h=920a63b00816c0e665e94edd86a54dc031c1d35d;hb=refs/heads/master

[2] https://github.com/paupino/rust-decimal/blob/master/src/postgres/driver.rs

vitaly-burovoy commented 2 years ago

P.S. in PG's sources functions which are used in the binary protocol are numeric_recv (line 991) and numeric_send (line 1079).

EvanCarroll commented 2 years ago

This is not a good idea. 128 bits is 16 bytes. PostgreSQL has a fixed-width 16 byte type: uuid. Ignoring the rendering, if you're going to implement a native store for i128 in PostgreSQL you'd be far better of using this type.

UUID is abused for this already too.

https://dba.stackexchange.com/a/115316/2639