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

Add commutator #16

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Add commutator #16

merged 2 commits into from
Aug 23, 2023

Conversation

pksunkara
Copy link
Owner

No description provided.

@pksunkara pksunkara linked an issue Aug 10, 2023 that may be closed by this pull request
@pksunkara pksunkara changed the base branch from master to v0 August 10, 2023 18:38
@pksunkara pksunkara force-pushed the fixes-13 branch 3 times, most recently from 8c85a66 to edd3c35 Compare August 12, 2023 23:03
@pksunkara pksunkara changed the base branch from v0 to master August 12, 2023 23:20
Adds test cases for simple foreign keys, many to many tables, and joins
between tables. This tests the functionality of the commutator and
hash functions for the `ulid` type in postgres.

ref: [#13][issue]

[issue]: #13
@pksunkara pksunkara changed the title Add failing tests Add commutator Aug 12, 2023
workingjubilee added a commit to workingjubilee/pgrx that referenced this pull request Aug 16, 2023
Derived from pksunkara/pgx_ulid#16
The test guarantees hash joins and commutative joins work,
but by default yield an error something like:

could not find commutator for operator 140551
postgres location: clauses.c
rust location: SQL statement "CREATE TABLE hexintext (

...and so on.
By indicating that these operators commute with each other
we can allow Postgres to use this while implementing joins.
@workingjubilee
Copy link
Contributor

If pgcentralfoundation/pgrx#1261 lands in some form, you can land this with minimal changes. Until then, you could, I guess, define the pg_extern functions and annotate them with pg_operator macros by hand?

...yeah that's not great.

@pksunkara
Copy link
Owner Author

If pgcentralfoundation/pgrx#1261 lands in some form, you can land this with minimal changes.

Thanks.

Until then, you could, I guess, define the pg_extern functions and annotate them with pg_operator macros by hand?

Could you please point me to an example? I had trouble getting this working.

workingjubilee added a commit to pgcentralfoundation/pgrx that referenced this pull request Aug 16, 2023
This fixes pksunkara/pgx_ulid#16 without
requiring additional work on the part of the programmer. However, there
is a design decision here: do we require explicit annotation before
assuming commutative equality or not?

Here I chose to demand that the equality implementation adheres to
Rust's requirements for "total" equality. However, as the SQL we emit is
used deep inside Postgres for various forms of algebraic reasoning,
including, as I understand it, index maintenance, I can't promise that
we don't wind up running afoul of Postgres using this data in a way that
will incur UB if the implementation is actively malicious, as can be
done with implementations of the following kind:

```rust
impl PartialEq for AlwaysRandom {
    fn eq(&self, other: &Self) -> bool {
        rand::random()
    }
}

impl Eq for AlwaysRandom { }
```

Of course, this also violates PARALLEL SAFE and IMMUTABLE.
This problem also was always true, as we emitted NEGATOR clauses,
which means even this was probably already broken:

```rust
impl PartialEq for AlwaysWrong {
    fn eq(&self, other: &Self) -> bool {
        false
    }
}

impl Eq for AlwaysWrong {}
```

This is less likely to cause issues in more realistic code samples.
@workingjubilee
Copy link
Contributor

I landed it, so pgrx 0.10.0 (or 0.10.0-beta.4, or whatever) will fix your code.

@pksunkara
Copy link
Owner Author

@workingjubilee Thank you for the help. Any timeline you guys are thinking for 0.10.0?

@workingjubilee
Copy link
Contributor

To actually answer this question:

Could you please point me to an example? I had trouble getting this working.

It would basically be like writing the fully-expanded, specific-type-instantiated version of this macro's inner contents:

https://github.com/pgcentralfoundation/pgrx/blob/159295238f7513da2e4c7075b004cf0b5a60d30b/pgrx-macros/src/operators.rs#L95-L114

But one might also have to correctly invoke the relevant parts of pgrx-sql-entity-graph to make it correctly create things like operator classes, etc. for the type, and it's all quite hellacious so I don't exactly recommend it.

@pksunkara pksunkara merged commit 75678e3 into master Aug 23, 2023
@pksunkara pksunkara deleted the fixes-13 branch August 23, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider adding hash operator and commutator
3 participants