From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Cc: | Tristan Partin <tristan(at)neon(dot)tech>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: On non-Windows, hard depend on uselocale(3) |
Date: | 2023-11-17 18:18:28 |
Message-ID: | 664255.1700245108@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> On Thu, Nov 16, 2023 at 12:06 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
>>> Perhaps we could use snprintf_l() and strtod_l() where available.
>>> They're not standard, but they are obvious extensions that NetBSD and
>>> Windows have, and those are the two systems for which we are doing
>>> grotty things in that code.
>> Yeah. I'd say the _l functions should be preferred over uselocale()
>> if available, but sadly they're not there on common systems. (It
>> looks like glibc has strtod_l but not snprintf_l, which is odd.)
> Here is a first attempt.
I've not reviewed this closely, but I did try it on mamba's host.
It compiles and passes regression testing, but I see two warnings:
common.c: In function 'PGTYPESsprintf':
common.c:120:2: warning: function 'PGTYPESsprintf' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
120 | return vsprintf_l(str, PGTYPESclocale, format, args);
| ^~~~~~
common.c: In function 'PGTYPESsnprintf':
common.c:136:2: warning: function 'PGTYPESsnprintf' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
136 | return vsnprintf_l(str, size, PGTYPESclocale, format, args);
| ^~~~~~
That happens because on NetBSD, we define PG_PRINTF_ATTRIBUTE as
"__syslog__" so that the compiler will not warn about use of %m
(apparently, they support %m in syslog() but not printf(), sigh).
I think this is telling us about an actual problem: these new
functions are based on libc's printf not what we have in snprintf.c,
and therefore we really shouldn't be assuming that they will support
any format specs beyond what POSIX requires for printf. If somebody
tried to use %m in one of these calls, we'd like to get warnings about
that.
I experimented with the attached delta patch and it does silence
these warnings. I suspect that ecpg_log() should be marked as
pg_attribute_std_printf() too, because it has the same issue,
but I didn't try that. (Probably, we see no warning for that
because the compiler isn't quite bright enough to connect the
format argument with the string that gets passed to vfprintf().)
regards, tom lane
Attachment | Content-Type | Size |
---|---|---|
0002-use-correct-printf-attribute.patch | text/x-diff | 1.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Euler Taveira | 2023-11-17 18:39:14 | Re: Lifetime of commit timestamps |
Previous Message | Tomas Vondra | 2023-11-17 14:36:25 | long-standing data loss bug in initial sync of logical replication |