Add %z support to elog/ereport?

Andres Freund <andres@2ndquadrant.com> writes:

I'd like to add support for the length modifier %z. Linux' manpages
describes it as:
z A following integer conversion corresponds to a size_t or ssize_t argument.

Since gcc's printf format checks understand it, we can add support for
it similar to the way we added %m support.

I think you'll find that %m is a totally different animal, because it
doesn't involve consuming an argument position. I'm less than sure that
every version of gcc will recognize %z, either ... and what about the
translation infrastructure?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-11-11 11:18:22 -0500, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I'd like to add support for the length modifier %z. Linux' manpages
describes it as:
z A following integer conversion corresponds to a size_t or ssize_t argument.

Since gcc's printf format checks understand it, we can add support for
it similar to the way we added %m support.

I think you'll find that %m is a totally different animal, because it
doesn't involve consuming an argument position.

I was thinking of just replacing '%z' by '%l', '%ll' or '%' as needed
and not expand it inplace. That should deal with keeping the argument
position and such.
But that won't easily work if somebody specifies flags like padding :/

I'm less than sure that every version of gcc will recognize %z, either
...

It's been in recognized in 2.95 afaics, so I think we're good.

and what about the translation infrastructure?

That I have no clue about yet.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-11-11 17:33:53 +0100, Andres Freund wrote:

On 2013-11-11 11:18:22 -0500, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I'd like to add support for the length modifier %z. Linux' manpages
describes it as:
z A following integer conversion corresponds to a size_t or ssize_t argument.

and what about the translation infrastructure?

That I have no clue about yet.

gettext has support for it afaics, it's part of POSIX:
http://www.gnu.org/software/gettext/manual/gettext.html#c_002dformat
http://www.opengroup.org/onlinepubs/007904975/functions/fprintf.html

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-11-11 11:18:22 -0500, Tom Lane wrote:

I think you'll find that %m is a totally different animal, because it
doesn't involve consuming an argument position.

I was thinking of just replacing '%z' by '%l', '%ll' or '%' as needed
and not expand it inplace. That should deal with keeping the argument
position and such.
But that won't easily work if somebody specifies flags like padding :/

[ reads manual ] Oh, I see that actually z *is* a flag, so you'd be
talking about replacing it with a different flag, ie %zu -> %llu or
similar. Yes, in principle that could work, but you'd have to be
prepared to cope with other flags, field width/precision, n$, etc,
appearing first. Sounds a bit messy, and this code is probably a
hot spot, remembering what we found out about the cost of fooling
with log_line_prefix.

I'm less than sure that every version of gcc will recognize %z, either
...

It's been in recognized in 2.95 afaics, so I think we're good.

[ fires up old HPUX box ] Nope:

$ cat test.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char**argv)
{
char buf[256];
size_t x = 0;

sprintf(buf, "%zu", (int)x);

return 0;
}
$ gcc -O2 -Wall test.c
test.c: In function `main':
test.c:9: warning: unknown conversion type character `z' in format
test.c:9: warning: too many arguments for format
$ gcc -v
Reading specs from /usr/local/lib/gcc-lib/hppa2.0-hp-hpux10.20/2.95.3/specs
gcc version 2.95.3 20010315 (release)

We might be willing to toss 2.95 overboard by now, but we'd need to be
sure of exactly what the new minimum usable version is.

and what about the translation infrastructure?

That I have no clue about yet.

Me either. I do recall Peter saying that the infrastructure knows how to
verify conversion specs in translated strings, so it would have to be
aware of 'z' flags for this to work.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

gettext has support for it afaics, it's part of POSIX:

