Re: Forget close an open relation in ReorderBufferProcessTXN()

On Fri, Apr 16, 2021 at 11:24 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > I think it is also important to *not* acquire any lock on relation
> > otherwise it can lead to some sort of deadlock or infinite wait in the
> > decoding process. Consider a case for operations like Truncate (or if
> > the user has acquired an exclusive lock on the relation in some other
> > way say via Lock command) which acquires an exclusive lock on
> > relation, it won't get replicated in synchronous mode (when
> > synchronous_standby_name is configured). The truncate operation will
> > wait for the transaction to be replicated to the subscriber and the
> > decoding process will wait for the Truncate operation to finish.
>
> However, this cannot be really relied upon for catalog tables. An output
> function might acquire locks or such. But for those we do not need to
> decode contents...
>

True, so, if we don't need to decode contents then we won't have the
problems of the above kind.

>
>
> This made me take a brief look at pgoutput.c - maybe I am missing
> something, but how is the following not a memory leak?
>
> static void
> maybe_send_schema(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn, ReorderBufferChange *change,
> Relation relation, RelationSyncEntry *relentry)
> {
> ...
> /* Map must live as long as the session does. */
> oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> relentry->map = convert_tuples_by_name(CreateTupleDescCopy(indesc),
> CreateTupleDescCopy(outdesc));
> MemoryContextSwitchTo(oldctx);
> send_relation_and_attrs(ancestor, xid, ctx);
> RelationClose(ancestor);
>
> If - and that's common - convert_tuples_by_name() won't have to do
> anything, the copied tuple descs will be permanently leaked.
>

I also think this is a permanent leak. I think we need to free all the
memory associated with this map on the invalidation of this particular
relsync entry (basically in rel_sync_cache_relation_cb).

--
With Regards,
Amit Kapila.