Implement compilation of triggers #5088

msullivan · 2023-02-23T03:01:34Z

Closes #4936.

The interesting part of the compilation runs at the end of IR->SQL and
injects the triggers into CTEs. The overlay system is (somewhat hackily)
used to drive what objects are triggered on and (slightly less hackily) to
provide a view of the new database state.

I have one known bug at the moment, which is approximately that we apply
access policies too much in one case. I have a plan for fixing it that
doesn't change that much of the big picture, and I wanted to get something
up for review.

elprans · 2023-02-23T04:26:58Z

edb/schema/inheriting.py

@@ -98,10 +98,15 @@ def inherit_fields(
        inherited_fields_update = {}
        deferred_complex_ops = []

-        for field_name in field_names:
+        # Iterate over mcls.get_schema_fields() instead of field_names for


Why is this important?

Compilation of the trigger expression depends on the values of kind and scope, and so we need to arrange to inherit those before we try to inherit expr

edb/edgeql/compiler/stmtctx.py

edb/pgsql/ast.py

elprans

👍

aljazerzen

Looks good!

I still have to re-read the pg compiler side with a fresh set of brain, but I can do that even after the merge.

aljazerzen · 2023-02-23T18:43:11Z

edb/ir/ast.py

@@ -1025,7 +1026,22 @@ class GroupStmt(FilteredStmt):
    ] = ast.field(factory=dict)


-class MutatingStmt(Stmt):
+# XXX: DOC
+class MutatingLikeStmt(Expr):


Suggested change

class MutatingLikeStmt(Expr):

class MutatingBaseStmt(Expr):

Eh, I like MutatingLike more, since it's not really a MutatingBase

edb/edgeql/compiler/stmt.py

aljazerzen · 2023-02-23T18:48:22Z

edb/edgeql/compiler/triggers.py

+    kinds = set(trigger.get_kinds(schema))
+    source = trigger.get_subject(schema)
+
+    with ctx.detached() as _, ctx.newscope(fenced=True) as sctx:


How is using ctx.detached() different then wrapping the expr into a qlast.DetactedExpr? Because this is what we do for defaults (and now rewrites):

https://github.com/edgedb/edgedb/blob/master/edb/edgeql/compiler/viewgen.py#L711-L746

DetachedExpr will trigger a new detached context when processing. I think the main difference is I'm just doing it directly

aljazerzen · 2023-02-23T18:55:20Z

edb/edgeql/compiler/triggers.py

+        # Process all the types, starting with the base type
+        for subtype in sorted(stypes, key=lambda t: t != stype):
+            for trigger in subtype.get_relevant_triggers(kind, schema):
+                mro = (trigger, *trigger.get_ancestors(schema).objects(schema))


Does the get_ancestors guarantee mro order? This should probably be documented somewhere.

Yeah, it does, though we don't actually care here.

Though also: #5118 😢

aljazerzen · 2023-02-23T19:02:05Z

edb/edgeql/compiler/triggers.py

+    for stmt in dml_stmts:
+        kind = TRIGGER_KINDS[type(stmt)]
+
+        stype = schemactx.concretify(


What does this do? I cannot gather from the function documentation. What are "views" in this context?

"view" is what "shape" used to be called, and which it is still sometimes called internally. But more generally, all of the transient subtypes we generate during compilation.

concretify finds non-view concrete base types, but it does it recursively through unions and intersections.

Arguably this should be the normal thing? Idk.

aljazerzen · 2023-02-23T19:14:20Z

edb/edgeql/compiler/triggers.py

+                # N.B: If the *base type* of the DML appears, that suffices,
+                # because it covers everything, and we don't need to duplicate.
+                if (stype, stmt) not in tmap:
+                    tmap.add((subtype, stmt))


I think this could be "If any of the previously handled triggers of ancestors appears, ...

This would have effect when you have a trigger on Child1 in hierarchy Updated > Child1 > Child2.
You would first add the trigger on Child1, but when processing Child2, the trigger would be added again, even though it's effects have already been covered.

(I may not be understanding this whole chunk correctly)

Yeah, it is a specific interaction with how the pgsql/dml.py side is implemented. I'm elaborating a bit more in the comment.

…5088)

Closes #4936. The interesting part of the compilation runs at the end of IR->SQL and injects the triggers into CTEs. The overlay system is (somewhat hackily) used to drive what objects are triggered on and (slightly less hackily) to provide a view of the new database state. I have one known bug at the moment, which is approximately that we apply access policies *too much* in one case. I have a plan for fixing it that doesn't change that much of the big picture, and I wanted to get something up for review.

