database/mariadb
1
0
Fork 0
mirror of https://github.com/MariaDB/server.git synced 2026-10-23 18:49:29 +03:00
Code Activity

MDEV-39143 follow-up: address PR review comments

Browse Source 
Two review comments on 88f91a4bf8 (fix review findings in MySQL JSON
replication):

Naming/comment clarity in apply_partial_json_column():
 - Renamed to convert_partial_json_column(), deliberately echoing
   convert_field(): it is called instead of unpack_field() +
   convert_field() in unpack_row()'s per-column loop, not in addition
   to them, and goes no further than they do - it populates
   result_field in table->record[0] and touches the storage engine
   exactly as much as convert_field() does, which is not at all. The
   previous name and its "stored ... directly" wording read as if
   something were committed to the table here, which it is not; the
   actual ha_write_row()/ha_update_row() happens later, in the
   caller's caller, identically for every column regardless of how
   its value was computed.
 - Rewrote the function comment to say this explicitly up front, and
   to explain why a Json_diff vector cannot go through convert_field()
   itself: that function's Copy_field-based conversion takes one
   source field's value and converts it into one target field, with
   no way to also consult a third input, but applying a diff needs
   exactly that - the column's own current value on the slave, which
   a Copy_field conversion never needs for any other type. MySQL's own
   apply_json_diffs()/Field_json::unpack_diff() sit outside its
   generic field-conversion machinery for the same reason.

Fixture reproducibility in rpl_json_partial_update.test:
 - The test's header only said the fixture std_data/
   mdev39143_partial_json.000001 was "recorded once on a real MySQL
   9.2 server", without describing its table, contents, or the
   binlog configuration needed to regenerate it. Row-based
   replication does not preserve the original SQL in the binlog, only
   its effect, so recovering that description meant decoding the
   fixture's events by hand (event headers, Table_map column
   metadata, and the raw JSONB/Json_diff bytes of each row event).
 - The header now documents the table (testdb.tj (id INT PRIMARY KEY,
   j JSON) ENGINE=InnoDB), the binlog configuration
   (binlog_format=ROW, binlog_row_value_options=PARTIAL_JSON, default
   binlog_row_image=FULL), and the equivalent SQL recording script,
   verified against the fixture's actual decoded content and the
   test's own assertions. This also surfaced that the fixture
   interleaves an unrelated second row (inserted, fully updated, then
   deleted) as noise alongside the row under test, which the test's
   final SELECT never checks - now called out explicitly instead of
   being invisible.
 - Fixed a stale comment in both rpl_json_partial_update.test and
   rpl_json_partial_insert.test left over from an earlier change:
   each referenced a per-test -slave.opt file for loading the
   mysql_json plugin that no longer exists, since the plugin is now
   auto-loaded via have_type_mysql_json.inc's companion .opt file.

Assisted-by: Claude AI
This commit is contained in:
Pekka Lampio
2026-10-20 12:02:07 +03:00
parent 88f91a4bf8
commit a3cd94a2dc
3 changed files with 89 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
mysql-test/suite/rpl/t/rpl_json_partial_insert.test
View File

@@ -50,9 +50,9 @@
--source include/master-slave.inc
# The slave decodes MySQL binary JSON with the read-only mysql_json plugin,
# loaded on the slave only via rpl_json_partial_insert-slave.opt (it never
# needs to be active on the master, which only ever serves the injected
# fixture's raw bytes).
# auto-loaded on the slave via have_type_mysql_json.inc's companion .opt
# file (it never needs to be active on the master, which only ever serves
# the injected fixture's raw bytes).
--connection slave
--source include/stop_slave.inc

46
mysql-test/suite/rpl/t/rpl_json_partial_update.test
View File

