Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <pgsql-bugs-owner+archive@lists.postgresql.org>)
	id 1iYC8J-0003O9-KQ
	for pgsql-bugs@arkaria.postgresql.org; Fri, 22 Nov 2019 16:50:11 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.89)
	(envelope-from <pgsql-bugs-owner+archive@lists.postgresql.org>)
	id 1iYC8I-0001Xl-03
	for pgsql-bugs@arkaria.postgresql.org; Fri, 22 Nov 2019 16:50:10 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1iYC8H-0001XP-LW
	for pgsql-bugs@lists.postgresql.org; Fri, 22 Nov 2019 16:50:09 +0000
Received: from sss.pgh.pa.us ([66.207.139.130])
	by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1iYC8F-0008Aj-5r
	for pgsql-bugs@lists.postgresql.org; Fri, 22 Nov 2019 16:50:08 +0000
Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1])
	by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id xAMGo3Uc026016;
	Fri, 22 Nov 2019 11:50:03 -0500
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
cc: =?utf-8?Q?Ond=C5=99ej?= Jirman <ienieghapheoghaiwida@xff.cz>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16129: Segfault in tts_virtual_materialize in logical
 replication worker
In-reply-to: <20191122161248.qnzeoh6t7fvg5wl3@development>
References: <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>
 <20191122003626.ydgwvjfx363blgip@core.my.home>
 <16189.1574383534@sss.pgh.pa.us>
 <20191122005401.bkdpmtqwuu6mvnyg@core.my.home>
 <20191122161248.qnzeoh6t7fvg5wl3@development>
Comments: In-reply-to Tomas Vondra <tomas.vondra@2ndquadrant.com>
	message dated "Fri, 22 Nov 2019 17:12:48 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <26014.1574441403.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 22 Nov 2019 11:50:03 -0500
Message-ID: <26015.1574441403@sss.pgh.pa.us>
List-Id: <pgsql-bugs.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-bugs@lists.postgresql.org>
List-Owner: <mailto:pgsql-bugs-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-bugs>
Precedence: bulk

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On Fri, Nov 22, 2019 at 01:54:01AM +0100, Ond=C5=99ej Jirman wrote:
>> On Thu, Nov 21, 2019 at 07:45:34PM -0500, Tom Lane wrote:
>>> =3D?utf-8?Q?Ond=3DC5=3D99ej?=3D Jirman <ienieghapheoghaiwida@xff.cz> w=
rites:
>>>> newtup.changed
>>>> {true, true, false, true, true, true, true, true, false <repeats 1656=
 times>}

>>> So column 3 is not getting replaced.

> Can you show us the attribute list as defined in the system, including
> e.g. dropped columns? That is, something like
>    SELECT attnum, attname, atttypid FROM pg_attribute
>     WHERE attrelid =3D 'public.videos'::regclass;
> both from the published and subscriber.

After digging around a bit more in the logrep code, it seems like we'd
only have a situation with newtup.changed[i] =3D false if the publisher
had hit this bit in logicalrep_write_tuple:

        else if (att->attlen =3D=3D -1 && VARATT_IS_EXTERNAL_ONDISK(values=
[i]))
        {
            pq_sendbyte(out, 'u');    /* unchanged toast column */
            continue;
        }

So this seems to indicate that Ond=C5=99ej's case involves an update on a =
row
that contained a toasted-out-of-line column that did not get updated.
That's different from the trigger conditions that I postulated, but the
end result is the same: slot_modify_cstrings would have a case where
it's supposed to retain the old data for a pass-by-ref column, and it'd
fail to do that correctly.

I also discovered that before v12, the calling code looked like this:

        ExecStoreTuple(localslot->tts_tuple, remoteslot, InvalidBuffer, fa=
lse);
        slot_modify_cstrings(remoteslot, rel, newtup.values, newtup.change=
d);

so that at that time, "remoteslot" indeed did not have its own local
copy of the tuple, and thus the code failed to fail.  Perhaps this was
intentional on the part of whoever wrote this, but it's still an
unacceptable abuse of the API if you ask me.

I added some tests involving dropped columns to what I had last night
and pushed it.   However, looking at this now, I see that we really
still don't have enough coverage --- the code path I showed above
is not hit by the regression tests, according to the code coverage
bot.  I don't think it's really acceptable for such a corner case
bit of the logrep protocol to not get tested :-(

			regards, tom lane