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: <13122.1574375845@sss.pgh.pa.us>
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>
 <13122.1574375845@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Thu, 21 Nov 2019 17:37:25 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <13187.1574379184.0@sss.pgh.pa.us>
Date: Thu, 21 Nov 2019 18:34:18 -0500
Message-ID: <13241.1574379258@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13187.1574379184.1@sss.pgh.pa.us>

I wrote:
> 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);

> 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.

Actually, it doesn't appear to me that we're testing this with
any non-replaced columns at all.  The test modifications in the
attached proposed patch add that.  For me, the unpatched code
doesn't crash with this test, but the non-replaced column reads
back as empty which is certainly wrong.  Valgrind would likely
complain too, but I didn't try it.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-bogus-slot-manipulation-in-logical-rep.patch";
	charset="us-ascii"
Content-ID: <13187.1574379184.2@sss.pgh.pa.us>
Content-Description: fix-bogus-slot-manipulation-in-logical-rep.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replic=
ation/logical/worker.c
index ff62303..9c06b67 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -363,13 +363,19 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRep=
RelMapEntry *rel,
 }
 =

 /*
- * Modify slot with user data provided as C strings.
+ * Replace selected columns with user data provided as C strings.
  * This is somewhat similar to heap_modify_tuple but also calls the type
- * input function on the user data as the input is the text representatio=
n
- * of the types.
+ * input functions on the user data.
+ * "slot" is filled with a copy of the tuple in "srcslot", with
+ * columns selected by the "replaces" array replaced with data values
+ * from "values".
+ * Caution: unreplaced pass-by-ref columns in "slot" will point into the
+ * storage for "srcslot".  This is OK for current usage, but someday we m=
ay
+ * need to materialize "slot" at the end to make it independent of "srcsl=
ot".
  */
 static void
-slot_modify_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
+slot_modify_cstrings(TupleTableSlot *slot, TupleTableSlot *srcslot,
+					 LogicalRepRelMapEntry *rel,
 					 char **values, bool *replaces)
 {
 	int			natts =3D slot->tts_tupleDescriptor->natts;
@@ -377,9 +383,18 @@ slot_modify_cstrings(TupleTableSlot *slot, LogicalRep=
RelMapEntry *rel,
 	SlotErrCallbackArg errarg;
 	ErrorContextCallback errcallback;
 =

-	slot_getallattrs(slot);
+	/* We'll fill "slot" with a virtual tuple, so we must start with ... */
 	ExecClearTuple(slot);
 =

+	/*
+	 * Transfer all the column data from srcslot, so that we have valid valu=
es
+	 * for unreplaced columns.
+	 */
+	Assert(natts =3D=3D srcslot->tts_tupleDescriptor->natts);
+	slot_getallattrs(srcslot);
+	memcpy(slot->tts_values, srcslot->tts_values, natts * sizeof(Datum));
+	memcpy(slot->tts_isnull, srcslot->tts_isnull, natts * sizeof(bool));
+
 	/* Push callback + info on the error context stack */
 	errarg.rel =3D rel;
 	errarg.local_attnum =3D -1;
@@ -428,6 +443,7 @@ slot_modify_cstrings(TupleTableSlot *slot, LogicalRepR=
elMapEntry *rel,
 	/* Pop the error context stack */
 	error_context_stack =3D errcallback.previous;
 =

+	/* And finally, declare that "slot" contains a valid virtual tuple */
 	ExecStoreVirtualTuple(slot);
 }
 =

@@ -740,8 +756,8 @@ apply_handle_update(StringInfo s)
 	{
 		/* Process and store remote tuple in the slot */
 		oldctx =3D MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
-		ExecCopySlot(remoteslot, localslot);
-		slot_modify_cstrings(remoteslot, rel, newtup.values, newtup.changed);
+		slot_modify_cstrings(remoteslot, localslot, rel,
+							 newtup.values, newtup.changed);
 		MemoryContextSwitchTo(oldctx);
 =

 		EvalPlanQualSetSlot(&epqstate, remoteslot);
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscri=
ption/t/001_rep_changes.pl
index 40e306a..116e487 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests =3D> 17;
+use Test::More tests =3D> 18;
 =

 # Initialize publisher node
 my $node_publisher =3D get_new_node('publisher');
@@ -45,7 +45,7 @@ $node_subscriber->safe_psql('postgres',
 =

 # different column count and order than on publisher
 $node_subscriber->safe_psql('postgres',
-	"CREATE TABLE tab_mixed (c text, b text, a int primary key)");
+	"CREATE TABLE tab_mixed (c text default 'local', b text, a int primary k=
ey)");
 =

 # replication of the table with included index
 $node_subscriber->safe_psql('postgres',
@@ -114,8 +114,8 @@ is($result, qq(20|-20|-1), 'check replicated changes o=
n subscriber');
 =

 $result =3D
   $node_subscriber->safe_psql('postgres', "SELECT c, b, a FROM tab_mixed"=
);
-is( $result, qq(|foo|1
-|bar|2), 'check replicated changes with different column order');
+is( $result, qq(local|foo|1
+local|bar|2), 'check replicated changes with different column order');
 =

 $result =3D $node_subscriber->safe_psql('postgres',
 	"SELECT count(*), min(a), max(a) FROM tab_include");
@@ -139,11 +139,14 @@ $node_publisher->safe_psql('postgres',
 	"ALTER TABLE tab_ins REPLICA IDENTITY FULL");
 $node_subscriber->safe_psql('postgres',
 	"ALTER TABLE tab_ins REPLICA IDENTITY FULL");
+# tab_mixed can use DEFAULT, since it has a primary key
 =

 # and do the updates
 $node_publisher->safe_psql('postgres', "UPDATE tab_full SET a =3D a * a")=
;
 $node_publisher->safe_psql('postgres',
 	"UPDATE tab_full2 SET x =3D 'bb' WHERE x =3D 'b'");
+$node_publisher->safe_psql('postgres',
+	"UPDATE tab_mixed SET b =3D 'baz' WHERE a =3D 1");
 =

 $node_publisher->wait_for_catchup('tap_sub');
 =

@@ -159,6 +162,12 @@ bb
 bb),
 	'update works with REPLICA IDENTITY FULL and text datums');
 =

+$result =3D $node_subscriber->safe_psql('postgres',
+	"SELECT * FROM tab_mixed ORDER BY a");
+is( $result, qq(local|baz|1
+local|bar|2),
+	'update works with different column order and subscriber local values');
+
 # check that change of connection string and/or publication list causes
 # restart of subscription workers. Not all of these are registered as tes=
ts
 # as we need to poll for a change but the test suite will fail none the l=
ess

------- =_aaaaaaaaaa0--