Re: Table refer leak in logical replication

On Wed, Apr 21, 2021 at 7:38 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Apr 21, 2021 at 04:21:52PM +0900, Amit Langote wrote:
> > So I had started last night by adding some tests for this in
> > 003_constraints.pl because there are already some replica BR trigger
> > tests there.  I like your suggestion to have some tests around
> > partitions, so added some in 013_partition.pl too.  Let me know what
> > you think.
>
> Thanks, cool!

Thanks for looking.

> +        IF (NEW.bid = 4 AND NEW.id = OLD.id) THEN
> +           RETURN NEW;
> +        ELSE
> +            RETURN NULL;
> +        END IF;
> Nit: the indentation is a bit off here.

Hmm, I checked that I used 4 spaces for indenting, but maybe you're
concerned that the whole thing is indented unnecessarily relative to
the parent ELSIF block?

> +CREATE FUNCTION log_tab_fk_ref_upd() RETURNS TRIGGER AS \$\$
> +BEGIN
> +    CREATE TABLE IF NOT EXISTS public.tab_fk_ref_op_log (tgtab text,
> tgop text, tgwhen text, tglevel text, oldbid int, newbid int);
> +    INSERT INTO public.tab_fk_ref_op_log SELECT TG_RELNAME, TG_OP,
> TG_WHEN, TG_LEVEL, OLD.bid, NEW.bid;
> +    RETURN NULL;
> +END;
> Let's use only one function here, then you can just do something like
> that and use NEW and OLD as you wish conditionally:
> IF (TG_OP = 'INSERT') THEN
>   INSERT INTO tab_fk_ref_op_log blah;
> ELSIF (TG_OP = 'UPDATE') THEN
>   INSERT INTO tab_fk_ref_op_log blah_;
> END IF;
>
> The same remark applies to the two files where the tests have been
> introduced.

That's certainly better with fewer lines.

> Why don't you create the table beforehand rather than making it part
> of the trigger function?

Makes sense too.

> +CREATE TRIGGER tab_fk_ref_log_ins_after_trg
> [...]
> +CREATE TRIGGER tab_fk_ref_log_upd_after_trg
> No need for two triggers either once there is only one function.

Right.

> +   "SELECT * FROM tab_fk_ref_op_log ORDER BY tgop, newbid;");
> I would add tgtab and tgwhen to the ORDER BY here, just to be on the
> safe side, and apply the same rule everywhere.  Your patch is already
> consistent regarding that and help future debugging, that's good.

Okay, done.

--
Amit Langote
EDB: http://www.enterprisedb.com