feat: extend sql generation via to_sql attribute #410
Conversation
|
Generally a fan of this idea, like the @eeeebbbbrrrr and I have just introduced a I'll give this a spin. :) |
|
@Hoverbear Thanks! I can certainly rework this to use the As an aside, I'm not really sure why the one test is failing, but will try to reproduce and track that down today. As far as I can tell it shouldn't be affected by the changes, but it might just be something subtle with the change to the |
|
I'm really excited about the direction this PR takes us, as well as the PR. :) |
bitwalker
force-pushed
the
bitwalker/to_sql-attribute
branch
from
abebee0
to
f5e89cf
Compare
|
I was thinking more about this PR and we have a /// ```pgx_sql
/// CREATE FUNCTION ...
/// ```
#[pg_extern]
function dooper() {}Could become #[pg_extern(sql = "
CREATE FUNCTION ...
")]
function dooper() {}So I wonder if we should change |
bitwalker
force-pushed
the
bitwalker/to_sql-attribute
branch
from
f5e89cf
to
6e20553
Compare
|
@Hoverbear I've made your recommended changes, and added a WIP commit that merges the The only thing with the merge that changes behavior, is that previously, |
| pub content: Option<&'static str>, | ||
| } | ||
| impl ToSqlConfigEntity { | ||
| /// Given a SqlGraphEntity, this function converts it to SQL based on the current configuration. |
There was a problem hiding this comment.
Thank you so much for doc comments. :)
I'm not jazzed about it b/c then I'll have to make changes to ZomboDB, but I suppose it's better and more consistent with how Rust would want us to do things. |
|
@eeeebbbbrrrr I will personally make the changes to ZDB for you if that helps. :) |
bitwalker
force-pushed
the
bitwalker/to_sql-attribute
branch
2 times, most recently
from
571d3b0
to
69acff1
Compare
|
@Hoverbear Any recommendations on getting to the bottom of the test failures? I can't seem to get any kind of useful output explaining what's going on either locally or in the CI runs, and I'm not sure why. As best I can tell, something in the test framework is swallowing output, but I haven't been able to find where yet. Have you run into this before? My latest local run looks like this: |
|
@bitwalker I have encountered this too, try |
|
@Hoverbear I think once this PR gets merged it'll stop hassling you, I hope haha. At least it looks like at least this run is finally going to be green 🤞 |
|
Yessssss!!! I'll give it a final lookover and get it merged. Thanks. :) |
pgx-tests/src/tests/schema_tests.rs
Outdated
| #[pg_extern(sql = false)] | ||
| fn func_elided_from_schema() {} | ||
|
|
||
| #[pg_extern(sql = "generate_function")] |
There was a problem hiding this comment.
It should actually be valid to do like #[pg_extern(sql = generate_function)] which would remove any ambiguity about if it's SQL or a function...
There was a problem hiding this comment.
Good point, I think that makes more sense, I'll adjust it accordingly
There was a problem hiding this comment.
@Hoverbear I just realized that the reason I used a string literal here, is that MetaNameValue expects the value part of key = value to be a syn::Lit. Your suggestion isn't supported AFAIK, unless you choose to avoid Attribute::parse_meta for parsing the attribute contents
There was a problem hiding this comment.
Hmmm I've seen this before in https://docs.rs/tracing/latest/tracing/attr.instrument.html#examples-1 ...
There was a problem hiding this comment.
I guess part of my concern (and want for that distinction) is I could very well see a bug where this code:
Parses some SQL as a Rust Path for some reason, and creates weird behavior.
There was a problem hiding this comment.
Yeah in that case, they are abandoning Attribute::parse_meta in favor of Attribute::parse_args, which allows them to use their own grammer for the attribute contents by manually parsing them. It's more effort but worth it when you need to support something that doesn't fit into the standard structured meta arguments approach. We can do something similar if you think it's worth it.
There was a problem hiding this comment.
I would definitely prefer a lack of ambiguity that if you have time, I know I've already asked a lot of you so if you are pressed on time I can wrap it up.
There was a problem hiding this comment.
I should have a commit that implements this ready shortly, just FYI
|
Once you're happy with this, just let me know and I'll squash commits, just leaving them broken up while we iterate on things |
|
Newest commit 344f685 looks good to me. :) |
|
Awesome! Once the tests pass, I'll go ahead and squash then :) |
This commit extends the `#[pgx]` and `#[pg_extern]` attributes with a new `sql` argument type that allows for customizing the behavior of SQL generation on certain entities in three different ways: 1. `#[pgx(sql = false)]` disables generation of the decorated item's SQL 2. `#[pgx(sql = path::to::function)]` delegates responsibility for generating SQL to the named function 3. `#[pgx(sql = "RAW SQL CODE;")]` uses the provided SQL string in place of the SQL that would have been generated by default In the 2nd case, the function has almost the same signature as the `to_sql` function of the built-in `ToSql` trait, except the function receives a reference to a `SqlGraphEntity` in place of `&self`. This allows for extending any of the `SqlGraphEntity` variants using a single function, as trying to use specific typed functions for each entity type was deemed overly complex. This also works well with the way `ToSql` is invoked today, which starts by calling the implementation on a value of type `SqlGraphEntity`, so we can perform all the checks in a single place. As an aside, these custom callbacks can still delegate to the built-in `ToSql` trait if desired. For example, if you wish to write the generated SQL for specific entities to a different file. The motivation for this is that in some edge cases, it is desirable to elide or modify the SQL being generated. In our case specifically, we need to chop up the generated SQL so that we can split out non-idempotent operations separately from idempotent operations in order to properly manage extension upgrades. Rather than lose the facilities pgx provides, the `#[pgx(sql)]` attribute allows us to make ad-hoc adjustments to what, when and where things get generated without needing any upstream support for those details.
bitwalker
force-pushed
the
bitwalker/to_sql-attribute
branch
from
344f685
to
f653be4
Compare
| LANGUAGE c /* Rust */\n\ | ||
| AS 'MODULE_PATHNAME', '{unaliased_name}_wrapper';\ | ||
| ", | ||
| unaliased_name = func.unaliased_name, |
There was a problem hiding this comment.
😅 I'm gonna need to clean up this API a bit.
There was a problem hiding this comment.
I had a similar thought when I was thinking through how to expose stuff to those functions haha. In my perfect world, users of this would get access to a hypothetical SqlBuilder API that let you replicate this using the builder pattern:
fn generate_function(entity: &SqlGraphEntity, builder: &mut SqlBuilder, _context: &PgxSql) -> Result<(), Box<dyn std::error::Error + Send + Sync + 'static>> {
if let SqlGraphEntity::Function(ref f) = entity {
// get_or_create lets you modify existing entities after initial construction, but before the builder is consumed
// returns a entity builder that holds a mutable reference to the parent builder
// when the entity is finalized, only then is the entity added to the parent builder
let mut func = builder.get_or_create_function(f.name);
func
.args(&[]) // defaults to no arguments if not set
.returns("void") // defaults to 'void' if not set
.replace(true) // indicates the builder should use CREATE OR REPLACE
.lang(FunctionLanguage::Rust) // lowers to LANGUAGE c /* Rust */
.as_nif("MODULE_PATHNAME", format!("{}_wrapper", func.unaliased_name)) // indicates the function is a Natively Implemented Function from the given module
.build()?; // adds the built function to the parent builder
Ok(())
} else {
panic!("expected extern function entity, got {:?}", entity);
}
}It'd be a big undertaking to cover all possible entities, but could probably start by just covering the cases that pgx itself requires, expose some kind of escape hatch akin to extension_sql!, and expand the API with specific entities as needed. Not sure if it is worth it or not, but would be a lot nicer and less error prone to work with!
|
I think my only remaining nit about this PR is we don't leave an comment anchor in the SQL noting that, for example, the content was overridden or skipped. I think I'll make a follow up PR for this since I've asked enough of you. :) |
|
@Hoverbear Yeah I think that would be fairly easy to add. I also think it'd be nice to ensure some easy mistakes are caught by pgx when using SQL strings returned from the callback or given as a literal, e.g. that any non-comment lines are terminated with a semicolon. I didn't add any such checks, since I wasn't sure what degree of validation we would want (though I did make an effort to ensure newlines are prepended when rendering the strings to help readability of the final output), but it is almost certainly something that will help avoid unnecessary bug reports from users. I agree though that it probably makes sense to merge this and make some of those improvements in follow-on PRs just to keep things tighter for review, just let me know what you need from me, and I'll try to make some time :) |
|
Let's merge this as it is now, I'm just prepping a follow up for the anchor comments as I had a particular way I wanted to do it. :) |
/cc @Hoverbear @JamesGuthrie
This PR was spawned from some needs we have over at Timescale in our Promscale extension. I looked into #377 as an option, but found that due to expectations the
ToSqltrait has about the entity graph, trying to manipulate entities at that stage is delicate at best. For example, you can't conditionally elide certain entities from SQL generation by removing them from the graph, because things like types are expected to exist in the graph during lowering. To make it work, you'd essentially need to reconstruct the graph within the hook to join the incoming and outgoing neighbors of those entities so that the dependency ordering is preserved without those entities being present in the output. It feels like that is exposing too much of the pgx internals implicitly, in a way that can break non-obviously and will always be fragile to internal changes.So as an alternative, this PR introduces an approach that I think you'll like, and is less invasive while still providing quite a bit of flexibility around how entities get lowered to SQL. It works great for our needs, and others that are in a similar boat, so I'm hoping it is something you all are open to merging!
This commit introduces a new
#[to_sql]attribute that allows forcustomizing the behavior of SQL generation on certain entities in two
different ways:
#[to_sql(false)]disables generation of the decorated item's SQL#[to_sql(path::to::function)]delegates responsibility forgenerating SQL to the named function
In the latter case, the function has almost the same signature as the
to_sqlfunction of the built-inToSqltrait, except the functionreceives a reference to a
SqlGraphEntityin place of&self. Thisallows for extending any of the
SqlGraphEntityvariants using a singlefunction, as trying to use specific typed functions for each entity type
was deemed overly complex. This also works well with the way
ToSqlisinvoked today, which starts by calling the implementation on a value of
type
SqlGraphEntity, so we can perform all the checks in a singleplace.
As an aside, these custom callbacks can still delegate to the built-in
ToSqltrait if desired. For example, if you wish to write thegenerated SQL for specific entities to a different file. One thing that might make
this even more powerful is if the lowering is by value (and also receives a mutable
reference to the
PgxSqlcontext), as this would allow custom functions tomodify certain attributes of the entities during lowering (as a simple exercise,
consider how one would modify the naming scheme used by the generator without
breaking dependents of the lowered entity - currently that isn't possible without taking
over almost all of the lowering yourself). Not super important I don't think, but something
to mull over.
The motivation for this is that in some edge cases, it is desirable to
elide or modify the SQL being generated. In our case specifically, we
need to chop up the generated SQL so that we can split out non-idempotent
operations separately from idempotent operations in order to properly
manage extension upgrades. Rather than lose the facilities pgx provides,
the
#[to_sql]attribute allows us to make ad-hoc adjustments to what,when and where things get generated without needing any upstream support
for those details.