Better dependency tracking for non-sql functions

[ewan-escience]

When trying out this product with the plan command, the program errors, because it seems to do some live validation of the plan. If I understand correctly what is happening, this generates errors because:

1. some of the function are written in plpgsql, for which the program doesn't check its dependencies
2. those functions seem to be created in alphabetical order, so some functions are created before their dependencies

One of the following would solve this:

1. add flag is added that sets check_function_bodies = off before doing the validation
2. add support for dependency checking of plpgsql functions
3. don't validate the plan when the plan was created from two live databases (i.e. with both --dsn and --schema-source-dsn)

[bplunkett-stripe]

Yes, that is an unfortunate limitation of postgres. It doesn't actually build any hard dependencies with pgplsql functions, so pg-schema-diff is kind of operating in the dark. We could manually try to parse dependencies from the function but that is a lot of complexity: not only would the function itself need to be parsed, but the context of the function would need to be known (is it used in a trigger, check constraint, etc).

Plan validaiton is a confidence check. If plan validation fails, it normally implies the plan will fail against your actual database. We can definitely add an option to disable it, but I'd be curious to know if the plan validation is actually failing here, or you're okay with an invalid plan.

Thanks for the great breakdown here! I might rename the ticket to "Improve track of plpgsql functions"

[ewan-escience]

Thanks for the quick reply. I'm trying to move away from migra, as it isn't developed anymore and doesn't support diffing domains. Migra would always add set check_function_bodies = off; to its diffs, and I never had any problems with it.

I'm quite sure the plan would work, but I cannot test it, as no plan is generated at all.

How does the validation work? Is it executed in the database to upgrade? Is it isolated? Is it reverted afterwards? I would like to disable it, as we always manually check the diff anyways.

[bplunkett-stripe]

I added a flag to disable plan validation in the cli via --disable-plan-validation.

The way plan validation works:

1. Spins up a temporary database on your postgres instance
2. Places the current schema in the temporary database
3. Runs the migration
4. Re-generates a diff and asserts that the diff is empty

We unfortunately do not support diffing of domains yet. If you add a ticket, we can try to get to it, since I imagine implementing that won't be too tricky.

[peterldowns]

We could manually try to parse dependencies from the function but that is a lot of complexity: not only would the function itself need to be parsed, but the context of the function would need to be known (is it used in a trigger, check constraint, etc).

@bplunkett-stripe have you considered letting the developer manually specify dependencies for the cases that they cannot be inferred (or inferred correctly)? That's what I do in pgmigrate, and it works great.

[bplunkett-stripe]

We could manually try to parse dependencies from the function but that is a lot of complexity: not only would the function itself need to be parsed, but the context of the function would need to be known (is it used in a trigger, check constraint, etc).

@bplunkett-stripe have you considered letting the developer manually specify dependencies for the cases that they cannot be inferred (or inferred correctly)? That's what I do in pgmigrate, and it works great.

Interesting. That is also a definite possibility. Not super conducive to the CLI format unfortunately, but I'm also thinking of adding support for some sort of configuration YAML.

[peterldowns]

Yeah, has to come from a configuration file, not passable on the command line, but it's pretty common for these kinds of tools to have config files (in my experience.) You could take a look at how I parse/merge cli options and config file options here in pgmigrate or just use something like Viper, which I've been meaning to try.

btw very cool project, watching with interest, keep up the good work 🫡

[bplunkett-stripe]

Awesome! pgmigrate looks pretty neat. We use cobra already, so viper will probably slot in pretty nicely.

Hopefully can get some bigger features out soon. This project is a bit on the side-burner for me with current priorities 😢 .

[bplunkett-stripe]

This is effectively blocked by #131, which will enable us to more easily build dependencies on statements like column additions/deletions.

[aleclarson]

Migra would always add set check_function_bodies = off; to its diffs, and I never had any problems with it.

This should be an option in pg-schema-diff. It seems that SET statements are not included in the diff plan, so the plan validation fails even when I tried setting check_function_bodies = off myself.

Maybe a special filename like pre_plan.sql could let us always run certain statements on the temporary database before plan validation?

[Navbryce]

This should be an option in pg-schema-diff. It seems that SET statements are not included in the diff plan, so the plan validation fails even when I tried setting check_function_bodies = off myself.

Maybe a special filename like pre_plan.sql could let us always run certain statements on the temporary database before plan validation?

The problem you're trying to solve for here is there are certain schema objects not yet supported by pg-schema-diff but referenced by your functions?

[aleclarson]

@Navbryce Yeah exactly. I just tried adding a !pre-plan.sql file to my schema dir (named with ! to ensure it's first in the ddlSchemaSource.ddl array). I confirmed that it is running the set check_function_bodies statement, but it has no effect (still see the same error).

edit: The above is to be expected, because my issue is occurring while the migration plan is being run, not while the new schema is being retrieved from the temporary database.

edit 2: Everything in my comment before this edit is a bit outdated. More context can be found here: #168 (comment)