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.

Sign up for GitHub

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

Jump to bottom

chore(electric): Clean up templated SQL routines #511

Merged
merged 9 commits into from
Jan 8, 2024
Merged

chore(electric): Clean up templated SQL routines #511

merged 9 commits into from
Jan 8, 2024

Conversation

alco
Copy link
Member

@alco alco commented Sep 29, 2023 •
edited
Loading

This is a cleanup PR that includes the following changes:

  • Remove some of the dead SQL code from Extension migration modules. The code used to define routines but they are now defined at run time.
  • Import the Extension module into Functions to make all functions from the former available automatically to all templated SQL routines. This removes the need to maintain a map of assigns by hand.
  • Fix the wildcard pattern in Functions to include subdirectories. Prior to this change, the functions from lib/electric/postgres/extension/functions/ddlx/ weren't included in the module.

@alco alco force-pushed the alco/vax-1100-check-for-default-column-clause branch 2 times, most recently from ff0c722 to 1a93b31 Compare September 30, 2023 20:43
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch from 34d5a5b to 065abdd Compare October 3, 2023 10:25
@alco alco force-pushed the alco/vax-1100-check-for-default-column-clause branch from 1a93b31 to d7217db Compare October 16, 2023 09:54
Base automatically changed from alco/vax-1100-check-for-default-column-clause to main October 16, 2023 12:45
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch 2 times, most recently from 7ce86cc to 9a7f2b8 Compare October 30, 2023 21:34
@alco alco changed the title Alco/define sql functions at run time chore(electric): Clean up templated SQL routines Oct 30, 2023
@alco alco requested a review from magnetised October 30, 2023 21:41
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch from 9a7f2b8 to 74a5da9 Compare October 30, 2023 21:45
@alco alco marked this pull request as ready for review October 30, 2023 21:45
magnetised
magnetised approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😓 thanks for the hard work on this one! good changes all in all, but I do question the need for the functions-as-functions approach considering the required lifetime of these blobs of rendered sql.

components/electric/lib/electric/postgres/extension/functions.ex Outdated Show resolved Hide resolved
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch from 7fa9214 to a4eab72 Compare December 22, 2023 22:38
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch from 70b4700 to d81b331 Compare January 3, 2024 14:52
@alco alco requested a review from magnetised January 8, 2024 09:44
magnetised
magnetised approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lovely stuff. Shame git didn't track the renames.

...ic/lib/electric/postgres/extension/migrations/20230512000000_conflict_resolution_triggers.ex
@@ -19,11 +19,6 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230512000000_confli
def up(schema) do
[
@contents["electric_tag_type_and_operators"],
@contents["utility_functions"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing that this is ok and referencing functions that don't exist is not a problem -- if so then good to get rid of the duplication

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the context. If you have a function that calls another function, it is fine for the former to be defined before the latter.

On the other hand, if you have a trigger definition that references a function, the function has to be defined prior to the evaluation of the trigger def.

alco added 7 commits January 8, 2024 15:25
@alco
Remove leftover SQL function/procedure definitions from old migrations
126dff4 
The functions/procedures are now defined at run time.
@alco
Clean up templated SQL routines
99b1424
@alco
Disable SQL function definitions in Extension tests
088868e
@alco
Prettify the log output of SQL routine definitions
3b3c8ed 
Log sample:

    00:43:08.719 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'capture_ddl.sql.eex'
    00:43:08.720 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'current_transaction_id.sql.eex'
    00:43:08.720 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'current_xact_id.sql.eex'
    00:43:08.721 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'current_xact_ts.sql.eex'
    00:43:08.723 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'ddlx/assign.sql.eex'
    00:43:08.724 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'ddlx/disable.sql.eex'
    00:43:08.725 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'ddlx/enable.sql.eex'
    00:43:08.725 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'ddlx/grant.sql.eex'
    00:43:08.726 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'ddlx/unassign.sql.eex'
    00:43:08.727 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'electrify.sql.eex'
    00:43:08.728 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'electrify/__validate_table_column_defaults.sql.eex'
    00:43:08.728 pid=<0.1285.0> origin=postgres_1 [debug] Successfully (re)defined SQL routine from 'electrify/__validate_table_column_types.sql.eex'
    00:45:22.624 pid=<0.1285.0> origin=postgres_1 [error] initialization for postgresql failed with reason: {:rollback, %RuntimeError{message: "Failed to define SQL routine from 'electrify.sql.eex' with error: {:error, {:error, :error, \"42601\", :syntax_error, \"syntax error at or near \\\"SELECD\\\"\", [file: \"scan.l\", line: \"1176\", position: \"243\", routine: \"scanner_yyerror\", severity: \"ERROR\"]}}"}}
@alco
Load SQL function templates on-demand at run time
d04e534
@alco
Move SQL function templates into priv/
ebd078d 
...so that they are available in the packaged Elixir release to be
loaded and evaluated at run time.
@alco
Remove empty lines from SQL function templates
1b10be8
@alco alco force-pushed the alco/define-sql-functions-at-run-time branch from d81b331 to 1b10be8 Compare January 8, 2024 13:34
@alco alco merged commit b197c3b into main Jan 8, 2024
5 checks passed
@alco alco deleted the alco/define-sql-functions-at-run-time branch January 8, 2024 14:18
msfstef pushed a commit that referenced this pull request Jan 15, 2024
@alco @msfstef
chore(electric): Clean up templated SQL routines (#511)
5991759 
This is a cleanup PR that includes the following changes:
* Remove some of the dead SQL code from Extension migration modules. The
code used to define routines but they are now defined at run time.
* Import the Extension module into Functions to make all functions from
the former available automatically to all templated SQL routines. This
removes the need to maintain a map of assigns by hand.
* Fix the wildcard pattern in Functions to include subdirectories. Prior
to this change, the functions from
`lib/electric/postgres/extension/functions/ddlx/` weren't included in
the module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magnetised magnetised magnetised approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alco @magnetised