Ensure the DDL trigger requests are cached

[dyemanov]

Currently, EXE_execute_ddl_triggers() composes a temporary vector of triggers and passes it to EXE_execute_triggers(). This may cause triggers to be compiled before first execution, but the resulting statement (and possibly extTrigger) is never moved to the original (cached) copy of the triggers. So (1) every time triggers are recompiled again and again, (2) they're leaking memory and resources/locks, (3) existing calls of MET_release_triggers() for att_ddl_triggers seem useless, as these triggers are never compiled. This PR fixes this problem, but as I could be missing something, I'd appreciate a review.

[asfernandes]

I didn't found a crash nor I'm saying it makes the code worse, but the triggers code is awful.

Let see a case I suspected to be problematic.

create trigger t1 before create table position 1
as
begin
end!

create trigger t2 before create table position 2
as
begin
    in autonomous transaction do
        execute statement 'create trigger t4 before create table position 4 as begin end';
end!

create trigger t3 before create table position 3
as
begin
end!

create table tab1 (n1 integer)!

In this case it looks like it leaks in the same way as before.

But even more interesting, att_ddl_triggers is not deallocated before it's used only because its useCount goes to -1!

[dyemanov]

I agree triggers management code is a horrible mess. I already tried to improve something (reference counting and resource management) once but succeeded only partially, regressions required to restore some logic back. This beast deserves a complete rewrite, IMHO, but we cannot risk with any big changes before the v5 release.

This patch was born just to avoid repeated (and obviously redundant) re-compilation of triggers (I noticed that during my other work). I'm not going to insist if it causes any doubts, provided that we'll schedule the triggers code rewrite for the next major release. I'm wondering whether Alex already changed something during his shared metadata cache work.

As for memory leaks, I was tracking mon$memory_usage for the current attachment and my tests show memory usage is still growing after DDLs, although less than before this patch. But these DDL also load new metadata objects into the metadata cache, so I was not convinced it was really a leak. Didn't look deeper though.

[AlexPeshkoff]

On 12/17/22 11:11, Dmitry Yemanov wrote: I'm wondering whether Alex already changed something during his shared metadata cache work.

Not much - I could not understand the logic of it's operaton in some places. Now I see that it's just buggy sometimes :(