Re: Memory context can be its own parent and child in replication command

Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com> writes:
> I've updated the patch with another approach:

I got back to looking at this today. I still don't like very much
about it. After thinking for awhile, I concur that we want to create
the cmd_context under TopMemoryContext, but I think we can make things
a great deal simpler and less fragile by just keeping it in existence
indefinitely, as attached. The fundamental problem here is that
exec_replication_command is creating and deleting the context without
regard to where transaction boundaries are. I don't think the best
way to deal with that is to require it to know where those boundaries
are.

I also don't like the proposed test script. The test case alone would
be fine, but spinning up an entire new instance seems a rather huge
overhead to carry forevermore for a single test that likely will never
find anything again. I tried to cram the test case into one of the
existing scripts, but it was hard to find one where it would fit
naturally. An obvious candidate is subscription/t/100_bugs.pl, but
the test failed there for lack of access to the test_decoding plugin.
Perhaps we could use another plugin, but I didn't try hard.

As for the memory-context-bug-catching changes, I'm not very convinced
by the idea of setting context->type = T_Invalid. That would help
if someone tries to reference the context while it's still in the
freelist, but not if the reference is from re-use of a stale pointer
after the context has been reassigned (which IIUC is the case here).
I also think that decorating a few MemoryContextSwitchTos with
assertions looks pretty random. I wonder if we should content
ourselves with adding some assertions that catch attempts to make
a context be its own parent. It looks like MemoryContextSetParent
already does that, so the only new assert would be in
MemoryContextCreate.

regards, tom lane