On Fri, May 21, 2021 at 1:12 PM Amit Langote <amitlangote09@gmail.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.
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?
--
With Regards,
Amit Kapila.