On Fri, Nov 28, 2025 at 3:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Smith <smithpb2250@gmail.com> writes:
> > On Fri, Nov 28, 2025 at 1:49 PM Tom Lane <tgl@sss.pgh.pa.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.)
>
Got it. Thanks for the detailed reason.
Here is a revised patch v2 that removes all ternaries from the format
string decision making (including the ones that were already in
master).
======
Kind Regards,
Peter Smith.
Fujitsu Australia
