Re: Table refer leak in logical replication

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