From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Ondřej Jirman <ienieghapheoghaiwida@xff.cz>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16129: Segfault in tts_virtual_materialize in logical replication worker
Date: Thu, 21 Nov 2019 17:37:25 -0500
Message-ID: <13122.1574375845@sss.pgh.pa.us> (raw)
In-Reply-To: <20191121220854.ebdzjtloel56sfwu@development>
References: <20191121135523.aybyo7o6yzxxungd@core.my.home>
<20191121143503.nkdwgjwb7xjopeuq@development>
<20191121150716.szxqukxrtpjpckm3@core.my.home>
<20191121152732.hu6y4wzw7nkxpsvk@development>
<20191121155707.pqkgjphh65o5fapz@core.my.home>
<20191121161502.xfiy65i3cm6adscr@core.my.home>
<20191121165752.dffge6bh756xlfdg@development>
<20191121173555.midj6vp4dmiiqs24@core.my.home>
<20191121185844.6qsqhwzzuhu3fpsg@core.my.home>
<20191121214115.3tgefmvkmrzaim74@development>
<20191121220854.ebdzjtloel56sfwu@development>
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> FWIW my hunch is the bug is somewhere in this chunk of code from
> apply_heap_update:
> oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
> ExecCopySlot(remoteslot, localslot);
> slot_modify_cstrings(remoteslot, rel, newtup.values, newtup.changed);
> MemoryContextSwitchTo(oldctx);
My first reaction to looking at this was that ExecCopySlot() could
be dropped entirely, because the first thing that slot_modify_cstrings
does is
slot_getallattrs(slot);
ExecClearTuple(slot);
That slot_getallattrs call seems 100% useless as well, because
surely ExecClearTuple is just going to drop all the data.
On closer inspection, though, it looks like the author of this
code is relying on ExecClearTuple to *not* destroy the data,
because the subsequent loop might overwrite only some of the
slot's columns, before it does
ExecStoreVirtualTuple(slot);
which supposes that all the columns are valid.
But of course this is 100% broken, because whatever ExecClearTuple
may have done or not done to the slot's datum/isnull arrays, it
certainly pfree'd the slot's physical tuple. So any pass-by-ref
datums are now dangling pointers.
I imagine the only reason this code has gotten past the valgrind
animals is that we're not testing cases where non-replaced columns
in the subscriber table are of pass-by-ref types.
I draw a direct line between the existence of this bug and the lack
of commentary in slot_modify_cstrings about what it's doing. If the
author had troubled to write a comment explaining these machinations,
he might've noticed the bug, or at least reviewers might have.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, ienieghapheoghaiwida@xff.cz, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16129: Segfault in tts_virtual_materialize in logical replication worker
In-Reply-To: <13122.1574375845@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox