oracle / quicksql

A library for generating DDL SQL and entity-relationship-diagrams from Quick SQL code
Universal Permissive License v1.0
53 stars 11 forks source link

QuickSQL should generate compound triggers by default #25

Open RichardSoule opened 1 year ago

RichardSoule commented 1 year ago

Originally, this was suggested in the APEX Ideas application: https://apexapps.oracle.com/pls/apex/r/apex_pm/ideas/details?idea=FR-3332 Below is the gist, and some of the information from the thread there.

Idea Summary

Right now QuickSQL generates non-compound triggers with names like “employees_biu” (employees before insert update) leading folks down the path of creating another trigger like “employees_aiu” (employees after insert update). Generally a table should really only have a single trigger (yes, if you are using Edition Based Redefinition you'd have two triggers). Since compound triggers were introduced in Oracle 11g, that single trigger should be a compound trigger.

Use Case

Compound triggers have many advantages over multiple triggers including global declarations and a specific firing order, but by far the biggest advantage is a single place to look for trigger logic.

When a trigger in QuickSQL is generated, it should look something like the below:

create or replace trigger <trigger-name> --example: employee_compound_trigger
  for insert or update or delete on <table-name>
    compound trigger

  -- global declarations
  -- g_global_variable varchar2(10);

  before statement is
  begin
    null; -- do something here.
  end before statement;

  before each row is
  begin
    null; -- do something here.
  end before each row;

  after each row is
  begin
    null; -- do something here.
  end after each row;

  after statement is
  begin
    null; -- do something here.
  end after statement;

end <trigger-name>;
/

There are no benefits to using separate triggers, only detriments. (My opinion.)

If you really, really want the ability to have QuickSQL generate separate triggers, you could generate compound triggers by default (as one should ;) ) and provide an option of “Add complexity to our implementation by implementing triggers as separate triggers, but realize that if you need to share state or variable information between those triggers you'll have to put that information into packages that will need to be in scope for the triggers”. Or you could just label it “Use legacy triggers”.

Steven Feuerstein chimed in with the following:

I love this idea - compound triggers have been around since 11g.

Basically, if you agree that all procs and funcs should be inside packages and not “stand-alone”, the very same argument applies to individual triggers vs compound.

vadim-tropashko commented 1 year ago

Could you please provide an example where more than one trigger is generated for the same table? In current JS implementation trigger functionality is aggregated undertable_name_bui, e.g.

dept /rowversion
    name /lower

which generates

-- create tables

create table dept (
    id             number generated by default on null as identity
                   constraint dept_id_pk primary key,
    name           varchar2(255 char),
    row_version    integer not null
);

-- triggers
create or replace trigger dept_biu
    before insert or update
    on dept
    for each row
begin
    :new.name := lower(:new.name);
    if inserting then
        :new.row_version := 1;
    elsif updating then
        :new.row_version := NVL(:old.row_version, 0) + 1;
    end if;
end dept_biu;
/

-- Generated by Quick SQL...
RichardSoule commented 1 year ago

Vadim,

Yes, the current implementation creates a single trigger. But the trigger that it creates is a ‘classic trigger’ rather than a compound trigger. The name of the trigger has an _biu at the end. This would encourage anyone who wanted logic to after after insert or update to create a second _aiu trigger. What I’m suggesting is we encourage the better behavior of a single trigger for each table, as a compound trigger, with the ‘extra’ spots commented out.

Does that make sense? I tried to communicate that previously, I’m sorry it wasn’t clear.