search

teoljungberg / fx

Versioned database functions and triggers for Rails
MIT License
762 stars 76 forks source link

Support for `SET LOCAL check_function_bodies` #134

Closed cmer closed 6 months ago

cmer commented 1 year ago

Added a new configuration parameter check_function_bodies to allow controlling the database behavior when loading functions.

This is necessary if the function created relies on the existence of a table that is not yet created, for example.

Disabling check_function_bodies is pg_dump's default behaviour and, in my opinion, should be Fx's default behavior as well.

cmer commented 1 year ago

I just realized there's a small issue with my implementation. I marked this PR as DRAFT while I resolve it.

cmer commented 1 year ago

This is now ready for review. Thank you!

teoljungberg commented 1 year ago

I've been meaning to cut a release for fx to get the last commits out, let me do that before reviewing this thoroughly and considering if this is something we want supported.

At a glance, I'd like if the new behavior is done per adapter. As we have 3 different adapters that might not support this behavior (I have not checked, just guessing).

With that said, and for transparency - I haven't considered if this is something I'd want to support in fx yet or not.

cmer commented 1 year ago

Thanks a lot.

The problem is that without this, the order in schema.rb matters. For example, I have functions that rely on some tables existing, but they appear after in schema.rb. I also have tables that rely on custom functions, so I can't move my functions to the end of my schema file.

I have been using my fork and it solved my issue. I'm assuming my code is Postgres-only.

teoljungberg commented 1 year ago

Gotcha, thanks for context. Regardless if we do this we'd need to support multiple adapters and make this postgres-agnostic to the extent we can.

Ordering of these schema files is an issue that has hunt this project as well as Scenic since their beginning. Topological sorting would be something I would love to have.

cmer commented 1 year ago

I made it configurable, but IMO this should be the default behavior and probably shouldn't even be configurable?

teoljungberg commented 6 months ago

If we’d consider this I’d go with #135, closing this one.