Re: Forget close an open relation in ReorderBufferProcessTXN()

Hi,

On 2021-04-16 08:42:40 +0530, Amit Kapila wrote:
> I think it is because relation_open expects either caller to have a
> lock on the relation or don't use 'NoLock' lockmode. AFAIU, we don't
> need to acquire a lock on relation while decoding changes from WAL
> because it uses a historic snapshot to build a relcache entry and all
> the later changes to the rel are absorbed while decoding WAL.

Right.

> 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...

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.

Greetings,

Andres Freund