Really? [ pokes around at pubs.opengroup.org ] Hm, I don't see it
in Single Unix Spec v2 (1997), but it is there in POSIX issue 7 (2008).
Also, the POSIX page says it defers to the C standard, and I see it
in C99. Too bad not all our platforms are C99 yet :-(

Actually this raises an interesting testing question: how sure are we
that there aren't already some format strings in the code that depend
on C99 additions to the printf spec? If they're not in code paths
exercised by the regression tests, I'm not sure we'd find out from
the buildfarm.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-11-11 12:01:40 -0500, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I'm less than sure that every version of gcc will recognize %z, either
...

It's been in recognized in 2.95 afaics, so I think we're good.

Hm. Strange. Has to have been backpatched to the ancient debian I have
around. Unfortunately I can't easily "apt-get source" there...

The commit that added it to upstream is:
commit 44e9fa656d60bb19ab81d76698a61e47a4b0857c
Author: drepper <drepper@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Mon Jan 3 21:48:49 2000 +0000

(format_char_info): Update comment. (check_format_info): Recognize 'z'
modifier in the same way 'Z' was recognized. Emit warning for formats
new in ISO C99 only if flag_isoc9x is not set.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@31188 138bc75d-0d04-0410-961f-82ee72b054a4

That's 3.0. Verified it in the 3.0. tarball, although I didn't compile
test it.

We might be willing to toss 2.95 overboard by now, but we'd need to be
sure of exactly what the new minimum usable version is.

Well, we don't even need to toss it overboard, just live with useless
warnings there since we'd translate it ourselves.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 11, 2013 at 10:50 AM, Andres Freund <andres@2ndquadrant.com> wrote:

Hi,

I'd like to add support for the length modifier %z. Linux' manpages
describes it as:
z A following integer conversion corresponds to a size_t or ssize_t argument.

Since gcc's printf format checks understand it, we can add support for
it similar to the way we added %m support.

I seem to recall that our %m support involves rewriting the error
string twice, which I think is actually kind of expensive if, for
example, you've got a loop around a PL/pgsql EXCEPTION block. I'd
actually like to find a way to get rid of the existing %m support,
maybe by having a flag that says "oh, and by the way append the system
error to my format string"; or by changing %m to %s and having the
caller pass system_error_string() or similar for that format position.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-11-11 12:18:46 -0500, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

gettext has support for it afaics, it's part of POSIX:

Really? [ pokes around at pubs.opengroup.org ] Hm, I don't see it
in Single Unix Spec v2 (1997), but it is there in POSIX issue 7 (2008).
Also, the POSIX page says it defers to the C standard, and I see it
in C99. Too bad not all our platforms are C99 yet :-(

Seems to be 2001:
http://pubs.opengroup.org/onlinepubs/007904975/functions/fprintf.html

Even though the date says it's from 2004, it's IEEE Std 1003.1 + minor
errata.

Actually this raises an interesting testing question: how sure are we
that there aren't already some format strings in the code that depend
on C99 additions to the printf spec? If they're not in code paths
exercised by the regression tests, I'm not sure we'd find out from
the buildfarm.

I agree, it's problematic. Especially as such code is likely to be in
error paths. I seem to recall you fixing some occurances you found
manually some time back so we clearly don't have an automated process
for it yet.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-11-11 12:31:55 -0500, Robert Haas wrote:

On Mon, Nov 11, 2013 at 10:50 AM, Andres Freund <andres@2ndquadrant.com> wrote:

Hi,

I'd like to add support for the length modifier %z. Linux' manpages
describes it as:
z A following integer conversion corresponds to a size_t or ssize_t argument.

Since gcc's printf format checks understand it, we can add support for
it similar to the way we added %m support.

I seem to recall that our %m support involves rewriting the error
string twice, which I think is actually kind of expensive if, for
example, you've got a loop around a PL/pgsql EXCEPTION block.

Yes, it does that. Is that actually where a significant amount of time
is spent? I have a somewhat hard time believing that.

I'd
actually like to find a way to get rid of the existing %m support,
maybe by having a flag that says "oh, and by the way append the system
error to my format string"; or by changing %m to %s and having the
caller pass system_error_string() or similar for that format position.

I'd rather get our own printf implementation from somewhere before doing
that which would quite likely result in a bigger speedup besides the
significant portability improvments.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2013-11-11 12:31:55 -0500, Robert Haas wrote:

I seem to recall that our %m support involves rewriting the error
string twice, which I think is actually kind of expensive if, for
example, you've got a loop around a PL/pgsql EXCEPTION block.

Yes, it does that. Is that actually where a significant amount of time
is spent? I have a somewhat hard time believing that.

I don't see any double copying. There is *one* copy made by
expand_fmt_string. Like Andres, I'd want to see proof that
expand_fmt_string is a significant time sink before we jump through
these kinds of hoops to get rid of it. It looks like a pretty cheap
loop to me. (It might be less cheap if we made it smart enough to
identify 'z' flags, though :-()

I'd
actually like to find a way to get rid of the existing %m support,
maybe by having a flag that says "oh, and by the way append the system
error to my format string"; or by changing %m to %s and having the
caller pass system_error_string() or similar for that format position.

The first of these doesn't work unless you require translations to
assemble the string the same way the English version does. The second
would work, I guess, but it'd sure be a pain to convert everything.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/11/13, 12:01 PM, Tom Lane wrote:

I do recall Peter saying that the infrastructure knows how to
verify conversion specs in translated strings, so it would have to be
aware of 'z' flags for this to work.

It just checks that the same conversion placeholders appear in the
translation. It doesn't interpret the actual placeholders. I don't
think this will be a problem.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-12-06 09:54:59 -0500, Peter Eisentraut wrote:

On 11/11/13, 12:01 PM, Tom Lane wrote:

I do recall Peter saying that the infrastructure knows how to
verify conversion specs in translated strings, so it would have to be
aware of 'z' flags for this to work.

It just checks that the same conversion placeholders appear in the
translation. It doesn't interpret the actual placeholders. I don't
think this will be a problem.

Ok, so here's a patch.

Patch 01 introduces infrastructure, and elog.c implementation.
Patch 02 converts some elog/ereport() callers to using the z modifier,
some were wrong at least on 64 bit windows.

In patch 01, I've modified configure to not define [U]INT64_FORMAT
directly, but rather just define INT64_LENGTH_MODIFIER as
appropriate. The formats are now defined in c.h.
INT64_LENGTH_MODIFIER is defined without quotes - I am not sure whether
that was the right choice, it requires using CppAsString2() in some
places. Maybe I should just have defined it with quotes instead. Happy
to rejigger it if somebody thinks the other way would be better.

Note that I have decided to only support the z modifier properly if no
further modifiers (precision, width) are present. That seems sufficient
for now, given the usecases, and more complete support seems to be
potentially expensive without much use.

I wonder if we should also introduce SIZE_T_FORMAT and SSIZE_T_FORMAT,
there's some places that have #ifdef _WIN64 guards and similar that look
like they could be simplified.

Btw, walsender.c/walreceiver.c upcast an int into an unsigned long for
printing? That's a bit strange.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, 2013-12-08 at 01:32 +0100, Andres Freund wrote:

Patch 02 converts some elog/ereport() callers to using the z modifier,
some were wrong at least on 64 bit windows.

This is making the assumption that Size is the same as size_t. If we
are going to commit to that, we might as well get rid of Size.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-12-08 11:43:46 -0500, Peter Eisentraut wrote:

On Sun, 2013-12-08 at 01:32 +0100, Andres Freund wrote:

Patch 02 converts some elog/ereport() callers to using the z modifier,
some were wrong at least on 64 bit windows.

This is making the assumption that Size is the same as size_t. If we
are going to commit to that, we might as well get rid of Size.

Well, we're unconditionally defining it as such in c.h and its usage is
pretty much what size_t is for. Given how widespread Size's use is, I am
not seing a realistic way of getting rid of it though.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2013-12-08 01:32:45 +0100, Andres Freund wrote:

On 2013-12-06 09:54:59 -0500, Peter Eisentraut wrote:

On 11/11/13, 12:01 PM, Tom Lane wrote:

I do recall Peter saying that the infrastructure knows how to
verify conversion specs in translated strings, so it would have to be
aware of 'z' flags for this to work.

It just checks that the same conversion placeholders appear in the
translation. It doesn't interpret the actual placeholders. I don't
think this will be a problem.

Ok, so here's a patch.

Patch 01 introduces infrastructure, and elog.c implementation.
Patch 02 converts some elog/ereport() callers to using the z modifier,
some were wrong at least on 64 bit windows.

There's just been another instance of printing size_t values being
annoying, reminding me of this patch. I'll add it to the current
commitfest given I've posted it over a month ago unless somebody
protests?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

[ 0001-Add-support-for-printing-Size-arguments-to-elog-erep.patch ]

I think this approach is fundamentally broken, because it can't reasonably
cope with any case more complicated than "%zu" or "%zd". While it's
arguable that we can get along without the ability to specify field width,
since the [U]INT64_FORMAT macros never allowed that, it is surely going
to be the case that translators will need to insert "n$" flags in the
translated versions of messages.

Another objection is that this only fixes the problem in elog/ereport
format strings, not anyplace else that it might be useful to print a
size_t value. You suggest below that we could invent some additional
macros to support that; but since the "z" flag is in C99, there really
ought to be only a small minority of platforms where it doesn't work.
So I don't think we should be contorting our code with some nonstandard
notation for the case, if there's any way to avoid that.

I think a better solution approach is to teach our src/port/snprintf.c
about the z flag, then extend configure's checking to force use of our
snprintf if the platform's version doesn't handle z. While it might be
objected that this creates a need for a run-time check in configure,
we already have one to check if snprintf handles "n$", so this approach
doesn't really make life any worse for cross-compiles.

In patch 01, I've modified configure to not define [U]INT64_FORMAT
directly, but rather just define INT64_LENGTH_MODIFIER as
appropriate. The formats are now defined in c.h.
INT64_LENGTH_MODIFIER is defined without quotes - I am not sure whether
that was the right choice, it requires using CppAsString2() in some
places. Maybe I should just have defined it with quotes instead. Happy
to rejigger it if somebody thinks the other way would be better.

Meh. This isn't needed if we do what I suggest above, but in any case
I don't approve of removing the existing [U]INT64_FORMAT macros.
That breaks code that doesn't need to get broken, probably including
third-party modules. It'd be more sensible to just add an additional
macro for the flag character(s). (And yeah, I'd put quotes in it.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

Meh. This isn't needed if we do what I suggest above, but in any case
I don't approve of removing the existing [U]INT64_FORMAT macros.
That breaks code that doesn't need to get broken, probably including
third-party modules.

After looking more closely I see you didn't actually *remove* those
macros, just define them in a different place/way. So the above objection
is just noise, sorry. (Though I think it'd be notationally cleaner to let
configure continue to define the macros; it doesn't need to do anything as
ugly as CppAsString2() to concatenate...)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2014-01-17 13:50:08 -0500, Tom Lane wrote:

I think a better solution approach is to teach our src/port/snprintf.c
about the z flag, then extend configure's checking to force use of our
snprintf if the platform's version doesn't handle z. While it might be
objected that this creates a need for a run-time check in configure,
we already have one to check if snprintf handles "n$", so this approach
doesn't really make life any worse for cross-compiles.

Hm. I had thought about that, but dismissed it because I thought people
would argue about it being too invasive...
If we're going there, we should just eliminate expand_fmt_string() from
elog.c and test for it in configure too, right?

You suggest below that we could invent some additional
macros to support that; but since the "z" flag is in C99, there really
ought to be only a small minority of platforms where it doesn't work.

Well, maybe just a minority numberwise, but one of them being windows
surely makes it count in number of installations...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-01-17 14:18:55 -0500, Tom Lane wrote:

I wrote:

Meh. This isn't needed if we do what I suggest above, but in any case
I don't approve of removing the existing [U]INT64_FORMAT macros.
That breaks code that doesn't need to get broken, probably including
third-party modules.

After looking more closely I see you didn't actually *remove* those
macros, just define them in a different place/way. So the above objection
is just noise, sorry. (Though I think it'd be notationally cleaner to let
configure continue to define the macros; it doesn't need to do anything as
ugly as CppAsString2() to concatenate...)

I prefer having configure just define the lenght modifier since that
allows to define further macros containing formats. But I think defining
them as strings instead row literals as I had might make it a bit less ugly...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers