Table refer leak in logical replication

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 connection 'dbname=postgres' publication pub;
create function funcA() returns trigger as $$ begin return null; end; $$ language
plpgsql; create trigger my_trig after insert or update or delete on test for each
row execute procedure funcA(); alter table test enable replica trigger my_trig;

------publisher------
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 solves the problem.

Attaching a patch that fix this.
BTW, it seems better to add a testcase for this ?

Best regards,
houzj

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

On Tue, Apr 6, 2021 at 10:15 AM houzj.fnst@fujitsu.com
<houzj.fnst@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;

------subscriber------
create table test (a int primary key);
create subscription sub connection 'dbname=postgres' publication pub;
create function funcA() returns trigger as $$ begin return null; end; $$ language
plpgsql; create trigger my_trig after insert or update or delete on test for each
row execute procedure funcA(); alter table test enable replica trigger my_trig;

------publisher------
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 solves the problem.

It seems like commit 1375422c is related to this bug. The commit
introduced a new function ExecInitResultRelation() that sets both
estate->es_result_relations and estate->es_opened_result_relations. I
think it's better to use ExecInitResultRelation() rather than directly
setting estate->es_opened_result_relations. It might be better to do
that in create_estate_for_relation() though. Please find an attached
patch.

Since this issue happens on only HEAD and it seems an oversight of
commit 1375422c, I don't think regression tests for this are
essential.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.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

solves the problem.

It seems like commit 1375422c is related to this bug. The commit introduced a
new function ExecInitResultRelation() that sets both
estate->es_result_relations and estate->es_opened_result_relations. I
think it's better to use ExecInitResultRelation() rather than directly setting
estate->es_opened_result_relations. It might be better to do that in
create_estate_for_relation() though. Please find an attached patch.

Since this issue happens on only HEAD and it seems an oversight of commit
1375422c, I don't think regression tests for this are essential.

It seems we can not only use ExecInitResultRelation.
In function ExecInitResultRelation, it will use ExecGetRangeTableRelation which
will also open the target table and store the rel in "Estate->es_relations".
We should call ExecCloseRangeTableRelations at the end of apply_handle_xxx to
close the rel in "Estate->es_relations".

Attaching the patch with this change.

Best regards,
houzj

On Tue, Apr 6, 2021 at 1:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@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.

Storing the opened table in estate->es_opened_result_relations seems

solves the problem.

It seems like commit 1375422c is related to this bug.

Right, thanks for pointing this out.

The commit introduced a

new function ExecInitResultRelation() that sets both
estate->es_result_relations and estate->es_opened_result_relations. I
think it's better to use ExecInitResultRelation() rather than directly setting
estate->es_opened_result_relations. It might be better to do that in
create_estate_for_relation() though. Please find an attached patch.

Agree that ExecInitResultRelations() would be better.

Since this issue happens on only HEAD and it seems an oversight of commit
1375422c, I don't think regression tests for this are essential.

It seems we can not only use ExecInitResultRelation.
In function ExecInitResultRelation, it will use ExecGetRangeTableRelation which
will also open the target table and store the rel in "Estate->es_relations".
We should call ExecCloseRangeTableRelations at the end of apply_handle_xxx to
close the rel in "Estate->es_relations".

Right, ExecCloseRangeTableRelations() was missing.

I think it may be better to create a sibling function to
create_estate_for_relation(), say, close_estate(EState *), that
performs the cleanup actions, including the firing of any AFTER
triggers. See attached updated patch to see what I mean.

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

On Tue, Apr 6, 2021 at 1:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Apr 6, 2021 at 1:01 PM houzj.fnst@fujitsu.com
<houzj.fnst@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.

Storing the opened table in estate->es_opened_result_relations seems

solves the problem.

It seems like commit 1375422c is related to this bug.

Right, thanks for pointing this out.

The commit introduced a

new function ExecInitResultRelation() that sets both
estate->es_result_relations and estate->es_opened_result_relations. I
think it's better to use ExecInitResultRelation() rather than directly setting
estate->es_opened_result_relations. It might be better to do that in
create_estate_for_relation() though. Please find an attached patch.

Agree that ExecInitResultRelations() would be better.

Since this issue happens on only HEAD and it seems an oversight of commit
1375422c, I don't think regression tests for this are essential.

It seems we can not only use ExecInitResultRelation.
In function ExecInitResultRelation, it will use ExecGetRangeTableRelation which
will also open the target table and store the rel in "Estate->es_relations".
We should call ExecCloseRangeTableRelations at the end of apply_handle_xxx to
close the rel in "Estate->es_relations".

Right, ExecCloseRangeTableRelations() was missing.

Yeah, I had missed it. Thank you for pointing out it.

I think it may be better to create a sibling function to
create_estate_for_relation(), say, close_estate(EState *), that
performs the cleanup actions, including the firing of any AFTER
triggers. See attached updated patch to see what I mean.

Looks good to me.

BTW I found the following comments in create_estate_for_relation():

/*
* Executor state preparation for evaluation of constraint expressions,
* indexes and triggers.
*
* This is based on similar code in copy.c
*/
static EState *
create_estate_for_relation(LogicalRepRelMapEntry *rel)

It seems like the comments meant the code around CopyFrom() and
DoCopy() but it would no longer be true since copy.c has been split
into some files and I don't find similar code in copy.c. I think it’s
better to remove the sentence rather than update the file name as this
comment doesn’t really informative and hard to track the updates. What
do you think?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Apr 6, 2021 at 1:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Apr 6, 2021 at 1:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Apr 6, 2021 at 1:01 PM houzj.fnst@fujitsu.com

The commit introduced a

new function ExecInitResultRelation() that sets both
estate->es_result_relations and estate->es_opened_result_relations. I
think it's better to use ExecInitResultRelation() rather than directly setting
estate->es_opened_result_relations. It might be better to do that in
create_estate_for_relation() though. Please find an attached patch.

Agree that ExecInitResultRelations() would be better.

Since this issue happens on only HEAD and it seems an oversight of commit
1375422c, I don't think regression tests for this are essential.

It seems we can not only use ExecInitResultRelation.
In function ExecInitResultRelation, it will use ExecGetRangeTableRelation which
will also open the target table and store the rel in "Estate->es_relations".
We should call ExecCloseRangeTableRelations at the end of apply_handle_xxx to
close the rel in "Estate->es_relations".

Right, ExecCloseRangeTableRelations() was missing.

Yeah, I had missed it. Thank you for pointing out it.

I think it may be better to create a sibling function to
create_estate_for_relation(), say, close_estate(EState *), that
performs the cleanup actions, including the firing of any AFTER
triggers. See attached updated patch to see what I mean.

Looks good to me.

BTW I found the following comments in create_estate_for_relation():

/*
* Executor state preparation for evaluation of constraint expressions,
* indexes and triggers.
*
* This is based on similar code in copy.c
*/
static EState *
create_estate_for_relation(LogicalRepRelMapEntry *rel)

It seems like the comments meant the code around CopyFrom() and
DoCopy() but it would no longer be true since copy.c has been split
into some files and I don't find similar code in copy.c. I think it’s
better to remove the sentence rather than update the file name as this
comment doesn’t really informative and hard to track the updates. What
do you think?

Yeah, agree with simply removing that comment.

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. Actually I remember
proposing that as part of the commit you shared in your earlier email,
but for some reason it didn't end up in the commit. I now think maybe
we should do that after all.

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

On Tuesday, April 6, 2021 2:25 PM Amit Langote <amitlangote09@gmail.com> 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 your fixing. The code LGTM.
Made a confirmation right now, the problem has been solved after applying your patch.

Regards,
Tang

I added this as an Open Item.
https://wiki.postgresql.org/index.php?title=PostgreSQL_14_Open_Items&amp;type=revision&amp;diff=35895&amp;oldid=35890
/messages/by-id/OS0PR01MB6113BA0A760C9964A4A0C5C2FB769@OS0PR01MB6113.jpnprd01.prod.outlook.com

On Sat, Apr 10, 2021 at 10:39 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

I added this as an Open Item.

Thanks, Justin.

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

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. Actually I remember
proposing that as part of the commit you shared in your earlier email,
but for some reason it didn't end up in the commit. I now think maybe
we should do that after all.

So, I have been studying 1375422c and this thread. Using
ExecCloseRangeTableRelations() when cleaning up the executor state is
reasonable as of the equivalent call to ExecInitRangeTable(). Now,
there is something that itched me quite a lot while reviewing the
proposed patch: ExecCloseResultRelations() is missing, but other
code paths make an effort to mirror any calls of ExecInitRangeTable()
with it. Looking closer, I think that the worker code is actually
confused with the handling of the opening and closing of the indexes
needed by a result relation once you use that, because some code paths
related to tuple routing for partitions may, or may not, need to do
that. However, once the code integrates with ExecInitRangeTable() and
ExecCloseResultRelations(), the index handlings could get much simpler
to understand as the internal apply routines for INSERT/UPDATE/DELETE
have no need to think about the opening or closing them. Well,
mostly, to be honest.

There are two exceptions when it comes the tuple routing for
partitioned tables, one for INSERT/DELETE as the result relation found
at the top of apply_handle_tuple_routing() can be used, and a second
for the UPDATE case as it is necessary to re-route the tuple to the
new partition, as it becomes necessary to open and close the indexes
of the new partition relation where a tuple is sent to. I think that
there is a lot of room for a much better integration in terms of
estate handling for this stuff with the executor, but that would be
too invasive for v14 post feature freeze, and I am not sure what a
good design would be.

Related to that, I found confusing that the patch was passing down a
result relation from create_estate_for_relation() for something that's
just stored in the executor state. Having a "close" routine that maps
to the "create" routine gives a good vibe, though "close" is usually
used in parallel of "open" in the PG code, and instead of "free" I
have found "finish" a better term.

Another thing, and I think that it is a good change, is that it is
necessary to push a snapshot in the worker process before creating the
executor state as any index predicates of the result relation are
going to need that when opened. My impression of the code of worker.c
is that the amount of code duplication is quite high between the three
DML code paths, with the update tuple routing logic being a space of
improvements on its own, and that it could gain in clarity with more
refactoring work around the executor states, but I am fine to leave
that as future work. That's too late for v14.

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 relations in terms of relations opened for
a partitioned table within create_estate_for_relation() when
redirecting to the tuple routing, but my guess is that this would be
better in the long-term. We could bypass doing that when working on a
partitioned table, but I really don't think that this code should be
made more confusing and that we had better apply
ExecCloseResultRelations() for all the relations faced. That's
simpler to reason about IMO.
--
Michael

On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier <michael@paquier.xyz> 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 relations in terms of relations opened for
a partitioned table within create_estate_for_relation() when
redirecting to the tuple routing, but my guess is that this would be
better in the long-term.

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.

@@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
  slot_getallattrs(remoteslot);
  }
  MemoryContextSwitchTo(oldctx);
+
+ ExecOpenIndices(partrelinfo_new, false);
  apply_handle_insert_internal(partrelinfo_new, estate,
  remoteslot_part);
+ ExecCloseIndices(partrelinfo_new);
  }

It seems you forgot to call open indexes before apply_handle_delete_internal.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1]/messages/by-id/OS0PR01MB571686F75FBDC219FF3DFF0D94769@OS0PR01MB5716.jpnprd01.prod.outlook.com because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open. OTOH, using
ExecInitResultRelation might encapsulate the assignment we are doing
outside. In general, it seems doing bigger refactoring or changes
might lead to some bugs or unintended consequences, so if possible, we
can try such refactoring as a separate patch. One argument against the
proposed refactoring could be that with the previous code, we were
trying to open the indexes just before it is required but with the new
patch in some cases, they will be opened during the initial phase and
for other cases, they are opened just before they are required. It
might not necessarily be a bad idea to rearrange code like that but
maybe there is a better way to do that.

[1]: /messages/by-id/OS0PR01MB571686F75FBDC219FF3DFF0D94769@OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

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 much messier than it was
before. Before that, we were at a point where we had to rely on one
single ResultRelInfo with all its indexes opened and closed before
doing the DML. After f1ac27b, the code becomes shaped so as the
original ResultRelInfo may or may not be used depending on if this
code is working on a partitioned table or not. With an UPDATE, not
one but *two* ResultRelInfo may be used if a tuple is moved to a
different partition. I think that in the long term, and if we want to
make use of ExecInitResultRelation() in this area, we are going to
need to split the apply code in two parts, roughly (easier to say in
words than actually doing it, still):
- Find out first which relations it is necessary to work on, and
create a set of ResultRelInfo assigned to an executor state by
ExecInitResultRelation(), doing all the relation openings that are
necessary. The gets funky when attempting to do an update across
partitions.
- Do the actual DML, with all the relations already opened and ready
for use.

On top of that, I am really getting scared by the following, done in
not one but now two places:
/*
* The tuple to be updated could not be found.
*
* TODO what to do here, change the log level to LOG perhaps?
*/
elog(DEBUG1,
"logical replication did not find row for update "
"in replication target relation \"%s\"",
RelationGetRelationName(localrel));
This already existed in once place before f1ac27b, but this got
duplicated in a second area when applying the first update to a
partition table.

The refactoring change done in 1375422c in worker.c without the tuple
routing pieces would be a piece of cake in terms of relations that
require to be opened and closed, including the timings of each call
because they could be unified in single code paths, and I also guess
that we would avoid leak issues really easily. If the tuple routing
code actually does not consider the case of moving a tuple across
partitions, the level of difficulty to do an integration with
ExecInitResultRelation() is much more reduced, though it makes the
feature much less appealing as it becomes much more difficult to do
some data redistribution across a different set of partitions with
logical changes.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1] because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open. OTOH, using
ExecInitResultRelation might encapsulate the assignment we are doing
outside.

Yeah, that would be nice to just rely on that. copyfrom.c does
basically what I guess we should try to copy a maximum here. With a
proper cleanup of the executor state using ExecCloseResultRelations()
once we are done with the tuple apply.

In general, it seems doing bigger refactoring or changes
might lead to some bugs or unintended consequences, so if possible, we
can try such refactoring as a separate patch. One argument against the
proposed refactoring could be that with the previous code, we were
trying to open the indexes just before it is required but with the new
patch in some cases, they will be opened during the initial phase and
for other cases, they are opened just before they are required. It
might not necessarily be a bad idea to rearrange code like that but
maybe there is a better way to do that.

[1] - /messages/by-id/OS0PR01MB571686F75FBDC219FF3DFF0D94769@OS0PR01MB5716.jpnprd01.prod.outlook.com

This feels like piling one extra hack on top of what looks like an
abuse of the executor calls to me, and the apply code is already full
of it. True that we do that in ExecuteTruncateGuts() for allow
triggers to be fired, but I think that it would be better to avoid
spread that to consolidate the trigger and execution code. 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 indexes. Once the code is in a cleaner state, we could come back
into making an integration with partitioned tables into this code.
--
Michael

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 indexes. Once the code is in a cleaner state, we could come back
into making an integration with partitioned tables into this code.

But you cannot do that either as f1ac27bf got into 13..
--
Michael

On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier <michael@paquier.xyz> 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 relations in terms of relations opened for
a partitioned table within create_estate_for_relation() when
redirecting to the tuple routing, but my guess is that this would be
better in the long-term.

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.

@@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
slot_getallattrs(remoteslot);
}
MemoryContextSwitchTo(oldctx);
+
+ ExecOpenIndices(partrelinfo_new, false);
apply_handle_insert_internal(partrelinfo_new, estate,
remoteslot_part);
+ ExecCloseIndices(partrelinfo_new);
}

It seems you forgot to call open indexes before apply_handle_delete_internal.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1] because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open.

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 apply_handle_{insert|update|delete} initialize themselves to
es_opened_result_relations. I would prefer that only
ExecInitResultRelation() add anything to es_opened_result_relations()
to avoid future maintenance problems. Instead, a fix as simple as the
Hou-san's proposed fix would be to add a ExecCloseResultRelations()
call at the end of each of apply_handle_{insert|update|delete}. That
would fix the originally reported leak, because
ExecCloseResultRelations() has this:

/* Close any relations that have been opened by
ExecGetTriggerResultRel(). */
foreach(l, estate->es_trig_target_relations)
{

We do end up with the reality though that trigger.c now opens the
replication target relation on its own (adding it to
es_trig_target_relations) to fire its triggers.

I am also not opposed to reviewing the changes of 1375422c in light of
these findings while we still have time. For example, it might
perhaps be nice for ExecInitResultRelation to accept a Relation
pointer that the callers from copyfrom.c, worker.c can use to pass
their locally opened Relation. In that case, ExecInitResultRelation()
would perform InitResultRelInfo() with that Relation pointer, instead
of getting one using ExecGetRangeTableRelation() which is what causes
the Relation to be opened again.

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

On Fri, Apr 16, 2021 at 3:24 PM Michael Paquier <michael@paquier.xyz> 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 of removing
duplicative code. See attached updated patch. Actually I remember
proposing that as part of the commit you shared in your earlier email,
but for some reason it didn't end up in the commit. I now think maybe
we should do that after all.

So, I have been studying 1375422c and this thread. Using
ExecCloseRangeTableRelations() when cleaning up the executor state is
reasonable as of the equivalent call to ExecInitRangeTable(). Now,
there is something that itched me quite a lot while reviewing the
proposed patch: ExecCloseResultRelations() is missing, but other
code paths make an effort to mirror any calls of ExecInitRangeTable()
with it. Looking closer, I think that the worker code is actually
confused with the handling of the opening and closing of the indexes
needed by a result relation once you use that, because some code paths
related to tuple routing for partitions may, or may not, need to do
that. However, once the code integrates with ExecInitRangeTable() and
ExecCloseResultRelations(), the index handlings could get much simpler
to understand as the internal apply routines for INSERT/UPDATE/DELETE
have no need to think about the opening or closing them. Well,
mostly, to be honest.

To bring up another point, if we were to follow the spirit of the
recent c5b7ba4e67a, whereby we moved ExecOpenIndices() from
ExecInitModifyTable() into ExecInsert() and ExecUpdate(), that is,
from during the initialization phase of an INSERT/UPDATE to its actual
execution, we could even consider performing Exec{Open|Close}Indices()
for a replication target relation in
ExecSimpleRelation{Insert|Update}. The ExecOpenIndices() performed in
worker.c is pointless in some cases, for example, when
ExecSimpleRelation{Insert|Update} end up skipping the insert/update of
a tuple due to BR triggers.

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

On Mon, Apr 19, 2021 at 1:51 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Apr 16, 2021 at 3:24 PM Michael Paquier <michael@paquier.xyz> 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 of removing
duplicative code. See attached updated patch. Actually I remember
proposing that as part of the commit you shared in your earlier email,
but for some reason it didn't end up in the commit. I now think maybe
we should do that after all.

So, I have been studying 1375422c and this thread. Using
ExecCloseRangeTableRelations() when cleaning up the executor state is
reasonable as of the equivalent call to ExecInitRangeTable(). Now,
there is something that itched me quite a lot while reviewing the
proposed patch: ExecCloseResultRelations() is missing, but other
code paths make an effort to mirror any calls of ExecInitRangeTable()
with it. Looking closer, I think that the worker code is actually
confused with the handling of the opening and closing of the indexes
needed by a result relation once you use that, because some code paths
related to tuple routing for partitions may, or may not, need to do
that. However, once the code integrates with ExecInitRangeTable() and
ExecCloseResultRelations(), the index handlings could get much simpler
to understand as the internal apply routines for INSERT/UPDATE/DELETE
have no need to think about the opening or closing them. Well,
mostly, to be honest.

To bring up another point, if we were to follow the spirit of the
recent c5b7ba4e67a, whereby we moved ExecOpenIndices() from
ExecInitModifyTable() into ExecInsert() and ExecUpdate(), that is,
from during the initialization phase of an INSERT/UPDATE to its actual
execution, we could even consider performing Exec{Open|Close}Indices()
for a replication target relation in
ExecSimpleRelation{Insert|Update}. The ExecOpenIndices() performed in
worker.c is pointless in some cases, for example, when
ExecSimpleRelation{Insert|Update} end up skipping the insert/update of
a tuple due to BR triggers.

Yeah, that is also worth considering and sounds like a good idea. But,
as I mentioned before it might be better to consider this separately.

--
With Regards,
Amit Kapila.

On Mon, Apr 19, 2021 at 12:32 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier <michael@paquier.xyz> 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 relations in terms of relations opened for
a partitioned table within create_estate_for_relation() when
redirecting to the tuple routing, but my guess is that this would be
better in the long-term.

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.

@@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
slot_getallattrs(remoteslot);
}
MemoryContextSwitchTo(oldctx);
+
+ ExecOpenIndices(partrelinfo_new, false);
apply_handle_insert_internal(partrelinfo_new, estate,
remoteslot_part);
+ ExecCloseIndices(partrelinfo_new);
}

It seems you forgot to call open indexes before apply_handle_delete_internal.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1] because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open.

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 apply_handle_{insert|update|delete} initialize themselves to
es_opened_result_relations. I would prefer that only
ExecInitResultRelation() add anything to es_opened_result_relations()
to avoid future maintenance problems. Instead, a fix as simple as the
Hou-san's proposed fix would be to add a ExecCloseResultRelations()
call at the end of each of apply_handle_{insert|update|delete}.

Yeah, that will work too but might look a bit strange. BTW, how that
is taken care of for ExecuteTruncateGuts? I mean we do add rels there
like Hou-San's patch without calling ExecCloseResultRelations, the
rels are probably closed when we close the relation in worker.c but
what about memory for the list?

--
With Regards,
Amit Kapila.

On Mon, Apr 19, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Apr 19, 2021 at 12:32 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier <michael@paquier.xyz> 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 relations in terms of relations opened for
a partitioned table within create_estate_for_relation() when
redirecting to the tuple routing, but my guess is that this would be
better in the long-term.

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.

@@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
slot_getallattrs(remoteslot);
}
MemoryContextSwitchTo(oldctx);
+
+ ExecOpenIndices(partrelinfo_new, false);
apply_handle_insert_internal(partrelinfo_new, estate,
remoteslot_part);
+ ExecCloseIndices(partrelinfo_new);
}

It seems you forgot to call open indexes before apply_handle_delete_internal.

I am not sure if it is a good idea to do the refactoring related to
indexes or other things to fix a minor bug in commit 1375422c. It
might be better to add a simple fix like what Hou-San has originally
proposed [1] because even using ExecInitResultRelation might not be
the best thing as it is again trying to open a range table which we
have already opened in logicalrep_rel_open.

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 apply_handle_{insert|update|delete} initialize themselves to
es_opened_result_relations. I would prefer that only
ExecInitResultRelation() add anything to es_opened_result_relations()
to avoid future maintenance problems. Instead, a fix as simple as the
Hou-san's proposed fix would be to add a ExecCloseResultRelations()
call at the end of each of apply_handle_{insert|update|delete}.

Yeah, that will work too but might look a bit strange. BTW, how that
is taken care of for ExecuteTruncateGuts? I mean we do add rels there
like Hou-San's patch without calling ExecCloseResultRelations, the
rels are probably closed when we close the relation in worker.c but
what about memory for the list?

It seems I had forgotten the code I had written myself. The following
is from ExecuteTruncateGuts():

/*
* To fire triggers, we'll need an EState as well as a ResultRelInfo for
* each relation. We don't need to call ExecOpenIndices, though.
*
* We put the ResultRelInfos in the es_opened_result_relations list, even
* though we don't have a range table and don't populate the
* es_result_relations array. That's a bit bogus, but it's enough to make
* ExecGetTriggerResultRel() find them.
*/
estate = CreateExecutorState();
resultRelInfos = (ResultRelInfo *)
palloc(list_length(rels) * sizeof(ResultRelInfo));
resultRelInfo = resultRelInfos;
foreach(cell, rels)
{
Relation rel = (Relation) lfirst(cell);

InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
NULL,
0);
estate->es_opened_result_relations =
lappend(estate->es_opened_result_relations, resultRelInfo);
resultRelInfo++;
}

So, that is exactly what Hou-san's patch did. Although, the comment
does admit that doing this is a bit bogus and maybe written (by Heikki
IIRC) as a caution against repeating the pattern.

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