Skip to content

Add CREATE TRIGGER support for SQL Server #1810

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aharpervc
Copy link
Contributor

@aharpervc aharpervc commented Apr 11, 2025

Adjacent to: #1808 with similar considerations, and temporarily rebased on it (eg, this branch should probably wait for that branch to merge).


This PR introduces support for parsing CREATE TRIGGER for SQL Server.

The main concern is that for the existing dialects, there was an expectation of an EXECUTE keyword (PG: https://www.postgresql.org/docs/current/sql-createtrigger.html). However, SQL Server doesn't use this syntax and instead supports multi statement blocks (like a stored procedure).

The difficulty here in the codebase is what to do about CreateTrigger.exec_body & TriggerExecBody. In this iteration I made the property an Option, which seemed like the least impact on existing code. TriggerExecBody, etc can be left alone.

However in the future I think this could be improved with a more broad refactoring, such as a TriggerBody enum, which can consolidate TriggerExecBodyType's options & use Vec<Statements> for a MultiStatement variant and FunctionDesc for Function & Procedure variants. That'd basically remove the TriggerExecBody struct, too. Overall I think that'd be a cleaner approach for the CreateTrigger struct.


That's all speculative followup work, for now I just want to be able to parse most SQL Server triggers. Other SQL Server specific things like particular trigger options, DDL trigger stuff, etc can come later as needed.

@aharpervc aharpervc force-pushed the mssql-create-trigger branch from 39517ce to b94916f Compare April 11, 2025 21:30
@aharpervc aharpervc marked this pull request as ready for review April 11, 2025 22:00
@aharpervc aharpervc force-pushed the mssql-create-trigger branch from b94916f to bd6d624 Compare April 14, 2025 16:13
@aharpervc
Copy link
Contributor Author

aharpervc commented Apr 14, 2025

This fails to parse on this branch, but should:

CREATE TRIGGER some_trigger ON some_table FOR INSERT
AS
BEGIN
    IF 1=1
    BEGIN
        RAISERROR('Trigger fired', 10, 1);
    END

    RETURN;
END

Fixed, never mind

@aharpervc
Copy link
Contributor Author

FYI for reviewers I rebased this on the create function branch, due to the return statement logic here: #1808 (comment)

@aharpervc aharpervc force-pushed the mssql-create-trigger branch from bd6d624 to aaa2ab3 Compare April 14, 2025 19:53
@aharpervc aharpervc force-pushed the mssql-create-trigger branch 6 times, most recently from 77776fc to d5d376e Compare April 21, 2025 18:54
- similar to functions & procedures, this dialect can define triggers with a multi statement block
- there's no `EXECUTE` keyword here, so that means the `exec_body` used by other dialects becomes an `Option`, and our `statements` is also optional for them
@aharpervc aharpervc force-pushed the mssql-create-trigger branch from d5d376e to d2b564d Compare April 23, 2025 19:41
@aharpervc
Copy link
Contributor Author

Rebased again now that #1808 has been merged. Should be ready for review.

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.

1 participant