@@ -11,11 +11,45 @@
# slave apply path: PARTIAL_UPDATE_ROWS_EVENT (39) parsing, Json_diff
# decoding, and diff application - see unpack_row() in sql/rpl_record.cc.
#
# The recorded events are fixed: the SQL the MySQL master ran is not stored
# in the binlog in row format, only its effect, so it cannot be recovered
# from the file itself. What follows is that effect decoded by hand (event
# headers, Table_map column metadata, and the raw JSONB/Json_diff bytes of
# each row/update/partial-update event), written back as the equivalent SQL
# that reproduces it - this is what a re-recording needs to match:
#
# -- MySQL 9.2, binlog_format=ROW, default binlog_row_image=FULL.
# SET SESSION binlog_row_value_options= 'PARTIAL_JSON';
# CREATE DATABASE IF NOT EXISTS testdb;
# CREATE TABLE testdb.tj (id INT PRIMARY KEY, j JSON) ENGINE=InnoDB;
# -- id=2 (inserted, fully updated, then deleted) is interleaved purely as
# -- realism/noise alongside id=1's partial updates below - the slave must
# -- apply ordinary WRITE_ROWS/UPDATE_ROWS/DELETE_ROWS events on the same
# -- table correctly too, not just PARTIAL_UPDATE_ROWS_EVENT. Its outcome
# -- is not itself asserted by this test (id=2 is gone by the final
# -- SELECT). The large 'pad' members exist so the base document is big
# -- enough that MySQL's diffs against it stay small - the point being
# -- tested, even though PARTIAL_JSON always logs a diff when enabled,
# -- regardless of document size.
# INSERT INTO testdb.tj VALUES
# (1, JSON_OBJECT('a','xxxx','n',42,'arr',JSON_ARRAY(1,2,3),
# 'pad',REPEAT('z',400)));
# INSERT INTO testdb.tj VALUES
# (2, JSON_OBJECT('a','q','pad',REPEAT('w',400)));
# UPDATE testdb.tj SET j=JSON_OBJECT('x',JSON_ARRAY(1,2),'full','true')
# WHERE id=2;
# DELETE FROM testdb.tj WHERE id=2;
# -- From here on, each UPDATE on id=1 logs as one PARTIAL_UPDATE_ROWS_EVENT
# -- carrying only the diff below, not the (padded) document.
# UPDATE testdb.tj SET j=JSON_REPLACE(j,'$.a','yyyy') WHERE id=1;
# UPDATE testdb.tj SET j=JSON_REPLACE(j,'$.n',43) WHERE id=1;
# UPDATE testdb.tj SET j=JSON_REMOVE(j,'$.arr[2]') WHERE id=1;
# UPDATE testdb.tj SET j=JSON_REPLACE(j,'$.a','zzzz','$.n',44) WHERE id=1;
#
# Important points:
# - The recorded events are fixed. The SQL the MySQL master ran is not
# visible in this file, only its effect (described at the SELECT below);
# extending coverage needs a MySQL server, a re-recording, and a new file.
# - The Json_diff encoding exercised is MySQL 9.2's specifically.
# - Extending coverage needs a MySQL server, a re-recording along the lines
# above, and a new file - the recorded events themselves cannot be edited.
# - have_binlog_format_row.inc constrains the MariaDB master's own binlog
# format; the injected events are row events regardless of it.
--source include/have_innodb.inc
@@ -24,9 +58,9 @@
--source include/master-slave.inc
# The slave decodes MySQL binary JSON with the read-only mysql_json plugin,
# loaded on the slave only via rpl_json_partial_update-slave.opt (it never
# needs to be active on the master, which only ever serves the injected
# fixture's raw bytes).
# auto-loaded on the slave via have_type_mysql_json.inc's companion .opt
# file (it never needs to be active on the master, which only ever serves
# the injected fixture's raw bytes).
--connection slave
--source include/stop_slave.inc

72
sql/rpl_record.cc
View File

@@ -446,7 +446,7 @@ static bool read_mysql_json_diff_vector(const uchar **row_image_pos,
i.e. it was written as '[last]' or '[last-N]'. Such a
path is stored with a negative n_item. The caller
refuses these for now; see the note in
apply_partial_json_column().
convert_partial_json_column().
@retval false Path parsed; both out-parameters are set.
@retval true The path could not be parsed.
@@ -580,30 +580,49 @@ static void report_pj_apply_error(TABLE *table, Field *result_field,
/**
Apply one partial-JSON column of a MySQL PARTIAL_UPDATE_ROWS_EVENT
after-image.
Compute the new value of one partial-JSON column of a MySQL
PARTIAL_UPDATE_ROWS_EVENT after-image, and write it into result_field.
A partial column carries a Json_diff vector rather than a value, so it
cannot go through the normal unpack path: there is nothing to unpack into
the conversion table and copy out with convert_field(). Instead the base
document is taken from the matched row, the diffs are applied to it in
order, and the resulting text is stored into the slave field directly.
This occupies the same slot in unpack_row()'s per-column loop as
unpack_field() + convert_field() do for an ordinary column - called
instead of them, not in addition - and goes no further than they do: it
populates result_field in table->record[0], the same in-memory row
buffer being built for every other column, and touches the storage
engine exactly as much as convert_field() does, which is to say not at
all. The actual ha_write_row()/ha_update_row() happens later, in the
caller's caller (Rows_log_event::do_apply_event()/do_exec_row() in
log_event_server.cc), identically for every column regardless of how
its value was computed.
The base document is the *before-image*, which lives in record[1] while
record[0] is being built. The field is temporarily shifted there to read
it (move_field_offset()), matching MySQL, which applies diffs to the
A partial column carries a Json_diff vector rather than a value, though,
so it cannot go through convert_field() itself: that function's
Copy_field-based conversion takes one source field's value and converts
it into one target field, with no way to also consult a third input -
but applying a diff needs exactly that, the column's own *current* value
on the slave, which a Copy_field conversion never needs for any other
type. (MySQL's own apply_json_diffs()/Field_json::unpack_diff() -
sql/item_json_func.cc, sql/field.cc in a MySQL source tree - likewise
sit outside its generic field-conversion machinery, for the same
reason: there is no Field shape for "a sequence of patch operations".)
So instead, the diffs are applied to that current value one at a time
with MariaDB's own JSON functions, and the resulting text is what gets
written into result_field, via the same Field::store() every other
column's value eventually goes through too. That current value is the
row's *before-image*, which lives in record[1] while record[0] is being
built; the field is temporarily shifted there to read it
(move_field_offset()), matching MySQL, which applies diffs to the
field's current value.
Each diff is folded through MariaDB's own JSON functions, so the result
matches what the corresponding SQL function would produce:
Each diff is folded through MariaDB's own JSON functions in turn, one
feeding the next, so the result matches what the corresponding SQL
function would produce:
REPLACE -> JSON_REPLACE, INSERT -> JSON_INSERT, REMOVE -> JSON_REMOVE
applied one at a time, each result feeding the next. A diff value arrives
as MySQL binary JSON and is decoded to text by storing it into
\a conv_field (a Field_mysql_json) and reading it back; the text is then
wrapped in JSON_COMPACT() so that JSON_INSERT/JSON_REPLACE treat it as a
JSON value rather than as a quoted string.
A diff value arrives as MySQL binary JSON and is decoded to text by
storing it into \a conv_field (a Field_mysql_json) and reading it back;
the text is then wrapped in JSON_COMPACT() so that JSON_INSERT/
JSON_REPLACE treat it as a JSON value rather than as a quoted string.
The Items built for this are created on a private THD::free_list which is
released here, so their internal buffers do not accumulate until the end
@@ -652,10 +671,11 @@ static void report_pj_apply_error(TABLE *table, Field *result_field,
building/evaluating a JSON function failed.
Reported via report_pj_apply_error().
*/
static int apply_partial_json_column(const rpl_group_info *rgi, TABLE *table,
Field *result_field, Field *conv_field,
Unpack_record_state &state,
const uchar *row_end, bool only_seek)
static int convert_partial_json_column(const rpl_group_info *rgi,
TABLE *table, Field *result_field,
Field *conv_field,
Unpack_record_state &state,
const uchar *row_end, bool only_seek)
{
Dynamic_array<Mysql_json_diff> diffs((PSI_memory_key) PSI_NOT_INSTRUMENTED);
const uchar *ptr= state.pack_ptr;
@@ -986,7 +1006,7 @@ static int apply_partial_json_column(const rpl_group_info *rgi, TABLE *table,
together with is_partial_json_after_image: a partial JSON
column's diff vector is still decoded, to correctly
advance past it, but is discarded rather than applied -
see apply_partial_json_column(). Ignored otherwise, since
see convert_partial_json_column(). Ignored otherwise, since
ordinary field unpacking never reads record[1], so it is
harmless regardless of whether a real row was matched.
@@ -1201,9 +1221,9 @@ int unpack_row(const rpl_group_info *rgi, TABLE *table, uint const master_cols,
if (is_partial_col)
{
int pj_err= apply_partial_json_column(rgi, table, result_field,
conv_field, st, row_end,
only_seek);
int pj_err= convert_partial_json_column(rgi, table, result_field,
conv_field, st, row_end,
only_seek);
if (pj_err)
DBUG_RETURN(pj_err);
}