Re: Simplify code building the LR conflict messages

Peter Smith <smithpb2250(at)gmail(dot)com> writes:
> On Fri, Nov 28, 2025 at 1:49 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ... and, probably, less ability of the compiler to verify that the
>> variadic arguments match the format string. I think you've taken
>> this a bit too far.

> * Or is it the use of the ternary operator to select the format
> string? If that's the issue, please note that my patch only introduced
> the ternary operator for the first two code fragments. The third
> fragment already uses ternaries in the same way on master, so I
> understood that to be an established pattern as well.

Sadly, you were copying bad code that we need to fix.

> I'd like to make sure I understand your concern correctly so I can
> revise the patch appropriately.

My concern is that we follow a coding style that the compiler can
check. Most C compilers can only verify that printf-like format
strings match the actual arguments if the format string is a constant.
So if I fat-finger the format string in this example:

--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -383,7 +383,7 @@ build_tuple_value_details(EState *estate, ResultRelInfo *relinfo,
if (tuple_value.len > 0)
{
appendStringInfoString(&tuple_value, "; ");
- appendStringInfo(&tuple_value, _("existing local row %s"),
+ appendStringInfo(&tuple_value, _("existing local row %d"),
desc);
}

I'll hear about it:

conflict.c: In function 'build_tuple_value_details':
conflict.c:386:38: warning: format '%d' expects argument of type 'int', but argument 3 has type 'char *' [-Wformat=]
appendStringInfo(&tuple_value, _("existing local row %d"),
^~~~~~~~~~~~~~~~~~~~~~~
conflict.c:386:36: note: in expansion of macro '_'
appendStringInfo(&tuple_value, _("existing local row %d"),
^

But I don't trust the compiler to see through a ternary expression
and check (both!) format strings against the actuals.

In a quick test, the gcc version I have handy does seem to be able to
do that, but I don't think we should rely on that. Format strings
are something that is way too easy to get wrong, and most of us just
expect the compiler to cross-check them for us, so coding patterns
that might silently disable such compiler warnings are best avoided.

(There's also some magic going on here to allow the compiler to see
through gettext(), but it's quite magic. We don't need to assume
multiple layers of magic will work reliably.)

regards, tom lane