Add a macro pg_cast for constructing PG casts from Rust functions.
#1445
Conversation
|
Thanks. Best Christmas present ever! I won’t have a chance to review it probably until January, so hang in there. |
To use the new macro, add the `pg_cast` and `pg_extern` macros on
single-argument single-return value Rust function:
```rust
[pg_extern]
[pg_cast]
fn test_cast(_value: Json) -> i32 {
0
}
```
It is possible to parametrize the cast by adding either `assignment` or
`implicit` to the `pg_cast` macro as argument:
```
[pg_cast(assignment)] -> creates a PG cast that can be used for casting
from one type to another at assignment time
[pg_cast(implicit)] -> creates a PG cast that can be used for casting
from one type to another in any context
```
TESTED=`cargo-pgrx pgrx test --features "pg13"`
|
why does |
I considered 3 implementation options:
Option 2 requires 2 more macros which seems unnecessary to me (though this is the approach that I may be missing something or not grasping the overall macro design consideration here so curious about what you think! |
|
Option 2 does seem bad, yes.
Oh, we wouldn't want to accept using Obviously, that's a one-way constraint. The other way around wouldn't suddenly emit errors, since that would break... uh, every instance of In general, people don't actually write the For Option 3, it's fine if we make any number of breaking changes to that enum, so it remains a possibility. |
pgrx-macros/src/lib.rs
Outdated
| /// Declare a function as `#[pg_cast]` to indicate that it represents a Postgres cast | ||
| /// `cargo pgrx schema` will automatically generate the underlying SQL | ||
| #[proc_macro_attribute] | ||
| pub fn pg_cast(_attr: TokenStream, item: TokenStream) -> TokenStream { | ||
| item | ||
| } |
There was a problem hiding this comment.
This should, if we go with pg_cast requiring pg_extern, specify that it requires pg_extern.
In either case, it should also document the valid arguments to the attribute (implicit, assignment), because these things are not made obvious by tooling, unlike how they would be in e.g. a function (which would emit its arguments and types into rustdoc, at least).
Not all proc_macro_attributes in pgrx-macros do this, so you may be short on examples to imitate. It just has to cover the very basics, here.
There was a problem hiding this comment.
Went with a different approach of having pg_cast do the compile time validation and only after validation delegate to PgExtern for code-gen for the actual pg functions and casts. This avoids all the impossible states that the previous approach has.
Let me know what you think!
- Validate attributes at compile time before delegating to `PgExtern` - Using `pg_cast` and `pg_extern` on the same item will result in duplicate definitions - Added a `as_cast(PgCast)` option to `PgExtern` to augment `PgExtern`. Did not implement this as `PgExtern` attribute to avoid poluting `PgExtern` options
|
Nice! This seems pretty good. Going to take a closer look tomorrow, but it seems mergeable as-is or with perhaps small changes. I'm considering revising the idiom for these attribute macros to match the SQL they implement but that would go beyond this PR so it won't affect this, though I'm happy to hear if you have any feelings after having tooled around here. |
| #[pg_cast] | ||
| pub fn cast_function() -> i32 { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
This one successfully compiles (which is incorrect). The implementation needs to validate the argument count, it appears. When you have made this fail to compile, please run the same command as I described for the other test.
There was a problem hiding this comment.
Removed this one because this is a runtime error that only get triggered when we actually generate some SQL entities. Therefore it does not fit as a ui test.
There was a problem hiding this comment.
Oh okay! I sometimes don't remember what actually happens during compilation and what is technically """runtime""". I'd like to add schema-gen testing to the UI work, but I think that may need a burlier library than trybuild to do it, so it's definitely a separate issue. Thanks!
| if self.fn_args.len() != 1 { | ||
| return Err(eyre!( | ||
| "PG cast function must have exactly one argument, got {}", | ||
| self.fn_args.len() | ||
| )); | ||
| } |
There was a problem hiding this comment.
Somehow, this does nothing. I'm not sure what's going on there, honestly. My "essentially just instinct" guess is that this is somehow skipped over outright during building the example function used, and the closest reason why seems to be that the else if let Some was missed?
Is it correct for it to be an else if let?
There was a problem hiding this comment.
Updated to if let. FWIW, this does do what we expect (output below), it's just this is only triggered at SQL generation time (see the other comment in the UI test).
#[pg_cast]
pub fn cast_function() -> bool {
true
}
Finished dev [unoptimized + debuginfo] target(s) in 3.28s
Discovering SQL entities
Discovered 2 SQL entities: 0 schemas (0 unique), 2 functions, 0 types, 0 enums, 0 sqls, 0 ords, 0 hashes, 0 aggregates, 0 triggers
Writing SQL entities to /dev/stdout
Error:
0: Could not write SQL to stdout
1: PG cast function (cast_function) must have exactly one argument, got 0
Location:
pgrx-sql-entity-graph/src/pg_extern/entity/mod.rs:515
The validation against the entity itself is done at runtime (during SQL
generation). Hence it will not be triggered at compile time. Here's the
output from a local test with an extension:
```
$/home/louiskuang/Documents/pgrx/target/debug/cargo-pgrx pgrx schema --features "pg13"
Using FeatureFlag("pg13") and `pg_config` from /home/louiskuang/.pgrx/13.13/pgrx-install/bin/pg_config
Building for SQL generation with features `pg13`
Compiling pgrx-sql-entity-graph v0.11.2 (/home/louiskuang/Documents/pgrx/pgrx-sql-entity-graph)
Compiling pgrx-macros v0.11.2 (/home/louiskuang/Documents/pgrx/pgrx-macros)
Compiling pgrx-pg-sys v0.11.2 (/home/louiskuang/Documents/pgrx/pgrx-pg-sys)
Compiling pgrx v0.11.2 (/home/louiskuang/Documents/pgrx/pgrx)
Compiling my_extension v0.0.0 (/home/louiskuang/Documents/my_extension)
warning: unused import: `Json`
--> src/lib.rs:1:24
|
1 | use pgrx::{prelude::*, Json};
| ^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: `my_extension` (lib) generated 1 warning (run `cargo fix --lib -p my_extension` to apply 1 suggestion)
Finished dev [unoptimized + debuginfo] target(s) in 11.21s
Discovering SQL entities
Discovered 2 SQL entities: 0 schemas (0 unique), 2 functions, 0 types, 0 enums, 0 sqls, 0 ords, 0 hashes, 0 aggregates, 0 triggers
Writing SQL entities to /dev/stdout
Error:
0: Could not write SQL to stdout
1: PG cast function (cast_function) must have exactly one argument, got 0
Location:
pgrx-sql-entity-graph/src/pg_extern/entity/mod.rs:515
```
While `pg_cast` and `pg_operator` are indeed mutually-exclusive currently (
`pg_operator` always requires a function with two parameters and `pg_cast`
always requires a function with exactly one parameter), there's no
reason why they can not both apply on the same element, say in the
future the need for non-binary operator arises.
In addition, both `pg_cast` and `pg_operator` are doing its own
validation at the moment so there's no risk of being in an impossible
state like the following:
```
pub fn foo(left: i32, right: i32) -> bool {
true
}
```
|
Thank you for the review! |
Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1
To use the new macro, add the
pg_castmacro on a single-argument single-return value Rust function:It is possible to modify the cast by adding either
assignmentorimplicitto thepg_castmacro as argument:TESTED="cargo test"