Change to after insert for trigger #2
Conversation
What: Changed function `audit.enable_tracking` to use after insert trigger. Why: In case of using `ON CONFLICT DO UPDATE` as an upsert mechanism, for a conflicting record, the before insert trigger receives 3 events: - INSERT - UPDATE - UPDATE This makes 3 records in audit table and it's not what we want. The change to after insert makes sure that record operation succeeds first and only then emits an event for audit table. In this case it's only one: UPDATE.
WalkthroughThe SQL migration updates the audit trigger function to return Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
drizzle/0002_classy_war_machine.sql (1)
88-90: Index definition uses the wrong column – will not speed upold_record_idlook-upsThe second index is declared as
create index record_version_old_record_id on audit.record_version (record_id) where old_record_id is not null;It indexes
record_idagain instead ofold_record_id, so queries filtering byold_record_idwon’t benefit.-create index record_version_old_record_id on audit.record_version (record_id) +create index record_version_old_record_id on audit.record_version (old_record_id)Recommend correcting before this migration is applied in production, otherwise you’ll need a follow-up migration to drop & recreate the index.
🧹 Nitpick comments (1)
drizzle/0002_classy_war_machine.sql (1)
160-167: Consider schema-qualified table reference in dynamic CREATE TRIGGER statement
%Ion aregclassargument expands only the relation name, relying onsearch_pathto resolve the schema.
If consumers callaudit.enable_tracking('other_schema.my_table'::regclass)whilesearch_pathlater points to a different schema containing a table with the same name, the trigger could be installed on the wrong relation.You can make the SQL robust by extracting schema + relname and quoting both:
-declare - statement_row text = format(' - create trigger audit_i_u_d - after insert or update or delete - on %I +declare + statement_row text = format(' + create trigger audit_i_u_d + after insert or update or delete + on %I.%I for each row execute procedure audit.insert_update_delete_trigger();', - $1 + (select nspname from pg_class c join pg_namespace n on n.oid = relnamespace where c.oid = $1), + (select relname from pg_class where oid = $1) );This keeps behaviour identical while eliminating any
search_pathdependency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
drizzle/0002_classy_war_machine.sql(2 hunks)
🔇 Additional comments (1)
drizzle/0002_classy_war_machine.sql (1)
136-137: Return value for AFTER trigger is now correct – looks goodFor
AFTER-row triggers the return value is ignored, so changing the body toreturn null;is the safest choice.
No further action required.
| old_record_jsonb; | ||
|
|
||
| return coalesce(new, old); | ||
| return null; |
There was a problem hiding this comment.
"After insert" trigger doesn't need to return anything, since the record was already written.
2 participants
What:
Changed function
audit.enable_trackingto use after insert trigger.Why:
In case of using
ON CONFLICT DO UPDATEas an upsert mechanism, for a conflicting record, the before insert trigger receives 3 events:This makes 3 records in audit table and it's not what we want.
The change to after insert makes sure that record operation succeeds first and only then emits an event for audit table. In this case it's only one: UPDATE.
Summary by CodeRabbit