From ab736e99c51609008c415d313938b5ff11ce66d5 Mon Sep 17 00:00:00 2001 From: Vignesh C Date: Thu, 23 Apr 2026 17:48:09 +0530 Subject: [PATCH v4] replication: fix translation issues in tuple value detail messages append_tuple_value_detail() constructed user-visible messages using separately translated fragments such as ": ", ", ", and ".",. This makes correct translation difficult or impossible in some languages. Refactor append_tuple_value_detail() to move all punctuation and sentence construction to the callers, which now use a single translatable string with a %s placeholder for the tuple data. Reported-by: David Rowley Author: vignesh C Reviewed-by: Amit Kapila Reviewed-by: Zhijie Hou Reviewed-by: Peter Smith Reviewed-by: Hayato Kuroda Discussion: https://www.postgresql.org/message-id/flat/227279.1775956328%40sss.pgh.pa.us#8f3a5f50543556c60cc5a13270cb7ba4 --- src/backend/replication/logical/conflict.c | 222 ++++++++++++++------- 1 file changed, 147 insertions(+), 75 deletions(-) diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c index 48ef84ee924..b01fbe4d6ac 100644 --- a/src/backend/replication/logical/conflict.c +++ b/src/backend/replication/logical/conflict.c @@ -192,8 +192,7 @@ errcode_apply_conflict(ConflictType type) * local row, remote row, and replica identity columns. */ static void -append_tuple_value_detail(StringInfo buf, List *tuple_values, - bool need_newline) +append_tuple_value_detail(StringInfo buf, List *tuple_values) { bool first = true; @@ -209,34 +208,12 @@ append_tuple_value_detail(StringInfo buf, List *tuple_values, if (!tuple_value) continue; - if (first) - { - /* - * translator: The colon is used as a separator in conflict - * messages. The first part, built in the caller, describes what - * happened locally; the second part lists the conflicting keys - * and tuple data. - */ - appendStringInfoString(buf, _(": ")); - } - else - { - /* - * translator: This is a separator in a list of conflicting keys - * and tuple data. - */ - appendStringInfoString(buf, _(", ")); - } + if (!first) + appendStringInfoString(buf, ", "); appendStringInfoString(buf, tuple_value); first = false; } - - /* translator: This is the terminator of a conflict message */ - appendStringInfoString(buf, _(".")); - - if (need_newline) - appendStringInfoChar(buf, '\n'); } /* @@ -258,6 +235,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, StringInfo err_msg) { StringInfoData err_detail; + StringInfoData tuple_buf; char *origin_name; char *key_desc = NULL; char *local_desc = NULL; @@ -272,6 +250,7 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, indexoid); initStringInfo(&err_detail); + initStringInfo(&tuple_buf); /* Construct a detailed message describing the type of conflict */ switch (type) @@ -284,23 +263,48 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, if (err_msg->len == 0) { - appendStringInfoString(&err_detail, _("Could not apply remote change")); + append_tuple_value_detail(&tuple_buf, + list_make2(remote_desc, search_desc)); - append_tuple_value_detail(&err_detail, - list_make2(remote_desc, search_desc), - true); + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Could not apply remote change: %s.\n"), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Could not apply remote change.\n")); + + + resetStringInfo(&tuple_buf); } + append_tuple_value_detail(&tuple_buf, + list_make2(key_desc, local_desc)); + if (localts) { if (localorigin == InvalidReplOriginId) - appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified locally in transaction %u at %s"), - get_rel_name(indexoid), - localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified locally in transaction %u at %s: %s."), + get_rel_name(indexoid), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified locally in transaction %u at %s."), + get_rel_name(indexoid), + localxmin, timestamptz_to_str(localts)); + } else if (replorigin_by_oid(localorigin, true, &origin_name)) - appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by origin \"%s\" in transaction %u at %s"), - get_rel_name(indexoid), origin_name, - localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by origin \"%s\" in transaction %u at %s: %s."), + get_rel_name(indexoid), origin_name, + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by origin \"%s\" in transaction %u at %s."), + get_rel_name(indexoid), origin_name, + localxmin, timestamptz_to_str(localts)); + } /* * The origin that modified this row has been removed. This @@ -310,44 +314,82 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, * manually dropped by the user. */ else - appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by a non-existent origin in transaction %u at %s"), - get_rel_name(indexoid), - localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by a non-existent origin in transaction %u at %s: %s."), + get_rel_name(indexoid), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified by a non-existent origin in transaction %u at %s."), + get_rel_name(indexoid), + localxmin, timestamptz_to_str(localts)); + } } else - appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u"), - get_rel_name(indexoid), localxmin); - - append_tuple_value_detail(&err_detail, - list_make2(key_desc, local_desc), false); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u: %s."), + get_rel_name(indexoid), localxmin, + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Key already exists in unique index \"%s\", modified in transaction %u."), + get_rel_name(indexoid), localxmin); + } break; case CT_UPDATE_ORIGIN_DIFFERS: + append_tuple_value_detail(&tuple_buf, + list_make3(local_desc, remote_desc, + search_desc)); + if (localorigin == InvalidReplOriginId) - appendStringInfo(&err_detail, _("Updating the row that was modified locally in transaction %u at %s"), - localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Updating the row that was modified locally in transaction %u at %s: %s."), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Updating the row that was modified locally in transaction %u at %s."), + localxmin, timestamptz_to_str(localts)); + } else if (replorigin_by_oid(localorigin, true, &origin_name)) - appendStringInfo(&err_detail, _("Updating the row that was modified by a different origin \"%s\" in transaction %u at %s"), - origin_name, localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Updating the row that was modified by a different origin \"%s\" in transaction %u at %s: %s."), + origin_name, localxmin, + timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Updating the row that was modified by a different origin \"%s\" in transaction %u at %s."), + origin_name, localxmin, + timestamptz_to_str(localts)); + } /* The origin that modified this row has been removed. */ else - appendStringInfo(&err_detail, _("Updating the row that was modified by a non-existent origin in transaction %u at %s"), - localxmin, timestamptz_to_str(localts)); - - append_tuple_value_detail(&err_detail, - list_make3(local_desc, remote_desc, - search_desc), false); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Updating the row that was modified by a non-existent origin in transaction %u at %s: %s."), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Updating the row that was modified by a non-existent origin in transaction %u at %s."), + localxmin, timestamptz_to_str(localts)); + } break; case CT_UPDATE_DELETED: - appendStringInfoString(&err_detail, _("Could not find the row to be updated")); + append_tuple_value_detail(&tuple_buf, + list_make2(remote_desc, search_desc)); - append_tuple_value_detail(&err_detail, - list_make2(remote_desc, search_desc), - true); + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Could not find the row to be updated: %s.\n"), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Could not find the row to be updated.\n")); if (localts) { @@ -369,38 +411,68 @@ errdetail_apply_conflict(EState *estate, ResultRelInfo *relinfo, break; case CT_UPDATE_MISSING: - appendStringInfoString(&err_detail, _("Could not find the row to be updated")); + append_tuple_value_detail(&tuple_buf, + list_make2(remote_desc, search_desc)); - append_tuple_value_detail(&err_detail, - list_make2(remote_desc, search_desc), - false); + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Could not find the row to be updated: %s."), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Could not find the row to be updated.")); break; case CT_DELETE_ORIGIN_DIFFERS: + append_tuple_value_detail(&tuple_buf, + list_make3(local_desc, remote_desc, + search_desc)); + if (localorigin == InvalidReplOriginId) - appendStringInfo(&err_detail, _("Deleting the row that was modified locally in transaction %u at %s"), - localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Deleting the row that was modified locally in transaction %u at %s: %s."), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Deleting the row that was modified locally in transaction %u at %s."), + localxmin, timestamptz_to_str(localts)); + } else if (replorigin_by_oid(localorigin, true, &origin_name)) - appendStringInfo(&err_detail, _("Deleting the row that was modified by a different origin \"%s\" in transaction %u at %s"), - origin_name, localxmin, timestamptz_to_str(localts)); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Deleting the row that was modified by a different origin \"%s\" in transaction %u at %s: %s."), + origin_name, localxmin, + timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Deleting the row that was modified by a different origin \"%s\" in transaction %u at %s."), + origin_name, localxmin, + timestamptz_to_str(localts)); + } /* The origin that modified this row has been removed. */ else - appendStringInfo(&err_detail, _("Deleting the row that was modified by a non-existent origin in transaction %u at %s"), - localxmin, timestamptz_to_str(localts)); - - append_tuple_value_detail(&err_detail, - list_make3(local_desc, remote_desc, - search_desc), false); + { + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Deleting the row that was modified by a non-existent origin in transaction %u at %s: %s."), + localxmin, timestamptz_to_str(localts), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Deleting the row that was modified by a non-existent origin in transaction %u at %s."), + localxmin, timestamptz_to_str(localts)); + } break; case CT_DELETE_MISSING: - appendStringInfoString(&err_detail, _("Could not find the row to be deleted")); + append_tuple_value_detail(&tuple_buf, + list_make1(search_desc)); - append_tuple_value_detail(&err_detail, - list_make1(search_desc), false); + if (tuple_buf.len) + appendStringInfo(&err_detail, _("Could not find the row to be deleted: %s."), + tuple_buf.data); + else + appendStringInfo(&err_detail, _("Could not find the row to be deleted.")); break; } -- 2.43.0