Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitb72a44c

Browse files
committed
Fix bogus tuple-slot management in logical replication UPDATE handling.
slot_modify_cstrings seriously abused the TupleTableSlot API by relyingon a slot's underlying data to stay valid across ExecClearTuple. Sincethis abuse was also quite undocumented, it's little surprise that thecase got broken during the v12 slot rewrites. As reported in bug #16129from Ondřej Jirman, this could lead to crashes or data corruption whena logical replication subscriber processes a row update. Problems wouldonly arise if the subscriber's table contained columns of pass-by-reftypes that were not being copied from the publisher.Fix by explicitly copying the datum/isnull arrays from the source slotthat the old row was in already. This ends up being about the samething that happened pre-v12, but hopefully in a less opaque andfragile way.We might've caught the problem sooner if there were any test casesdealing with updates involving non-replicated or dropped columns.Now there are.Back-patch to v10 where this code came in. Even though the failuredoes not manifest before v12, IMO this code is too fragile to leaveas-is. In any case we certainly want the additional test coverage.Patch by me; thanks to Tomas Vondra for initial investigation.Discussion:https://postgr.es/m/16129-a0c0f48e71741e5f@postgresql.org
1 parent669138e commitb72a44c

File tree

2 files changed

+70
-17
lines changed

2 files changed

+70
-17
lines changed

‎src/backend/replication/logical/worker.c‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -380,24 +380,39 @@ slot_store_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
380380
}
381381

382382
/*
383-
*Modify slot with user data provided as C strings.
383+
*Replace selected columns with user data provided as C strings.
384384
* This is somewhat similar to heap_modify_tuple but also calls the type
385-
* input function on the user data as the input is the text representation
386-
* of the types.
385+
* input functions on the user data.
386+
* "slot" is filled with a copy of the tuple in "srcslot", with
387+
* columns selected by the "replaces" array replaced with data values
388+
* from "values".
389+
* Caution: unreplaced pass-by-ref columns in "slot" will point into the
390+
* storage for "srcslot". This is OK for current usage, but someday we may
391+
* need to materialize "slot" at the end to make it independent of "srcslot".
387392
*/
388393
staticvoid
389-
slot_modify_cstrings(TupleTableSlot*slot,LogicalRepRelMapEntry*rel,
394+
slot_modify_cstrings(TupleTableSlot*slot,TupleTableSlot*srcslot,
395+
LogicalRepRelMapEntry*rel,
390396
char**values,bool*replaces)
391397
{
392398
intnatts=slot->tts_tupleDescriptor->natts;
393399
inti;
394400
SlotErrCallbackArgerrarg;
395401
ErrorContextCallbackerrcallback;
396402

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

400-
/* Push callback + info on the error context stack */
406+
/*
407+
* Copy all the column data from srcslot, so that we'll have valid values
408+
* for unreplaced columns.
409+
*/
410+
Assert(natts==srcslot->tts_tupleDescriptor->natts);
411+
slot_getallattrs(srcslot);
412+
memcpy(slot->tts_values,srcslot->tts_values,natts*sizeof(Datum));
413+
memcpy(slot->tts_isnull,srcslot->tts_isnull,natts*sizeof(bool));
414+
415+
/* For error reporting, push callback + info on the error context stack */
401416
errarg.rel=rel;
402417
errarg.local_attnum=-1;
403418
errarg.remote_attnum=-1;
@@ -445,6 +460,7 @@ slot_modify_cstrings(TupleTableSlot *slot, LogicalRepRelMapEntry *rel,
445460
/* Pop the error context stack */
446461
error_context_stack=errcallback.previous;
447462

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

@@ -755,8 +771,8 @@ apply_handle_update(StringInfo s)
755771
{
756772
/* Process and store remote tuple in the slot */
757773
oldctx=MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
758-
ExecStoreTuple(localslot->tts_tuple,remoteslot,InvalidBuffer, false);
759-
slot_modify_cstrings(remoteslot,rel,newtup.values,newtup.changed);
774+
slot_modify_cstrings(remoteslot,localslot,rel,
775+
newtup.values,newtup.changed);
760776
MemoryContextSwitchTo(oldctx);
761777

762778
EvalPlanQualSetSlot(&epqstate,remoteslot);

‎src/test/subscription/t/001_rep_changes.pl‎

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
use warnings;
44
use PostgresNode;
55
use TestLib;
6-
use Test::Moretests=>17;
6+
use Test::Moretests=>20;
77

88
# Initialize publisher node
99
my$node_publisher = get_new_node('publisher');
@@ -28,9 +28,9 @@
2828
$node_publisher->safe_psql('postgres',
2929
"CREATE TABLE tab_rep (a int primary key)");
3030
$node_publisher->safe_psql('postgres',
31-
"CREATE TABLE tab_mixed (a int primary key, b text)");
31+
"CREATE TABLE tab_mixed (a int primary key, b text, c numeric)");
3232
$node_publisher->safe_psql('postgres',
33-
"INSERT INTO tab_mixed (a, b) VALUES (1, 'foo')");
33+
"INSERT INTO tab_mixed (a, b, c) VALUES (1, 'foo', 1.1)");
3434
$node_publisher->safe_psql('postgres',
3535
"CREATE TABLE tab_include (a int, b text, CONSTRAINT covering PRIMARY KEY(a) INCLUDE(b))"
3636
);
@@ -45,7 +45,8 @@
4545

4646
# different column count and order than on publisher
4747
$node_subscriber->safe_psql('postgres',
48-
"CREATE TABLE tab_mixed (c text, b text, a int primary key)");
48+
"CREATE TABLE tab_mixed (d text default 'local', c numeric, b text, a int primary key)"
49+
);
4950

5051
# replication of the table with included index
5152
$node_subscriber->safe_psql('postgres',
@@ -95,7 +96,7 @@
9596
$node_publisher->safe_psql('postgres',"UPDATE tab_rep SET a = -a");
9697

9798
$node_publisher->safe_psql('postgres',
98-
"INSERT INTO tab_mixed VALUES (2, 'bar')");
99+
"INSERT INTO tab_mixed VALUES (2, 'bar', 2.2)");
99100

100101
$node_publisher->safe_psql('postgres',
101102
"INSERT INTO tab_include SELECT generate_series(1,50)");
@@ -113,10 +114,9 @@
113114
"SELECT count(*), min(a), max(a) FROM tab_rep");
114115
is($result,qq(20|-20|-1),'check replicated changes on subscriber');
115116

116-
$result =
117-
$node_subscriber->safe_psql('postgres',"SELECT c, b, a FROM tab_mixed");
118-
is($result,qq(|foo|1
119-
|bar|2),'check replicated changes with different column order');
117+
$result =$node_subscriber->safe_psql('postgres',"SELECT * FROM tab_mixed");
118+
is($result,qq(local|1.1|foo|1
119+
local|2.2|bar|2),'check replicated changes with different column order');
120120

121121
$result =$node_subscriber->safe_psql('postgres',
122122
"SELECT count(*), min(a), max(a) FROM tab_include");
@@ -140,11 +140,14 @@
140140
"ALTER TABLE tab_ins REPLICA IDENTITY FULL");
141141
$node_subscriber->safe_psql('postgres',
142142
"ALTER TABLE tab_ins REPLICA IDENTITY FULL");
143+
# tab_mixed can use DEFAULT, since it has a primary key
143144

144145
# and do the updates
145146
$node_publisher->safe_psql('postgres',"UPDATE tab_full SET a = a * a");
146147
$node_publisher->safe_psql('postgres',
147148
"UPDATE tab_full2 SET x = 'bb' WHERE x = 'b'");
149+
$node_publisher->safe_psql('postgres',
150+
"UPDATE tab_mixed SET b = 'baz' WHERE a = 1");
148151

149152
$node_publisher->wait_for_catchup($appname);
150153

@@ -160,6 +163,40 @@
160163
bb),
161164
'update works with REPLICA IDENTITY FULL and text datums');
162165

166+
$result =$node_subscriber->safe_psql('postgres',
167+
"SELECT * FROM tab_mixed ORDER BY a");
168+
is($result,qq(local|1.1|baz|1
169+
local|2.2|bar|2),
170+
'update works with different column order and subscriber local values');
171+
172+
# check behavior with dropped columns
173+
174+
$node_publisher->safe_psql('postgres',"ALTER TABLE tab_mixed DROP COLUMN b");
175+
$node_publisher->safe_psql('postgres',
176+
"UPDATE tab_mixed SET c = 11.11 WHERE a = 1");
177+
178+
$node_publisher->wait_for_catchup($appname);
179+
180+
$result =$node_subscriber->safe_psql('postgres',
181+
"SELECT * FROM tab_mixed ORDER BY a");
182+
is($result,qq(local|11.11|baz|1
183+
local|2.2|bar|2),
184+
'update works with dropped publisher column');
185+
186+
$node_subscriber->safe_psql('postgres',
187+
"ALTER TABLE tab_mixed DROP COLUMN d");
188+
189+
$node_publisher->safe_psql('postgres',
190+
"UPDATE tab_mixed SET c = 22.22 WHERE a = 2");
191+
192+
$node_publisher->wait_for_catchup($appname);
193+
194+
$result =$node_subscriber->safe_psql('postgres',
195+
"SELECT * FROM tab_mixed ORDER BY a");
196+
is($result,qq(11.11|baz|1
197+
22.22|bar|2),
198+
'update works with dropped subscriber column');
199+
163200
# check that change of connection string and/or publication list causes
164201
# restart of subscription workers. Not all of these are registered as tests
165202
# as we need to poll for a change but the test suite will fail none the less

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp