Re: Table refer leak in logical replication

2021-04-26 Thread Amit Langote
On Mon, Apr 26, 2021 at 3:27 PM Michael Paquier wrote: > On Fri, Apr 23, 2021 at 09:38:01PM +0900, Amit Langote wrote: > > On Thu, Apr 22, 2021 at 1:45 PM Michael Paquier wrote: > >> On the other hand, the tests for partitions have much more value IMO, > >> but looking closely I think that we

Re: Table refer leak in logical replication

2021-04-26 Thread Michael Paquier
On Fri, Apr 23, 2021 at 09:38:01PM +0900, Amit Langote wrote: > On Thu, Apr 22, 2021 at 1:45 PM Michael Paquier wrote: >> On the other hand, the tests for partitions have much more value IMO, >> but looking closely I think that we can do better: >> - Create triggers on more relations of the

Re: Table refer leak in logical replication

2021-04-23 Thread Amit Langote
On Thu, Apr 22, 2021 at 1:45 PM Michael Paquier wrote: > On Wed, Apr 21, 2021 at 09:58:10PM +0900, Amit Langote wrote: > > Okay, done. > > So, I have been working on that today, and tried to apply the full set > before realizing when writing the commit message that this was a set > of bullet

Re: Table refer leak in logical replication

2021-04-21 Thread Michael Paquier
On Wed, Apr 21, 2021 at 09:58:10PM +0900, Amit Langote wrote: > Okay, done. So, I have been working on that today, and tried to apply the full set before realizing when writing the commit message that this was a set of bullet points, and that this was too much for a single commit. The tests are

Re: Table refer leak in logical replication

2021-04-21 Thread Amit Langote
On Wed, Apr 21, 2021 at 7:38 PM Michael Paquier 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

Re: Table refer leak in logical replication

2021-04-21 Thread Michael Paquier
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

Re: Table refer leak in logical replication

2021-04-21 Thread Amit Langote
On Wed, Apr 21, 2021 at 11:13 AM Amit Langote wrote: > On Wed, Apr 21, 2021 at 9:31 AM Michael Paquier wrote: > > On Tue, Apr 20, 2021 at 06:20:03PM +0530, Amit Kapila wrote: > > > +1. I think it makes sense to add a test case especially because we > > > don't have any existing test in this

Re: Table refer leak in logical replication

2021-04-20 Thread Michael Paquier
On Wed, Apr 21, 2021 at 11:13:06AM +0900, Amit Langote wrote: > Agree about adding tests along these lines. Will post in a bit. Thanks! -- Michael signature.asc Description: PGP signature

Re: Table refer leak in logical replication

2021-04-20 Thread Amit Langote
On Wed, Apr 21, 2021 at 9:31 AM Michael Paquier wrote: > On Tue, Apr 20, 2021 at 06:20:03PM +0530, Amit Kapila wrote: > > +1. I think it makes sense to add a test case especially because we > > don't have any existing test in this area. > > Yes, let's add add something into 013_partition.pl

Re: Table refer leak in logical replication

2021-04-20 Thread Michael Paquier
On Tue, Apr 20, 2021 at 06:20:03PM +0530, Amit Kapila wrote: > +1. I think it makes sense to add a test case especially because we > don't have any existing test in this area. Yes, let's add add something into 013_partition.pl within both subscriber1 and subscriber2. This will not catch up the

Re: Table refer leak in logical replication

2021-04-20 Thread Amit Kapila
On Tue, Apr 20, 2021 at 4:59 PM houzj.f...@fujitsu.com wrote: > > > > ... FWIW, I'd rather > > > agree to use what has been proposed with es_opened_result_relations > > > like TRUNCATE does rather than attempt to use ExecInitResultRelation() > > > combined with potentially asymmetric calls to > >

RE: Table refer leak in logical replication

2021-04-20 Thread houzj.f...@fujitsu.com
> > ... FWIW, I'd rather > > agree to use what has been proposed with es_opened_result_relations > > like TRUNCATE does rather than attempt to use ExecInitResultRelation() > > combined with potentially asymmetric calls to > > ExecCloseResultRelations(). > > Okay, how about the attached then? I

Re: Table refer leak in logical replication

2021-04-20 Thread Amit Langote
On Tue, Apr 20, 2021 at 4:22 PM Michael Paquier wrote: > On Tue, Apr 20, 2021 at 02:48:35PM +0900, Amit Langote wrote: > > Manipulating the contents of es_opened_result_relations directly in > > worker.c is admittedly a "hack", which I am reluctant to have other > > places participating in. As

Re: Table refer leak in logical replication

2021-04-20 Thread Michael Paquier
On Tue, Apr 20, 2021 at 02:48:35PM +0900, Amit Langote wrote: > Manipulating the contents of es_opened_result_relations directly in > worker.c is admittedly a "hack", which I am reluctant to have other > places participating in. As originally designed, that list is to > speed up

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
Thanks for taking a look. On Tue, Apr 20, 2021 at 2:09 PM Michael Paquier wrote: > On Mon, Apr 19, 2021 at 09:44:05PM +0900, Amit Langote wrote: > > Okay, how about the attached then? > > create_estate_for_relation() returns an extra resultRelInfo that's > also saved within

Re: Table refer leak in logical replication

2021-04-19 Thread Michael Paquier
On Mon, Apr 19, 2021 at 09:44:05PM +0900, Amit Langote wrote: > Okay, how about the attached then? create_estate_for_relation() returns an extra resultRelInfo that's also saved within es_opened_result_relations. Wouldn't is be simpler to take the first element from es_opened_result_relations

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
On Mon, Apr 19, 2021 at 6:32 PM Michael Paquier wrote: > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote: > > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote > > wrote: > >> FWIW, I agree with fixing this bug of 1375422c in as least scary > >> manner as possible. Hou-san proposed

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 5:20 PM Michael Paquier wrote: > > On Mon, Apr 19, 2021 at 08:09:05PM +0900, Amit Langote wrote: > > On Mon, Apr 19, 2021 at 7:00 PM Amit Kapila wrote: > >> It seems like the memory will be freed after we apply the truncate > >> because we reset the ApplyMessageContext

Re: Table refer leak in logical replication

2021-04-19 Thread Michael Paquier
On Mon, Apr 19, 2021 at 08:09:05PM +0900, Amit Langote wrote: > On Mon, Apr 19, 2021 at 7:00 PM Amit Kapila wrote: >> It seems like the memory will be freed after we apply the truncate >> because we reset the ApplyMessageContext after applying each message, >> so maybe we don't need to bother

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
On Mon, Apr 19, 2021 at 7:00 PM Amit Kapila wrote: > On Mon, Apr 19, 2021 at 3:25 PM Amit Kapila wrote: > > On Mon, Apr 19, 2021 at 3:12 PM Amit Kapila wrote: > > > On Mon, Apr 19, 2021 at 3:02 PM Michael Paquier > > > wrote: > > > > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 3:25 PM Amit Kapila wrote: > > On Mon, Apr 19, 2021 at 3:12 PM Amit Kapila wrote: > > > > On Mon, Apr 19, 2021 at 3:02 PM Michael Paquier wrote: > > > > > > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote: > > > > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 3:12 PM Amit Kapila wrote: > > On Mon, Apr 19, 2021 at 3:02 PM Michael Paquier wrote: > > > > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote: > > > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote > > > wrote: > > >> FWIW, I agree with fixing this bug of

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 3:02 PM Michael Paquier wrote: > > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote: > > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote > > wrote: > >> FWIW, I agree with fixing this bug of 1375422c in as least scary > >> manner as possible. Hou-san proposed

Re: Table refer leak in logical replication

2021-04-19 Thread Michael Paquier
On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote: > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote wrote: >> FWIW, I agree with fixing this bug of 1375422c in as least scary >> manner as possible. Hou-san proposed that we add the ResultRelInfo >> that

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
On Mon, Apr 19, 2021 at 6:03 PM Amit Kapila wrote: > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote wrote: > > On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila > > wrote: > > > On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier > > > wrote: > > > > Attached is v5 that I am finishing with. Much

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 12:32 PM Amit Langote wrote: > > On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila wrote: > > On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier > > wrote: > > > On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > > > > > > Attached is v5 that I am finishing with.

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Kapila
On Mon, Apr 19, 2021 at 1:51 PM Amit Langote wrote: > > On Fri, Apr 16, 2021 at 3:24 PM Michael Paquier wrote: > > On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > > > While updating the patch to do so, it occurred to me that maybe we > > > could move the ExecInitResultRelation()

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
On Fri, Apr 16, 2021 at 3:24 PM Michael Paquier wrote: > On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > > While updating the patch to do so, it occurred to me that maybe we > > could move the ExecInitResultRelation() call into > > create_estate_for_relation() too, in the spirit

Re: Table refer leak in logical replication

2021-04-19 Thread Amit Langote
On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila wrote: > On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier wrote: > > On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > > > > Attached is v5 that I am finishing with. Much more could be done but > > I don't want to do something too

Re: Table refer leak in logical replication

2021-04-19 Thread Michael Paquier
On Mon, Apr 19, 2021 at 03:08:41PM +0900, Michael Paquier wrote: > FWIW, I > would be tempted to send back f1ac27b to the blackboard, then refactor > the code of the apply worker to use ExecInitResultRelation() so as we > get more consistency with resource releases, simplifying the business > with

Re: Table refer leak in logical replication

2021-04-19 Thread Michael Paquier
On Sat, Apr 17, 2021 at 07:02:00PM +0530, Amit Kapila wrote: > Hmm, I am not sure if it is a good idea to open indexes needlessly > especially when it is not done in the previous code. Studying the history of this code, I think that f1ac27b is to blame here for making the code of the apply worker

