Re: Table refer leak in logical replication

From: Amit Langote <amitlangote09(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Subject: Re: Table refer leak in logical replication
Date: 2021-04-21 12:58:10
Message-ID: CA+HiwqEBGa-rmyxGs2tAUo7GPGEioCexpOzRRZB9JmyigRSjbA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Apr 21, 2021 at 7:38 PM Michael Paquier <michael(at)paquier(dot)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

Attachment Content-Type Size
fix_relcache_leak_in_lrworker_v8.patch application/octet-stream 13.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-04-21 13:34:46 Re: [bug?] Missed parallel safety checks, and wrong parallel safety
Previous Message Magnus Hagander 2021-04-21 12:38:44 Re: RFE: Make statistics robust for unplanned events