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

feat: add support for pg procedure #143

Closed
wants to merge 1 commit into from
Closed

Conversation

sileht
Copy link

@sileht sileht commented Jan 14, 2025

Some use-cases require to use pg procedure instead pg function , for
example a function that need to manage the transaction (use of
begin/commit/rollback).

This change adds support for them

Fixes #62

@sileht sileht marked this pull request as ready for review January 14, 2025 16:10
Some use-cases require to use pg procedure instead pg function , for
example a function that need to manage the transaction (use of
begin/commit/rollback).

This change adds support for them

Fixes olirice#62
@olirice
Copy link
Owner

olirice commented Jan 14, 2025

Thanks for opening the PR

Function support has a lot of limitations. At this point I'm not comfortable expanding into procedures which may introduce new incompatibilities

If this is sufficient for your own use I'd recommend copying the PGProcedure class into your target project

Sorry for that response, I can see you put a lot of work into it and the PR itself looks great

Happy to discuss these things in Issues before you open a PR in the future

@olirice olirice closed this Jan 14, 2025
@sileht
Copy link
Author

sileht commented Jan 14, 2025

Hi @olirice, don't worry I understand.

We (mergify.com) are very happy user of alembic_utils. We already use this change our self, and we wanted to share it with the community.

I would be happy to know what the limitations of PGFunction are?

From what I saw, the "fragile" part looks like the use of parse library to split the function into signature/body that may not 100% reliable, overwise thing looks good (at least for recent version of PG ;) )

@olirice
Copy link
Owner

olirice commented Jan 14, 2025

thanks

the parse part + the pre-processing part here

is_plpgsql: bool = "language plpgsql" in normalize_whitespace(definition).lower().replace(
"'", ""
)
escaping_callable = escape_colon_for_plpgsql if is_plpgsql else escape_colon_for_sql
# Override definition with correct escaping rules
self.definition: str = escaping_callable(strip_terminating_semicolon(definition))

differences between sql/plpgsql were initially an issue. It seemed for a while that other languages were going to become a big deal (plv8 and/or plrust) but plv8 doens't support pg17 and plrust hasn't seen wide adoption outside RDS yet so that hasn't turned out to be a big deal.

probably just being overly cautious b/c I don't have the bandwidth to dive into issues if something goes wrong with a new feature

really glad to hear its working well for you!

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.

PGProcedure
2 participants