So, circling back to the very beginning of this thread where I worried
about all the compile warnings we get on NetBSD-current, I'm pleased
to report that HEAD compiles warning-free so long as you define
PG_PRINTF_ATTRIBUTE to "__syslog__" rather than "gnu_printf".
So attached is a proposed patch
Andres Freund writes:
> Oh, come on. One can be disabled with a GUC, has (although not good
> enough) intelligence when it switches on, the other has ... none of
> that. Obviously performance is always a balancing act, but you'd be
> pretty pissed at anybody else regressing performance in a non-f
On 2018-09-26 18:31:07 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2018-09-26 17:41:36 -0400, Tom Lane wrote:
> >> Well, if you're unhappy about snprintf.c's performance, you could review
> >> https://commitfest.postgresql.org/19/1763/
> >> so I can push that. In my tests, that got us d
Andres Freund writes:
> On 2018-09-26 17:41:36 -0400, Tom Lane wrote:
>> Well, if you're unhappy about snprintf.c's performance, you could review
>> https://commitfest.postgresql.org/19/1763/
>> so I can push that. In my tests, that got us down to circa 10% penalty
>> for float conversions.
> Uh
Hi,
On 2018-09-26 17:41:36 -0400, Tom Lane wrote:
> Andres Freund writes:
> > I'm not saying we shouldn't default to our printf - in fact I think we
> > probably past due to use a faster float->string conversion than we
> > portably get from the OS - but I don't think we can default to our
> > sp
Andres Freund writes:
> I'm not saying we shouldn't default to our printf - in fact I think we
> probably past due to use a faster float->string conversion than we
> portably get from the OS - but I don't think we can default to our
> sprintf without doing something about the float conversion perf
Andres Freund writes:
> The strerror push, I assume it's that at least, broke something on icc:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fulmar&dt=2018-09-26%2018%3A00%3A16
Yeah. It looks like the problem is that configure's test for strerror_r's
return type does not work on icc
On 2018-09-26 12:09:34 -0700, Andres Freund wrote:
> and then fails with:
> xlc_r -qmaxmem=33554432 -D_LARGE_FILES=1 -DRANDOMIZE_ALLOCATED_MEMORY
> -qnoansialias -g -O2 -qmaxmem=16384 -qsrcmsg -D_REENTRANT -D_THREAD_SAFE
> -D_POSIX_PTHREAD_SEMANTICS -o libpq.so.5 libpq.a -Wl,-bE:libpq.exp
> -L.
On 2018-09-26 11:57:34 -0700, Andres Freund wrote:
> On 2018-09-26 11:09:59 -0400, Tom Lane wrote:
> > Michael Paquier writes:
> > > On Tue, Sep 25, 2018 at 12:05:42PM -0400, Tom Lane wrote:
> > >> Alvaro Herrera writes:
> > >>> Actually I think it *is* useful to do it like this, because then the
On 2018-09-26 11:09:59 -0400, Tom Lane wrote:
> Michael Paquier writes:
> > On Tue, Sep 25, 2018 at 12:05:42PM -0400, Tom Lane wrote:
> >> Alvaro Herrera writes:
> >>> Actually I think it *is* useful to do it like this, because then the
> >>> user knows to fix the netmsg.dll problem so that they
Hi,
On 2018-09-24 13:18:47 -0400, Tom Lane wrote:
> 0002 changes things so that we always use our snprintf, removing all the
> configure logic associated with that.
In the commit message you wrote:
> Preliminary performance testing suggests that as it stands, snprintf.c is
> faster than the nati
I wrote:
> While looking over the thread, I remembered I wanted to convert
> strerror_r into a wrapper as well. Think I'll go do that next,
> because really it'd be better for snprintf.c to be calling strerror_r
> not strerror.
So while chasing that, I realized that libpq contains its own version
Michael Paquier writes:
> On Tue, Sep 25, 2018 at 12:05:42PM -0400, Tom Lane wrote:
>> Alvaro Herrera writes:
>>> Actually I think it *is* useful to do it like this, because then the
>>> user knows to fix the netmsg.dll problem so that they can continue to
>>> investigate the winsock problem. If
On Tue, Sep 25, 2018 at 12:05:42PM -0400, Tom Lane wrote:
> Alvaro Herrera writes:
>> Actually I think it *is* useful to do it like this, because then the
>> user knows to fix the netmsg.dll problem so that they can continue to
>> investigate the winsock problem. If we don't report the secondary
Alvaro Herrera writes:
> On 2018-Sep-25, Tom Lane wrote:
>> We could possibly write something like
>>
>> sprintf(wserrbuf, "winsock error %d (could not load netmsg.dll to translate:
>> error code %lu)", err, GetLastError(;
>>
>> but I'm unconvinced that that's useful.
> Actually I think it
On 2018-Sep-25, Tom Lane wrote:
> Michael Paquier writes:
> > Ok. I won't fight hard on that. Why changing the error message from
> > "could not load netmsg.dll" to "unrecognized winsock error" then? The
> > original error string is much more verbose to grab the context.
>
> As the code stan
Michael Paquier writes:
> On Mon, Sep 24, 2018 at 01:18:47PM -0400, Tom Lane wrote:
>> Well, we have to change the code somehow to make it usable in frontend
>> as well as backend. And we can *not* have it do exit(1) in libpq.
>> So the solution I chose was to make it act the same as if FormatMes
On Mon, Sep 24, 2018 at 01:18:47PM -0400, Tom Lane wrote:
> Michael Paquier writes:
>> On Wed, Sep 12, 2018 at 01:40:01PM -0400, Tom Lane wrote:
>>> Rebase attached --- no substantive changes.
>
>> - if (handleDLL == NULL)
>> - ereport(FATAL,
>> - (errmsg_interna
Michael Paquier writes:
> On Wed, Sep 12, 2018 at 01:40:01PM -0400, Tom Lane wrote:
>> Rebase attached --- no substantive changes.
> - if (handleDLL == NULL)
> - ereport(FATAL,
> - (errmsg_internal("could not load netmsg.dll: error
> -code %lu", GetLa
On Wed, Sep 12, 2018 at 01:40:01PM -0400, Tom Lane wrote:
> Michael Paquier writes:
> > I would have liked to look at this patch in details, but it failed to
> > apply. Could you rebase?
>
> Ah, yeah, the dlopen patch touched a couple of the same places.
> Rebase attached --- no substantive chan
Michael Paquier writes:
> I would have liked to look at this patch in details, but it failed to
> apply. Could you rebase?
Ah, yeah, the dlopen patch touched a couple of the same places.
Rebase attached --- no substantive changes.
regards, tom lane
diff --git a/configur
On Sun, Aug 19, 2018 at 03:12:00PM -0400, Tom Lane wrote:
> * The Windows aspects of this are untested. It seems like importing
> pgwin32_socket_strerror's behavior into the frontend ought to be a
> bug fix, though: win32_port.h redefines socket error symbols whether
> FRONTEND is set or not, so a
I wrote:
>> Consider the following approach:
>> 1. Teach src/port/snprintf.c about %m. While I've not written a patch
>> for this, it looks pretty trivial.
>> 2. Teach configure to test for %m and if it's not there, use the
>> replacement snprintf. (Note: we're already forcing snprintf replacemen
On Sun, Aug 19, 2018 at 01:15:58AM -0400, Tom Lane wrote:
> Nico Williams writes:
> > On Sat, Aug 18, 2018 at 04:34:50PM -0400, Tom Lane wrote:
> >> So now I'm about ready to propose that we just *always* use
> >> snprintf.c, and forget all of the related configure probing.
>
> > You'd also get t
Nico Williams writes:
> On Sat, Aug 18, 2018 at 04:34:50PM -0400, Tom Lane wrote:
>> So now I'm about ready to propose that we just *always* use
>> snprintf.c, and forget all of the related configure probing.
> You'd also get to ensure that all uses from *die() are
> async-signal-safe.
[ raised
On Sat, Aug 18, 2018 at 04:34:50PM -0400, Tom Lane wrote:
> So now I'm about ready to propose that we just *always* use
> snprintf.c, and forget all of the related configure probing.
Yes.
> This'd have some advantages, notably that we'd get the
> useful_strerror() behavior in frontend as well as
I wrote:
> Consider the following approach:
> 1. Teach src/port/snprintf.c about %m. While I've not written a patch
> for this, it looks pretty trivial.
> 2. Teach configure to test for %m and if it's not there, use the
> replacement snprintf. (Note: we're already forcing snprintf replacement
> i
Indeed, there are hundreds of warnings around "pg_printf_attribute_m"
added with gcc 7.3.0 on by ubuntu 18.04 laptop, thanks to 3a60c8ff.
Oh? What warnings exactly? I would not expect any new warnings except
on platforms where gcc believes the local printf is non POSIX compliant,
which cert
On Sun, Aug 12, 2018 at 3:08 PM, Tom Lane wrote:
> Moreover,
> at least for the elog/ereport use-case, we'd be buying back some
> nontrivial part of that hit by getting rid of expand_fmt_string().
Yeah. I think expand_fmt_string() is pretty costly if you are doing a
lot of errors (e.g. write a f
I wrote:
> ... So we would be taking a hit on most platforms, but I've not really
> seen sprintf as a major component of very many profiles. Moreover,
> at least for the elog/ereport use-case, we'd be buying back some
> nontrivial part of that hit by getting rid of expand_fmt_string().
> Also wort
I wrote:
> So this is all pretty much of a mess. If we annotate the elog functions
> differently from printf's annotation then we risk getting these complaints
> in elog.c, but if we don't do that then we can't really describe their
> semantics correctly. We could possibly mark the replacement sn
Fabien COELHO writes:
> Indeed, there are hundreds of warnings around "pg_printf_attribute_m"
> added with gcc 7.3.0 on by ubuntu 18.04 laptop, thanks to 3a60c8ff.
Oh? What warnings exactly? I would not expect any new warnings except
on platforms where gcc believes the local printf is non POSI
[...]
At this point I'm inclined to give up and revert 3a60c8ff8. It's not
clear that we can really make the warning situation better, as opposed
to just moving the warnings from one platform to another.
Indeed, there are hundreds of warnings around "pg_printf_attribute_m"
added with gcc 7
I wrote:
> At this point I'm inclined to push both of those patches so we can
> see what the buildfarm makes of them.
So I did that, and while not all of the buildfarm has reported in,
enough of it has that I think we can draw conclusions. The only member
that's showing any new warnings, AFAICT,
I wrote:
> I think 0002 is probably pushable, really. The unresolved issue about
> 0001 is whether it will create a spate of warnings on Windows builds,
> and if so what to do about it. We might learn something from the
> cfbot about that, but I think the full buildfarm is going to be the
> only
In the hopes of getting the cfbot un-stuck (it's currently trying to
test a known-not-to-work patch), here are updated versions of the two
live patches we have in this thread.
0001 is the patch I originally proposed to adjust printf archetypes.
0002 is Thomas's patch to blow up on errno reference
Alvaro Herrera writes:
> This seems to say that we oughta assign GetLastError() to saved_errno
> during errstart, then use %m in the errmsg() instead.
No, because in some parts of the code, errno does mean something,
even in Windows builds.
I think the right fix is to leave %m alone, and instead
On 2018-May-27, Thomas Munro wrote:
> Out of curiosity I tried adding a GetLastError variable for Windows
> (to hide the function of that name and break callers) to the earlier
> experimental patch (attached). I had to give it an initial value to
> get rid of a warning about an unused variable (b
On Sun, May 27, 2018 at 12:38 PM, Tom Lane wrote:
> At least in the case of ereport, all it takes to create a hazard is
> more than one sub-function, eg this is risky:
>
> ereport(..., errmsg(..., strerror(errno)), errdetail(...));
>
> because errdetail() might run first and malloc some me
I wrote:
> ... It doesn't take much to make one nontrivial either. If
> memory serves, malloc() can trash errno on some platforms such as macOS,
> so even just a palloc creates a hazard of a hard-to-reproduce problem.
After digging around in the archives, the closest thing that we seem to
know fo
Thomas Munro writes:
> On Sun, May 27, 2018 at 4:21 AM, Tom Lane wrote:
>> (Basically what this would protect against is elog_start changing errno,
>> which it doesn't.)
> Hmm. It looks like errstart() preserves errno to protect %m not from
> itself, but from the caller's other arguments to the
On Sun, May 27, 2018 at 4:21 AM, Tom Lane wrote:
> ... So that seems like a rather high price to
> pay to deal with what, at present, is a purely hypothetical hazard.
> (Basically what this would protect against is elog_start changing errno,
> which it doesn't.)
Hmm. It looks like errstart() pr
I wrote:
> Thomas Munro writes:
>> Here's an experimental way to do that, if you don't mind depending on
>> gory details of libc implementations (ie knowledge of what it expands
>> too). Not sure how to avoid that since it's a macro on all modern
>> systems, and we don't have a way to temporarily
On Mon, May 21, 2018 at 2:41 PM, Thomas Munro
wrote:
> I've raised this on the freebsd-hackers
> list, let's see... I bet there's other software out there that just
> prints out "m" when things go wrong. It's arguably something that
> you'd want the complier to understand as a C dialect thing.
R
Thomas Munro writes:
> On Mon, May 21, 2018 at 4:36 PM, Tom Lane wrote:
>> I am wondering whether the elog/ereport macros can locally define some
>> version of "errno" that would cause a compile failure if it's referenced
>> within the macro args. But I'm too tired to work it out in detail.
> H
On Mon, May 21, 2018 at 4:36 PM, Tom Lane wrote:
> ... and, while we're thinking about this, how can we prevent the reverse
> problem of using strerror(errno) where you should have used %m? That
> is evidently not academic either, cf 81256cd.
>
> I am wondering whether the elog/ereport macros can
... and, while we're thinking about this, how can we prevent the reverse
problem of using strerror(errno) where you should have used %m? That
is evidently not academic either, cf 81256cd.
I am wondering whether the elog/ereport macros can locally define some
version of "errno" that would cause a
Thomas Munro writes:
> I tried this on macOS and FreeBSD using GCC and Clang: both accept
> printf("%m") without warning and then just print out "m". It'll be
> interesting to see if the NetBSD patch/idea travels further or some
> other solution can be found. I've raised this on the freebsd-hack
On Mon, May 21, 2018 at 12:30 PM, Tom Lane wrote:
> For amusement's sake, I was playing around with NetBSD-current (9-to-be)
> today, and tried to compile Postgres on it. It works OK --- and I can
> even confirm that our new code for using ARM v8 CRC instructions works
Excellent news.
> there -
For amusement's sake, I was playing around with NetBSD-current (9-to-be)
today, and tried to compile Postgres on it. It works OK --- and I can
even confirm that our new code for using ARM v8 CRC instructions works
there --- but I got a boatload of compile warnings like this:
latch.c:1180:4: war
50 matches
Mail list logo