Track deletes also, and track all types of dml in the same way, at the in fini_stmt. This requires more logic where we consume the data when compiling conflicts, but the data structure logic is more consistent. This also lets us drop the search for DML for triggers.

…5088)

Closes #4936. The interesting part of the compilation runs at the end of IR->SQL and injects the triggers into CTEs. The overlay system is (somewhat hackily) used to drive what objects are triggered on and (slightly less hackily) to provide a view of the new database state. I have one known bug at the moment, which is approximately that we apply access policies *too much* in one case. I have a plan for fixing it that doesn't change that much of the big picture, and I wanted to get something up for review.

Adds a test for #5241 which was fixed slightly by #5088 while implementing triggers. I'll cherry-pick the relevant commit and then this.

…5088)

Adds a test for #5241 which was fixed slightly by #5088 while implementing triggers. I'll cherry-pick the relevant commit and then this.

msullivan requested review from aljazerzen and elprans February 23, 2023 03:01

msullivan force-pushed the triggers branch from ec561a5 to 3a40a53 Compare February 23, 2023 04:00

elprans reviewed Feb 23, 2023

View reviewed changes

edb/edgeql/compiler/stmtctx.py Outdated Show resolved Hide resolved

elprans reviewed Feb 23, 2023

View reviewed changes

edb/pgsql/ast.py Show resolved Hide resolved

elprans approved these changes Feb 23, 2023

View reviewed changes

msullivan force-pushed the triggers branch from 3a40a53 to 697003a Compare February 23, 2023 06:38

aljazerzen approved these changes Feb 23, 2023

View reviewed changes

msullivan added 3 commits March 1, 2023 11:36

Iterate in inherit_fields in a deterministic way (#5088)

e6478ed

Move external_rels to ctx from env (#5088)

6e9bf72

Expand get_nearest_dml_stmt function call hack to be more permissive (#…

68f4db8

…5088)

msullivan force-pushed the triggers branch from 697003a to 197a8a9 Compare March 1, 2023 19:36

msullivan added 2 commits March 1, 2023 11:53

msullivan force-pushed the triggers branch from 197a8a9 to 6488172 Compare March 1, 2023 19:53

msullivan merged commit 60493f1 into master Mar 1, 2023

msullivan deleted the triggers branch March 1, 2023 20:56

msullivan added a commit that referenced this pull request Mar 1, 2023

Iterate in inherit_fields in a deterministic way (#5088)

0e716d3

msullivan added a commit that referenced this pull request Mar 1, 2023

Move external_rels to ctx from env (#5088)

e08240c

msullivan added a commit that referenced this pull request Mar 1, 2023

Expand get_nearest_dml_stmt function call hack to be more permissive (#…

6ac1ebf

…5088)

msullivan mentioned this pull request Mar 23, 2023

Add a test for assert_exists+assert_single+UPDATE #5242

Merged

msullivan added a commit that referenced this pull request Mar 23, 2023

Add a test for assert_exists+assert_single+UPDATE

3b99a42

Adds a test for #5241 which was fixed slightly by #5088 while implementing triggers. I'll cherry-pick the relevant commit and then this.

msullivan added a commit that referenced this pull request Mar 24, 2023

Add a test for assert_exists+assert_single+UPDATE (#5242)

85006e9

Adds a test for #5241 which was fixed slightly by #5088 while implementing triggers. I'll cherry-pick the relevant commit and then this.

msullivan added a commit that referenced this pull request Mar 24, 2023

Expand get_nearest_dml_stmt function call hack to be more permissive (#…

82385fa

…5088)

msullivan added a commit that referenced this pull request Mar 24, 2023

Add a test for assert_exists+assert_single+UPDATE (#5242)

f3d3178

Adds a test for #5241 which was fixed slightly by #5088 while implementing triggers. I'll cherry-pick the relevant commit and then this.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement compilation of triggers #5088

Implement compilation of triggers #5088

msullivan commented Feb 23, 2023

elprans Feb 23, 2023

msullivan Feb 23, 2023

elprans left a comment

aljazerzen left a comment

aljazerzen Feb 23, 2023

msullivan Mar 1, 2023

aljazerzen Feb 23, 2023

msullivan Mar 1, 2023

aljazerzen Feb 23, 2023

msullivan Mar 1, 2023

aljazerzen Feb 23, 2023

msullivan Mar 1, 2023

aljazerzen Feb 23, 2023

msullivan Mar 1, 2023

Implement compilation of triggers #5088

Implement compilation of triggers #5088

Conversation

msullivan commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elprans left a comment

Choose a reason for hiding this comment

aljazerzen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment