Re: Forget close an open relation in ReorderBufferProcessTXN()

On Thu, May 27, 2021 at 3:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, May 21, 2021 at 1:12 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > Hmm, maybe get_rel_syn_entry() should explicitly set map to NULL when
> > first initializing an entry. It's possible that without doing so, the
> > map remains set to a garbage value, which causes the invalidation
> > callback that runs into such partially initialized entry to segfault
> > upon trying to deference that garbage pointer.
> >
> > I've tried that in the attached v6 patches. Please check.
> >
>
> v6-0001
> =========
> + send_relation_and_attrs(ancestor, xid, ctx);
> +
> /* Map must live as long as the session does. */
> oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> - relentry->map = convert_tuples_by_name(CreateTupleDescCopy(indesc),
> - CreateTupleDescCopy(outdesc));
> +
> + /*
> + * Make copies of the TupleDescs that will live as long as the map
> + * does before putting into the map.
> + */
> + indesc = CreateTupleDescCopy(indesc);
> + outdesc = CreateTupleDescCopy(outdesc);
> + relentry->map = convert_tuples_by_name(indesc, outdesc);
> + if (relentry->map == NULL)
> + {
> + /* Map not necessary, so free the TupleDescs too. */
> + FreeTupleDesc(indesc);
> + FreeTupleDesc(outdesc);
> + }
> +
> MemoryContextSwitchTo(oldctx);
> - send_relation_and_attrs(ancestor, xid, ctx);
>
> Why do we need to move send_relation_and_attrs() call? I think it
> doesn't matter much either way but OTOH, in the existing code, if
> there is an error (say 'out of memory' or some other) while building
> the map, we won't send relation attrs whereas with your change we will
> unnecessarily send those in such a case.

That's a good point. I've reverted that change in the attached.

> I feel there is no need to backpatch v6-0002. We can just make it a
> HEAD-only change as that doesn't cause any bug even though it is
> better not to send it. If we consider it as a HEAD-only change then
> probably we can register it for PG-15. What do you think?

Okay, I will see about creating a PG15 CF entry for 0002.

Please see attached v7-0001 with the part mentioned above fixed.

--
Amit Langote
EDB: http://www.enterprisedb.com