Re: Table refer leak in logical replication

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 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. 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] - https://www.postgresql.org/message-id/OS0PR01MB571686F75FBDC219FF3DFF0D94769%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.