Re: Table refer leak in logical replication

2021-04-17 Thread Amit Kapila
On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier wrote: > > On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > > Attached is v5 that I am finishing with. Much more could be done but > I don't want to do something too invasive at this stage of the game. > There are a couple of extra

Re: Table refer leak in logical replication

2021-04-16 Thread Michael Paquier
On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote: > While updating the patch to do so, it occurred to me that maybe we > could move the ExecInitResultRelation() call into > create_estate_for_relation() too, in the spirit of removing > duplicative code. See attached updated patch.

Re: Table refer leak in logical replication

2021-04-11 Thread Amit Langote
On Sat, Apr 10, 2021 at 10:39 AM Justin Pryzby wrote: > I added this as an Open Item. Thanks, Justin. -- Amit Langote EDB: http://www.enterprisedb.com

Re: Table refer leak in logical replication

2021-04-09 Thread Justin Pryzby
I added this as an Open Item. https://wiki.postgresql.org/index.php?title=PostgreSQL_14_Open_Items=revision=35895=35890 https://www.postgresql.org/message-id/flat/OS0PR01MB6113BA0A760C9964A4A0C5C2FB769%40OS0PR01MB6113.jpnprd01.prod.outlook.com#2fc410dff5cd27eea357ffc17fc174f2 On Tue, Apr 06, 2021

RE: Table refer leak in logical replication

2021-04-06 Thread tanghy.f...@fujitsu.com
On Tuesday, April 6, 2021 2:25 PM Amit Langote wrote: >While updating the patch to do so, it occurred to me that maybe we >could move the ExecInitResultRelation() call into >create_estate_for_relation() too, in the spirit of removing >duplicative code. See attached updated patch. Thanks for

Re: Table refer leak in logical replication

2021-04-05 Thread Amit Langote
On Tue, Apr 6, 2021 at 1:57 PM Masahiko Sawada wrote: > On Tue, Apr 6, 2021 at 1:15 PM Amit Langote wrote: > > On Tue, Apr 6, 2021 at 1:01 PM houzj.f...@fujitsu.com > > > The commit introduced a > > > > new function ExecInitResultRelation() that sets both > > > > estate->es_result_relations and

Re: Table refer leak in logical replication

2021-04-05 Thread Masahiko Sawada
On Tue, Apr 6, 2021 at 1:15 PM Amit Langote wrote: > > On Tue, Apr 6, 2021 at 1:01 PM houzj.f...@fujitsu.com > wrote: > > > > > insert into test values (6); > > > > > > > > > > It seems an issue about reference leak. Anyone can fix this? > > > > > > > > It seems ExecGetTriggerResultRel will

Re: Table refer leak in logical replication

2021-04-05 Thread Amit Langote
On Tue, Apr 6, 2021 at 1:01 PM houzj.f...@fujitsu.com wrote: > > > > insert into test values (6); > > > > > > > > It seems an issue about reference leak. Anyone can fix this? > > > > > > It seems ExecGetTriggerResultRel will reopen the target table because it > > cannot find an existing one. > >

RE: Table refer leak in logical replication

2021-04-05 Thread houzj.f...@fujitsu.com
> > > insert into test values (6); > > > > > > It seems an issue about reference leak. Anyone can fix this? > > > > It seems ExecGetTriggerResultRel will reopen the target table because it > cannot find an existing one. > > Storing the opened table in estate->es_opened_result_relations seems >

Re: Table refer leak in logical replication

2021-04-05 Thread Masahiko Sawada
On Tue, Apr 6, 2021 at 10:15 AM houzj.f...@fujitsu.com wrote: > > > WARNING: relcache reference leak: relation "xxx" not closed. > > > > Example of the procedure: > > --publisher-- > > create table test (a int primary key); > > create publication pub for table test; > > > >

RE: Table refer leak in logical replication

2021-04-05 Thread shiy.f...@fujitsu.com
> BTW, it seems better to add a testcase for this ? I think the test for it can be added in src/test/subscription/t/003_constraints.pl, which is like what in my patch. Regards, Shi yu tests_for_table_refer_leak.diff Description: tests_for_table_refer_leak.diff

RE: Table refer leak in logical replication

2021-04-05 Thread houzj.f...@fujitsu.com
> WARNING: relcache reference leak: relation "xxx" not closed. > > Example of the procedure: > --publisher-- > create table test (a int primary key); > create publication pub for table test; > > --subscriber-- > create table test (a int primary key); > create subscription sub

Table refer leak in logical replication

2021-04-05 Thread tanghy.f...@fujitsu.com
Hi I met a problem about trigger in logical replication. I created a trigger after inserting data at subscriber, but there is a warning in the log of subscriber when the trigger fired: WARNING: relcache reference leak: relation "xxx" not closed. Example of the procedure: --publisher--