Skip to content
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

exp is not fully compatible with PG #8025

Closed
lmatz opened this issue Feb 17, 2023 · 11 comments
Closed

exp is not fully compatible with PG #8025

lmatz opened this issue Feb 17, 2023 · 11 comments
Assignees
Labels
good first issue Good for newcomers help wanted Issues that need help from contributors priority/low

Comments

@lmatz
Copy link
Contributor

lmatz commented Feb 17, 2023

For example:
select exp(-10000000); returns 0 in RW
but it errors underflow in PG

See comments at #7971 (comment)

PG has special logic for this: https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/float.c#L1649

@lmatz lmatz added good first issue Good for newcomers help wanted Issues that need help from contributors priority/low labels Feb 17, 2023
@github-actions github-actions bot added this to the release-0.1.18 milestone Feb 17, 2023
@lmatz lmatz removed this from the release-0.1.18 milestone Feb 20, 2023
@erichgess
Copy link
Contributor

Just for clarification, this ticket to update RW to have the same behavior as PG when the exp instruction returns 0? In other words, the result of this ticket would be to recreate the special logic from PG within RW?

@lmatz
Copy link
Contributor Author

lmatz commented Feb 25, 2023

Hi @erichgess, I think you are right, we intend to let RW have the same behavior as PG's, i.e. report underflow and overflow under precisely the same situation. cc: @xiangjinwu

@erichgess
Copy link
Contributor

Your point on the other ticket about not artificially restricting RW to what PG can do makes a lot of sense, but in this case, certainly, I think what PG is doing is the correct path, because their goal is to have consistent behavior across all platforms.

Could I hop on this ticket?

@lmatz
Copy link
Contributor Author

lmatz commented Feb 26, 2023

Sure @erichgess, I think your reasoning makes sense
please go ahead, thanks for your interest!

@erichgess
Copy link
Contributor

Great, thank you.

@erichgess
Copy link
Contributor

@lmatz PG has separate error codes for flow overflow and underflow, but RW only has NumericOutOfRange, which means that RW will return NumericOutOfRange when exp(x) == 0, so it will not perfectly mirror PG.

Is the long term plan to eventually have the error codes in RW have the same granularity as the error codes from PG?

@lmatz
Copy link
Contributor Author

lmatz commented Feb 28, 2023

@erichgess
Yes, we also try to have error codes compatible with PG ones whenever possible.

If you'd like to use overflow and underflow instead of simply out of range, feel free to change it. This is a good point!

@erichgess
Copy link
Contributor

erichgess commented Feb 28, 2023

@lmatz The PG implementation uses unlikely to hint to the compiler that some if branches are unlikely to be executed. Do you want to replicate the same optimizations in RW?

@lmatz
Copy link
Contributor Author

lmatz commented Mar 1, 2023

@erichgess, sure if you'd like to,
I just took a look,
and it seems people have some best practice when using stable Rust:https://users.rust-lang.org/t/compiler-hint-for-unlikely-likely-for-if-branches/62102/4
without relying on the nightly feature

@erichgess
Copy link
Contributor

Should this ticket be closed now that the PR is merged?

@xiangjinwu
Copy link
Contributor

Thank you @erichgess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Issues that need help from contributors priority/low
Projects
None yet
Development

No branches or pull requests

3 participants