Re: Re: proposal: a width specification for s specifier (format function), fix behave when positional and ordered placeholders are used

Hello

I found so we doesn't have functionality for simply text aligning - so
I propose support width for %s like printf's behave. glibc
implementation knows a rule for precision, that I don't would to
implement, because it is oriented to bytes and not to chars - and it
can be confusing. Still I would to have implementation and design of
"format" function maximally simple - and a rule for "s" specifier and
width is clean and simple.

postgres=# select format('||%4s|| ||%-4s||', 'ab', 'ab');
format
-------------------
|| ab|| ||ab ||

I also found so our implementation of positional and ordered
placeholders are not correct.

-- correct
postgres=# select format('%s %2$s %s', 'Hello', 'World');
format
-------------------
Hello World World

-- our current behave
postgres=# select format('%s %2$s %s', 'Hello', 'World');
ERROR: too few arguments for format
postgres=#

Comments, notices?

Regards

Pavel Stehule

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> I found so we doesn't have functionality for simply text aligning - so
> I propose support width for %s like printf's behave. glibc
> implementation knows a rule for precision, that I don't would to
> implement, because it is oriented to bytes and not to chars - and it
> can be confusing. Still I would to have implementation and design of
> "format" function maximally simple - and a rule for "s" specifier and
> width is clean and simple.

I started looking at this patch to get a head-start on the next
commitfest. There's no documentation, which certainly needs to be
fixed, but worse, this doesn't appear to match glibc printf and it's not
entirely clear to me why it doesn't.

> -- our current behave
> postgres=# select format('%s %2$s %s', 'Hello', 'World');
> ERROR: too few arguments for format
> postgres=#

This is correct, if we're matching glibc (and SUS, I believe), isn't it?
You're not allowed to mix '%2$s' type parameters and '%s' in a single
format.

Thanks,

Stephen

Hello

2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> I found so we doesn't have functionality for simply text aligning - so
>> I propose support width for %s like printf's behave. glibc
>> implementation knows a rule for precision, that I don't would to
>> implement, because it is oriented to bytes and not to chars - and it
>> can be confusing. Still I would to have implementation and design of
>> "format" function maximally simple - and a rule for "s" specifier and
>> width is clean and simple.
>
> I started looking at this patch to get a head-start on the next
> commitfest. There's no documentation, which certainly needs to be
> fixed, but worse, this doesn't appear to match glibc printf and it's not
> entirely clear to me why it doesn't.
>
>> -- our current behave
>> postgres=# select format('%s %2$s %s', 'Hello', 'World');
>> ERROR: too few arguments for format
>> postgres=#
>
> This is correct, if we're matching glibc (and SUS, I believe), isn't it?
> You're not allowed to mix '%2$s' type parameters and '%s' in a single
> format.

I am not sure, please recheck

pavel ~ $ cat test.c
#include <stdio.h>

void main()
{

printf("%s %2$s %s\n", "AHOJ", "Svete");
}

pavel ~ $ gcc test.c # no warning here
pavel ~ $ gcc --version
gcc (GCC) 4.7.2 20120921 (Red Hat 4.7.2-2)
Copyright (C) 2012 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

pavel ~ $ ./a.out
AHOJ Svete Svete
pavel ~ $

Regards

Pavel Stehule

>
> Thanks,
>
> Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> 2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
> > This is correct, if we're matching glibc (and SUS, I believe), isn't it?
> > You're not allowed to mix '%2$s' type parameters and '%s' in a single
> > format.
>
> I am not sure, please recheck

According to the man pages on my Ubuntu system, under 'Format of the
format string':

-------------------
If the style using '$' is used, it must be used throughout for
all conversions taking an argument and all width and precision
arguments, but it may be mixed with "%%" formats which do not consume
an argument.
-------------------

> pavel ~ $ cat test.c
> #include <stdio.h>
>
> void main()
> {
>
> printf("%s %2$s %s\n", "AHOJ", "Svete");
> }
>
> pavel ~ $ gcc test.c # no warning here

You didn't turn any on...

sfrost(at)tamriel:/home/sfrost> gcc -o qq -Wall test.c
test.c: In function ‘main’:
test.c:5:3: warning: $ operand number used after format without operand number [-Wformat]

Thanks,

Stephen

2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> 2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
>> > This is correct, if we're matching glibc (and SUS, I believe), isn't it?
>> > You're not allowed to mix '%2$s' type parameters and '%s' in a single
>> > format.
>>
>> I am not sure, please recheck
>
> According to the man pages on my Ubuntu system, under 'Format of the
> format string':
>
> -------------------
> If the style using '$' is used, it must be used throughout for
> all conversions taking an argument and all width and precision
> arguments, but it may be mixed with "%%" formats which do not consume
> an argument.
> -------------------
>
>> pavel ~ $ cat test.c
>> #include <stdio.h>
>>
>> void main()
>> {
>>
>> printf("%s %2$s %s\n", "AHOJ", "Svete");
>> }
>>
>> pavel ~ $ gcc test.c # no warning here
>
> You didn't turn any on...
>
> sfrost(at)tamriel:/home/sfrost> gcc -o qq -Wall test.c
> test.c: In function ‘main’:
> test.c:5:3: warning: $ operand number used after format without operand number [-Wformat]

ok, so what is proposed solution?

I see two possibilities - a) applying my current patch - although it
is not fully correct, b) new patch, that do necessary check and raise
more descriptive error message.

I have not strong preferences in this topic - both variants are
acceptable for me and I invite any community opinion. But current
state is not intuitive and should be fixed.

Regards

Pavel

>
> Thanks,
>
> Stephen

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> ok, so what is proposed solution?

My recommendation would be to match what glibc's printf does.

> I see two possibilities - a) applying my current patch - although it
> is not fully correct, b) new patch, that do necessary check and raise
> more descriptive error message.

Right, have a new patch that does error-checking and returns a better
error on that case, update the docs to reflect that restriction, and
then (ideally as an additional and independent patch..) implement the
width capability (and, ideally, the ability to pass the width as an
argument, as glibc supports) which matches the glibc arguments.

Part of the reason that this restriction is in place, I believe, is
because glibc expects the width to come before any explicit argument
being passed and if an explicit argument is used for width then an
explicit argument has to be used for the value also, otherwise it
wouldn't be clear from the format which was the argument number and
which was the explicit width size.

I don't think it's a good idea to come up with our own format
definition, particularly one which looks so similar to the well-known
printf() format.

> I have not strong preferences in this topic - both variants are
> acceptable for me and I invite any community opinion. But current
> state is not intuitive and should be fixed.

Agreed.

Thanks,

Stephen

Hello Stephen

2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> ok, so what is proposed solution?
>
> My recommendation would be to match what glibc's printf does.
>
>> I see two possibilities - a) applying my current patch - although it
>> is not fully correct, b) new patch, that do necessary check and raise
>> more descriptive error message.
>
> Right, have a new patch that does error-checking and returns a better
> error on that case, update the docs to reflect that restriction, and
> then (ideally as an additional and independent patch..) implement the
> width capability (and, ideally, the ability to pass the width as an
> argument, as glibc supports) which matches the glibc arguments.
>
> Part of the reason that this restriction is in place, I believe, is
> because glibc expects the width to come before any explicit argument
> being passed and if an explicit argument is used for width then an
> explicit argument has to be used for the value also, otherwise it
> wouldn't be clear from the format which was the argument number and
> which was the explicit width size.

I found one issue - if I disallow mixing positional and ordered style
I break compatibility with previous implementation.

so maybe third way is better - use fix from my patch - a behave is
same like in glibc - and raise warning (instead errors) when mixing
styles is detected - we can replace warnings by errors in future.

What do you think?

Regards

Pavel
>
> I don't think it's a good idea to come up with our own format
> definition, particularly one which looks so similar to the well-known
> printf() format.
>
>> I have not strong preferences in this topic - both variants are
>> acceptable for me and I invite any community opinion. But current
>> state is not intuitive and should be fixed.
>
> Agreed.
>
> Thanks,
>
> Stephen

2012/12/30 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello Stephen
>
> 2012/12/29 Stephen Frost <sfrost(at)snowman(dot)net>:
>> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>>> ok, so what is proposed solution?
>>
>> My recommendation would be to match what glibc's printf does.
>>
>>> I see two possibilities - a) applying my current patch - although it
>>> is not fully correct, b) new patch, that do necessary check and raise
>>> more descriptive error message.
>>
>> Right, have a new patch that does error-checking and returns a better
>> error on that case, update the docs to reflect that restriction, and
>> then (ideally as an additional and independent patch..) implement the
>> width capability (and, ideally, the ability to pass the width as an
>> argument, as glibc supports) which matches the glibc arguments.
>>
>> Part of the reason that this restriction is in place, I believe, is
>> because glibc expects the width to come before any explicit argument
>> being passed and if an explicit argument is used for width then an
>> explicit argument has to be used for the value also, otherwise it
>> wouldn't be clear from the format which was the argument number and
>> which was the explicit width size.
>
> I found one issue - if I disallow mixing positional and ordered style
> I break compatibility with previous implementation.
>
> so maybe third way is better - use fix from my patch - a behave is
> same like in glibc - and raise warning (instead errors) when mixing
> styles is detected - we can replace warnings by errors in future.

this is exactly what gcc does - and without breaking applications.

>
> What do you think?
>
> Regards
>
> Pavel
>>
>> I don't think it's a good idea to come up with our own format
>> definition, particularly one which looks so similar to the well-known
>> printf() format.
>>
>>> I have not strong preferences in this topic - both variants are
>>> acceptable for me and I invite any community opinion. But current
>>> state is not intuitive and should be fixed.
>>
>> Agreed.
>>
>> Thanks,
>>
>> Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> I found one issue - if I disallow mixing positional and ordered style
> I break compatibility with previous implementation.

Can you elaborate? In the previous example, an error was returned when
mixing (not a terribly good one, but still an error). Returning a
better error won't be a problem.

> so maybe third way is better - use fix from my patch - a behave is
> same like in glibc - and raise warning (instead errors) when mixing
> styles is detected - we can replace warnings by errors in future.
>
> What do you think?

If there are cases which work today then I agree that we should issue a
warning to avoid breaking existing applications. We should still use
the glibc format when adding width support, however.

Thanks,

Stephen

Hello

2012/12/31 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> I found one issue - if I disallow mixing positional and ordered style
>> I break compatibility with previous implementation.
>
> Can you elaborate? In the previous example, an error was returned when
> mixing (not a terribly good one, but still an error). Returning a
> better error won't be a problem.

A result from ours previous talk was a completely disabling mixing
positional and ordered placeholders - like is requested by man and gcc
raises warnings there.

But mixing is not explicitly disallowed in doc, and mixing was tested
in our regress tests. There are tests where placeholders are mixed -
so anybody can use it.

select format('Hello %s %1$s %s', 'World', 'Hello again'); -- is
enabled and supported and result is expected

-- but this raises error - and it is same situation like previous example
select format('%s %2$s %s', 'Hello', 'World');

-- so bot examples should be executed or should be disabled if this
functionality should be consistent. And I can't to break first
example, then I have to repair second example

Regards

>
>> so maybe third way is better - use fix from my patch - a behave is
>> same like in glibc - and raise warning (instead errors) when mixing
>> styles is detected - we can replace warnings by errors in future.
>>
>> What do you think?
>
> If there are cases which work today then I agree that we should issue a
> warning to avoid breaking existing applications. We should still use
> the glibc format when adding width support, however.
>
> Thanks,
>
> Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> A result from ours previous talk was a completely disabling mixing
> positional and ordered placeholders - like is requested by man and gcc
> raises warnings there.
>
> But mixing is not explicitly disallowed in doc, and mixing was tested
> in our regress tests. There are tests where placeholders are mixed -
> so anybody can use it.
> select format('Hello %s %1$s %s', 'World', 'Hello again'); -- is
> enabled and supported and result is expected

Alright, then I agree that raising a warning in that case makes sense
and let's update the docs to reflect that it shouldn't be done (like
what glibc/gcc do).

Thanks,

Stephen

2012/12/31 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> A result from ours previous talk was a completely disabling mixing
>> positional and ordered placeholders - like is requested by man and gcc
>> raises warnings there.
>>
>> But mixing is not explicitly disallowed in doc, and mixing was tested
>> in our regress tests. There are tests where placeholders are mixed -
>> so anybody can use it.
>> select format('Hello %s %1$s %s', 'World', 'Hello again'); -- is
>> enabled and supported and result is expected
>
> Alright, then I agree that raising a warning in that case makes sense
> and let's update the docs to reflect that it shouldn't be done (like
> what glibc/gcc do).

ok, I prepare patch

Regards

Pavel

>
> Thanks,
>
> Stephen

Hello

2012/12/31 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> A result from ours previous talk was a completely disabling mixing
>> positional and ordered placeholders - like is requested by man and gcc
>> raises warnings there.
>>
>> But mixing is not explicitly disallowed in doc, and mixing was tested
>> in our regress tests. There are tests where placeholders are mixed -
>> so anybody can use it.
>> select format('Hello %s %1$s %s', 'World', 'Hello again'); -- is
>> enabled and supported and result is expected
>
> Alright, then I agree that raising a warning in that case makes sense
> and let's update the docs to reflect that it shouldn't be done (like
> what glibc/gcc do).

so there are two patches - first is fix in logic when positional and
ordered parameters are mixed + add warning in this situation. Second
patch enables possibility to specify width for %s conversion.

I didn't finalize documentation due my net good English skills -
probably there is necessary new paragraph about function "format"
elsewhere than in table

Regards

Pavel

>
> Thanks,
>
> Stephen

Hello

2012/12/31 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2012/12/31 Stephen Frost <sfrost(at)snowman(dot)net>:
>> Pavel,
>>
>> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>>> A result from ours previous talk was a completely disabling mixing
>>> positional and ordered placeholders - like is requested by man and gcc
>>> raises warnings there.
>>>
>>> But mixing is not explicitly disallowed in doc, and mixing was tested
>>> in our regress tests. There are tests where placeholders are mixed -
>>> so anybody can use it.
>>> select format('Hello %s %1$s %s', 'World', 'Hello again'); -- is
>>> enabled and supported and result is expected
>>
>> Alright, then I agree that raising a warning in that case makes sense
>> and let's update the docs to reflect that it shouldn't be done (like
>> what glibc/gcc do).
>
> so there are two patches - first is fix in logic when positional and
> ordered parameters are mixed + add warning in this situation. Second
> patch enables possibility to specify width for %s conversion.
>
> I didn't finalize documentation due my net good English skills -
> probably there is necessary new paragraph about function "format"
> elsewhere than in table
>
> Regards
>
> Pavel

updated patches due changes for better variadic "any" function.

apply fix_mixing_positinal_ordered_placeholders_warnings_20130126.patch first

Regards

Pavel

On 26 January 2013 10:58, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> updated patches due changes for better variadic "any" function.
>
> apply fix_mixing_positinal_ordered_placeholders_warnings_20130126.patch first
>

Hi,

No one is listed as a reviewer for this patch so I thought I would
take a look at it, since it looks like a useful enhancement to
format().

Starting with the first patch - it issues a new WARNING if the format
string contains a mixture of format specifiers with and without
parameter indexes (e.g., 'Hello %s, %1$s').

Having thought about it a bit, I really don't like this for a number of reasons:

* I actually quite like the current behaviour. Admittedly putting
ordered specifiers (like '%s') after positional ones (like '%3$s') is
probably not so useful, and potentially open to different
interpretations. But putting positional specifiers at the end is
completely unambiguous and can save a lot of typing (e.g.,
'%s,%s,%s,%s,%,s,%s,%s,%1$s').

* On backwards compatibility grounds. The fact that the only example
of format() in the manual is precisely a case of mixed positional and
ordered parameters makes it quite likely that people will have used
this feature already.

* Part of the justification for adding the warning is for
compatibility with glibc/SUS printf(). But if we are aiming for that,
then we should also produce a warning if positional parameters are
used and not all parameters are consumed. That would be a pain to
implement and I don't think it would be particularly helpful in
practice. Here is what the SUS says:

"""
The format can contain either numbered argument specifications (that
is, %n$ and *m$), or unnumbered argument specifications (that is, %
and *), but normally not both. The only exception to this is that %%
can be mixed with the %n$ form. The results of mixing numbered and
unnumbered argument specifications in a format string are undefined.
When numbered argument specifications are used, specifying the Nth
argument requires that all the leading arguments, from the first to
the (N-1)th, are specified in the format string.
"""

I think that if we are going to do anything, we should explicitly
document our current behaviour as a PostgreSQL extension to the SUS
printf(), describing how we handle mixed parameters, rather than
adding this warning.

The current PostgreSQL code isn't inconsistent with the SUS, except in
the error case, and so can reasonably be regarded as an extension.

Regards,
Dean

Hello

2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 26 January 2013 10:58, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> updated patches due changes for better variadic "any" function.
>>
>> apply fix_mixing_positinal_ordered_placeholders_warnings_20130126.patch first
>>
>
> Hi,
>
> No one is listed as a reviewer for this patch so I thought I would
> take a look at it, since it looks like a useful enhancement to
> format().
>
> Starting with the first patch - it issues a new WARNING if the format
> string contains a mixture of format specifiers with and without
> parameter indexes (e.g., 'Hello %s, %1$s').
>
> Having thought about it a bit, I really don't like this for a number of reasons:
>
> * I actually quite like the current behaviour. Admittedly putting
> ordered specifiers (like '%s') after positional ones (like '%3$s') is
> probably not so useful, and potentially open to different
> interpretations. But putting positional specifiers at the end is
> completely unambiguous and can save a lot of typing (e.g.,
> '%s,%s,%s,%s,%,s,%s,%s,%1$s').
>
> * On backwards compatibility grounds. The fact that the only example
> of format() in the manual is precisely a case of mixed positional and
> ordered parameters makes it quite likely that people will have used
> this feature already.
>
> * Part of the justification for adding the warning is for
> compatibility with glibc/SUS printf(). But if we are aiming for that,
> then we should also produce a warning if positional parameters are
> used and not all parameters are consumed. That would be a pain to
> implement and I don't think it would be particularly helpful in
> practice. Here is what the SUS says:
>
> """
> The format can contain either numbered argument specifications (that
> is, %n$ and *m$), or unnumbered argument specifications (that is, %
> and *), but normally not both. The only exception to this is that %%
> can be mixed with the %n$ form. The results of mixing numbered and
> unnumbered argument specifications in a format string are undefined.
> When numbered argument specifications are used, specifying the Nth
> argument requires that all the leading arguments, from the first to
> the (N-1)th, are specified in the format string.
> """
>
> I think that if we are going to do anything, we should explicitly
> document our current behaviour as a PostgreSQL extension to the SUS
> printf(), describing how we handle mixed parameters, rather than
> adding this warning.
>
> The current PostgreSQL code isn't inconsistent with the SUS, except in
> the error case, and so can reasonably be regarded as an extension.
>

I am not sure what you dislike?

warnings or redesign of behave.

I can live without warnings, when this field will be documented - it
is not fully compatible with gcc, but gcc just raises warnings and
does correct implementation. Our warnings are on different level than
gcc warnings.

But I don't think so current implementation is correct

-- our current behave
postgres=# select format('%s %2$s %s', 'Hello', 'World');
ERROR: too few arguments for format
postgres=#

postgres=# select format('%s %1$s %s', 'Hello', 'World'); -- works

ordered parameters should be independent on positional parameters. And
this behave has glibc

Regards

Pavel

> Regards,
> Dean

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> Starting with the first patch - it issues a new WARNING if the format
>> string contains a mixture of format specifiers with and without
>> parameter indexes (e.g., 'Hello %s, %1$s').
>>
>> Having thought about it a bit, I really don't like this for a number of reasons:

> I am not sure what you dislike?
> warnings or redesign of behave.

Both. If we had done this when we first implemented format(), it'd be
fine, but it's too late to change it now. There very likely are
applications out there that depend on the current behavior. As Dean
says, it's not incompatible with SUS, just a superset, so ISTM this
patch is proposing to remove documented functionality --- for no very
strong reason.

I vote for rejecting this change entirely.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Both. If we had done this when we first implemented format(), it'd be
> fine, but it's too late to change it now. There very likely are
> applications out there that depend on the current behavior. As Dean
> says, it's not incompatible with SUS, just a superset, so ISTM this
> patch is proposing to remove documented functionality --- for no very
> strong reason.

It's only a "superset" of the very poor subset of printf()-like
functionality that we currently support through the format() function.

If we can actually match glibc/SUS (which I don't believe the initial
patch did..) and support a mix of explicitly specified arguments and
implicit arguments, along with the various width, precision, and other
format specifications, then fine by me.

I'm not convinced that's actually possible due to the ambiguity which
will certainly arise and I'm quite sure the documentation that explains
what we do in each case will deserve it's own chapter.

Thanks,

Stephen

On 28 January 2013 17:32, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Both. If we had done this when we first implemented format(), it'd be
>> fine, but it's too late to change it now. There very likely are
>> applications out there that depend on the current behavior. As Dean
>> says, it's not incompatible with SUS, just a superset, so ISTM this
>> patch is proposing to remove documented functionality --- for no very
>> strong reason.
>
> It's only a "superset" of the very poor subset of printf()-like
> functionality that we currently support through the format() function.
>
> If we can actually match glibc/SUS (which I don't believe the initial
> patch did..) and support a mix of explicitly specified arguments and
> implicit arguments, along with the various width, precision, and other
> format specifications, then fine by me.
>
> I'm not convinced that's actually possible due to the ambiguity which
> will certainly arise and I'm quite sure the documentation that explains
> what we do in each case will deserve it's own chapter.
>

There are a number of separate issues here, but I don't see this as an
intractable problem. In general a format specifier looks like:

%[parameter][flags][width][.precision][length]type

parameter - an optional n$. This is where we have implemented a
superset of the SUS printf(). But I think it is a useful superset, and
it's too late to remove it now. Any ambiguity lies here, where we go
beyond the SUS - some printf() implementations appear to do something
different (apparently without documenting what they do). I think our
documentation could be clearer here, to explain how mixed parameters
are handled.

flags - not currently implemented. Pavel's second patch adds support
for the '-' flag for left justified string output. However, I think
this should support all datatypes (i.e., %I and %L as well as %s).

width - not currently implemented. Pavel's second patch adds support
for this, but note that for full compatibility with the SUS it needs
to also support widths specified using * and *n$. Also, I think it
should support all supported datatypes, not just strings.

precision - only relevant to numeric datatypes, which we don't support.

length - only relevant to numeric datatypes, which we don't support.

type - this is where we only support a small subset of the SUS (plus a
couple of SQL-specific types). I'm not sure if anyone has any plans to
extend this, but that's certainly not on the cards for 9.3.

So the relevant pieces that Pavel's second patch is attempting to add
support for are the '-' flag and the width field. As noted above,
there are a couple of areas where the current patch falls short of the
SUS:

1). The '-' flag and width field are supposed to apply to all types. I
think that not supporting %I and %L will be somewhat limiting, and
goes against the intent of the SUS, even though those types are
PostgreSQL extensions.

2). The width field is supposed to support * (width specified by the
next function argument) and *n$ (width specified by the nth function
argument). If we supported this, then we could claim full
compatibility with the SUS in all fields except for the type support,
which would seem like a real step forward.

Regards,
Dean

2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 28 January 2013 17:32, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> Both. If we had done this when we first implemented format(), it'd be
>>> fine, but it's too late to change it now. There very likely are
>>> applications out there that depend on the current behavior. As Dean
>>> says, it's not incompatible with SUS, just a superset, so ISTM this
>>> patch is proposing to remove documented functionality --- for no very
>>> strong reason.
>>
>> It's only a "superset" of the very poor subset of printf()-like
>> functionality that we currently support through the format() function.
>>
>> If we can actually match glibc/SUS (which I don't believe the initial
>> patch did..) and support a mix of explicitly specified arguments and
>> implicit arguments, along with the various width, precision, and other
>> format specifications, then fine by me.
>>
>> I'm not convinced that's actually possible due to the ambiguity which
>> will certainly arise and I'm quite sure the documentation that explains
>> what we do in each case will deserve it's own chapter.
>>
>
> There are a number of separate issues here, but I don't see this as an
> intractable problem. In general a format specifier looks like:
>
> %[parameter][flags][width][.precision][length]type
>
> parameter - an optional n$. This is where we have implemented a
> superset of the SUS printf(). But I think it is a useful superset, and
> it's too late to remove it now. Any ambiguity lies here, where we go
> beyond the SUS - some printf() implementations appear to do something
> different (apparently without documenting what they do). I think our
> documentation could be clearer here, to explain how mixed parameters
> are handled.
>
> flags - not currently implemented. Pavel's second patch adds support
> for the '-' flag for left justified string output. However, I think
> this should support all datatypes (i.e., %I and %L as well as %s).

no - surely not - I% and L% is PostgreSQL extension and left or right
alignment is has no sense for PostgreSQL identifiers and PostgreSQL
literals.

Regards

Pavel

>
> width - not currently implemented. Pavel's second patch adds support
> for this, but note that for full compatibility with the SUS it needs
> to also support widths specified using * and *n$. Also, I think it
> should support all supported datatypes, not just strings.
>
> precision - only relevant to numeric datatypes, which we don't support.
>
> length - only relevant to numeric datatypes, which we don't support.
>
> type - this is where we only support a small subset of the SUS (plus a
> couple of SQL-specific types). I'm not sure if anyone has any plans to
> extend this, but that's certainly not on the cards for 9.3.
>
>
> So the relevant pieces that Pavel's second patch is attempting to add
> support for are the '-' flag and the width field. As noted above,
> there are a couple of areas where the current patch falls short of the
> SUS:
>
> 1). The '-' flag and width field are supposed to apply to all types. I
> think that not supporting %I and %L will be somewhat limiting, and
> goes against the intent of the SUS, even though those types are
> PostgreSQL extensions.
>
> 2). The width field is supposed to support * (width specified by the
> next function argument) and *n$ (width specified by the nth function
> argument). If we supported this, then we could claim full
> compatibility with the SUS in all fields except for the type support,
> which would seem like a real step forward.
>
> Regards,
> Dean

On 28 January 2013 20:40, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> On 28 January 2013 17:32, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>>> Both. If we had done this when we first implemented format(), it'd be
>>>> fine, but it's too late to change it now. There very likely are
>>>> applications out there that depend on the current behavior. As Dean
>>>> says, it's not incompatible with SUS, just a superset, so ISTM this
>>>> patch is proposing to remove documented functionality --- for no very
>>>> strong reason.
>>>
>>> It's only a "superset" of the very poor subset of printf()-like
>>> functionality that we currently support through the format() function.
>>>
>>> If we can actually match glibc/SUS (which I don't believe the initial
>>> patch did..) and support a mix of explicitly specified arguments and
>>> implicit arguments, along with the various width, precision, and other
>>> format specifications, then fine by me.
>>>
>>> I'm not convinced that's actually possible due to the ambiguity which
>>> will certainly arise and I'm quite sure the documentation that explains
>>> what we do in each case will deserve it's own chapter.
>>>
>>
>> There are a number of separate issues here, but I don't see this as an
>> intractable problem. In general a format specifier looks like:
>>
>> %[parameter][flags][width][.precision][length]type
>>
>> parameter - an optional n$. This is where we have implemented a
>> superset of the SUS printf(). But I think it is a useful superset, and
>> it's too late to remove it now. Any ambiguity lies here, where we go
>> beyond the SUS - some printf() implementations appear to do something
>> different (apparently without documenting what they do). I think our
>> documentation could be clearer here, to explain how mixed parameters
>> are handled.
>>
>> flags - not currently implemented. Pavel's second patch adds support
>> for the '-' flag for left justified string output. However, I think
>> this should support all datatypes (i.e., %I and %L as well as %s).
>
> no - surely not - I% and L% is PostgreSQL extension and left or right
> alignment is has no sense for PostgreSQL identifiers and PostgreSQL
> literals.

Left/right alignment and padding in printf() apply to all types, after
the data value is converted to a string. Why shouldn't that same
principle apply to %I and %L? The obvious use-case is for producing
tabular output of data with columns neatly aligned. If we don't
support %I and %L then any alignment of columns to the right is lost.

Regards,
Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 28 January 2013 20:40, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>>> flags - not currently implemented. Pavel's second patch adds support
>>> for the '-' flag for left justified string output. However, I think
>>> this should support all datatypes (i.e., %I and %L as well as %s).

>> no - surely not - I% and L% is PostgreSQL extension and left or right
>> alignment is has no sense for PostgreSQL identifiers and PostgreSQL
>> literals.

> Left/right alignment and padding in printf() apply to all types, after
> the data value is converted to a string. Why shouldn't that same
> principle apply to %I and %L?

I agree with Dean --- it would be very strange for these features not to
apply to all conversion specifiers (excepting %% of course, which isn't
really a conversion specifier but an escaping hack).

regards, tom lane

On 28 January 2013 20:32, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> In general a format specifier looks like:
>
> %[parameter][flags][width][.precision][length]type
>

This highlights another problem with the current implementation ---
the '-' flag and the width field need to be parsed separately. So
'%-3s' should be parsed as a '-' flag followed by a width of 3, not as
a width of -3, which is then interpreted as left-aligned. This might
seem like nitpicking, but actually it is important:

* In the future we might support more flags, and they can be specified
in any order, so the '-' flag won't necessarily come immediately
before the width.

* The width field is optional, even if the '-' flag is specified. So
'%-s' is perfectly legal and should be interpreted as '%s'. The
current implementation treats it as a width of 0, which is wrong.

* The width field might not be a number, it might be something like *
or *3$. Note that the SUS allows a negative width to be passed in as a
function argument using this syntax, in which case it should be
treated as if the '-' flag were specified.

Regards,
Dean

On 29 January 2013 08:19, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> * The width field is optional, even if the '-' flag is specified. So
> '%-s' is perfectly legal and should be interpreted as '%s'. The
> current implementation treats it as a width of 0, which is wrong.
>

Oh, but of course a width of 0 is the same as no width at all, so the
current code is correct after all. That's what happens if I try to
write emails before I've had my caffeine :-)

I think my other points remain valid though. It would still be neater
to parse the flags separately from the width field, and then all
literal numbers that appear in the format should be positive.

Regards,
Dean

2013/1/28 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>>> Starting with the first patch - it issues a new WARNING if the format
>>> string contains a mixture of format specifiers with and without
>>> parameter indexes (e.g., 'Hello %s, %1$s').
>>>
>>> Having thought about it a bit, I really don't like this for a number of reasons:
>
>> I am not sure what you dislike?
>> warnings or redesign of behave.
>
> Both. If we had done this when we first implemented format(), it'd be
> fine, but it's too late to change it now. There very likely are
> applications out there that depend on the current behavior. As Dean
> says, it's not incompatible with SUS, just a superset, so ISTM this
> patch is proposing to remove documented functionality --- for no very
> strong reason.

I disagree - but I have not a arguments. I am thinking so current
implementation is wrong, and now is last time when we can to fix it -
format() function is not too old and there is relative chance to
minimal impact to users.

I didn't propose removing this functionality, but fixing via more
logical independent counter for ordered arguments. Dependency on
previous positional argument is unpractical and unclean. I am not
satisfied so it is undefined and then it is ok.

Regards

Pavel

>
> I vote for rejecting this change entirely.
>
> regards, tom lane

2013/1/28 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 28 January 2013 20:40, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> 2013/1/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>>>> flags - not currently implemented. Pavel's second patch adds support
>>>> for the '-' flag for left justified string output. However, I think
>>>> this should support all datatypes (i.e., %I and %L as well as %s).
>
>>> no - surely not - I% and L% is PostgreSQL extension and left or right
>>> alignment is has no sense for PostgreSQL identifiers and PostgreSQL
>>> literals.
>
>> Left/right alignment and padding in printf() apply to all types, after
>> the data value is converted to a string. Why shouldn't that same
>> principle apply to %I and %L?
>
> I agree with Dean --- it would be very strange for these features not to
> apply to all conversion specifiers (excepting %% of course, which isn't
> really a conversion specifier but an escaping hack).

ok - I have no problem with it - after some thinking - just remove one check.

Regards

Pavel

>
> regards, tom lane

2013/1/29 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 28 January 2013 20:32, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> In general a format specifier looks like:
>>
>> %[parameter][flags][width][.precision][length]type
>>
>
> This highlights another problem with the current implementation ---
> the '-' flag and the width field need to be parsed separately. So
> '%-3s' should be parsed as a '-' flag followed by a width of 3, not as
> a width of -3, which is then interpreted as left-aligned. This might
> seem like nitpicking, but actually it is important:
>
> * In the future we might support more flags, and they can be specified
> in any order, so the '-' flag won't necessarily come immediately
> before the width.
>
> * The width field is optional, even if the '-' flag is specified. So
> '%-s' is perfectly legal and should be interpreted as '%s'. The
> current implementation treats it as a width of 0, which is wrong.
>
> * The width field might not be a number, it might be something like *
> or *3$. Note that the SUS allows a negative width to be passed in as a
> function argument using this syntax, in which case it should be
> treated as if the '-' flag were specified.

A possibility to specify width as * can be implemented in future. The
format() function was not designed to be fully compatible with SUS -
it is simplified subset with pg enhancing.

There was a talks about integration to_char() formats to format() and
we didn't block it - and it was reason why I proposed and pushed name
"format" and not "printf", because there can be little bit different
purposes than generic printf function.

Regards

Pavel

>
> Regards,
> Dean

Hello

2013/1/29 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 29 January 2013 08:19, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> * The width field is optional, even if the '-' flag is specified. So
>> '%-s' is perfectly legal and should be interpreted as '%s'. The
>> current implementation treats it as a width of 0, which is wrong.
>>
>
> Oh, but of course a width of 0 is the same as no width at all, so the
> current code is correct after all. That's what happens if I try to
> write emails before I've had my caffeine :-)
>
> I think my other points remain valid though. It would still be neater
> to parse the flags separately from the width field, and then all
> literal numbers that appear in the format should be positive.

I am sending rewritten code

It indirect width "*" and "*n$" is supported. It needs little bit more code.

There are a new question

what should be result of

format(">>%2$*1$s<<", NULL, "hello")

???

raise exception now, but I am able to modify to some agreement

Regards

Pavel

>
> Regards,
> Dean

Hello

minor update - fix align NULL for %L

Regards

Pavel

2013/1/31 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> 2013/1/29 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> On 29 January 2013 08:19, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> * The width field is optional, even if the '-' flag is specified. So
>>> '%-s' is perfectly legal and should be interpreted as '%s'. The
>>> current implementation treats it as a width of 0, which is wrong.
>>>
>>
>> Oh, but of course a width of 0 is the same as no width at all, so the
>> current code is correct after all. That's what happens if I try to
>> write emails before I've had my caffeine :-)
>>
>> I think my other points remain valid though. It would still be neater
>> to parse the flags separately from the width field, and then all
>> literal numbers that appear in the format should be positive.
>
> I am sending rewritten code
>
> It indirect width "*" and "*n$" is supported. It needs little bit more code.
>
> There are a new question
>
> what should be result of
>
> format(">>%2$*1$s<<", NULL, "hello")
>
> ???
>
> raise exception now, but I am able to modify to some agreement
>
> Regards
>
> Pavel
>
>
>
>
>
>>
>> Regards,
>> Dean

> 2013/1/31 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>> I am sending rewritten code

Nice. I think this will be very useful, and it looks like it now
supports everything that printf() does for %s format specifiers, and
it's good that %I and %L behave the same. Also the code is looking
cleaner.

>> It indirect width "*" and "*n$" is supported. It needs little bit more code.
>>
>> There are a new question
>>
>> what should be result of
>>
>> format(">>%2$*1$s<<", NULL, "hello")
>>
>> ???

My first thought is that a NULL width should be treated the same as no
width at all (equivalent to width=0), rather than raising an
exception.

> minor update - fix align NULL for %L

You need to do the same for a NULL value with %s, which currently
produces an empty string regardless of the width.

The documentation also needs to be updated. I'm thinking perhaps
format() should now have its own separate sub-section in the manual,
rather than trying to cram it's docs into a single table row. I can
help with the docs if you like.

Regards,
Dean

2013/2/9 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> 2013/1/31 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>> I am sending rewritten code
>
> Nice. I think this will be very useful, and it looks like it now
> supports everything that printf() does for %s format specifiers, and
> it's good that %I and %L behave the same. Also the code is looking
> cleaner.
>
>>> It indirect width "*" and "*n$" is supported. It needs little bit more code.
>>>
>>> There are a new question
>>>
>>> what should be result of
>>>
>>> format(">>%2$*1$s<<", NULL, "hello")
>>>
>>> ???
>
> My first thought is that a NULL width should be treated the same as no
> width at all (equivalent to width=0), rather than raising an
> exception.
>
>> minor update - fix align NULL for %L
>
> You need to do the same for a NULL value with %s, which currently
> produces an empty string regardless of the width.

have others same opinion? Usually I don't like hide NULLs, but this is
corner case (and specific function) and I have not strong opinion on
this issue.

>
> The documentation also needs to be updated. I'm thinking perhaps
> format() should now have its own separate sub-section in the manual,
> rather than trying to cram it's docs into a single table row. I can
> help with the docs if you like.

please, if you can, write it. I am sure, so you do it significantly
better than me.

Thank you

Pavel

>
> Regards,
> Dean

On 9 February 2013 18:30, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> There are a new question
>>>>
>>>> what should be result of
>>>>
>>>> format(">>%2$*1$s<<", NULL, "hello")
>>>>
>>>> ???
>>
>> My first thought is that a NULL width should be treated the same as no
>> width at all (equivalent to width=0), rather than raising an
>> exception.
>>
>>> minor update - fix align NULL for %L
>>
>> You need to do the same for a NULL value with %s, which currently
>> produces an empty string regardless of the width.
>
> have others same opinion? Usually I don't like hide NULLs, but this is
> corner case (and specific function) and I have not strong opinion on
> this issue.
>

One use case for this might be something like

SELECT format('%*s', minimum_width, value) FROM some_table;

Throwing an exception when minimum_width is NULL doesn't seem
particularly useful. Intuitively, it just means that row has no
minimum width, so I think we should allow it.

I think the case where the value is NULL is much more clear-cut.
format('%s') produces '' (empty string). So format('%3s') should
produce ' '.

>>
>> The documentation also needs to be updated. I'm thinking perhaps
>> format() should now have its own separate sub-section in the manual,
>> rather than trying to cram it's docs into a single table row. I can
>> help with the docs if you like.
>
> please, if you can, write it. I am sure, so you do it significantly
> better than me.
>

Here is my first draft. I've also attached the generated HTML page,
because it's not so easy to read an SGML patch.

Regards,
Dean

2013/2/10 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 9 February 2013 18:30, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>>> There are a new question
>>>>>
>>>>> what should be result of
>>>>>
>>>>> format(">>%2$*1$s<<", NULL, "hello")
>>>>>
>>>>> ???
>>>
>>> My first thought is that a NULL width should be treated the same as no
>>> width at all (equivalent to width=0), rather than raising an
>>> exception.
>>>
>>>> minor update - fix align NULL for %L
>>>
>>> You need to do the same for a NULL value with %s, which currently
>>> produces an empty string regardless of the width.
>>
>> have others same opinion? Usually I don't like hide NULLs, but this is
>> corner case (and specific function) and I have not strong opinion on
>> this issue.
>>
>
> One use case for this might be something like
>
> SELECT format('%*s', minimum_width, value) FROM some_table;
>
> Throwing an exception when minimum_width is NULL doesn't seem
> particularly useful. Intuitively, it just means that row has no
> minimum width, so I think we should allow it.
>
> I think the case where the value is NULL is much more clear-cut.
> format('%s') produces '' (empty string). So format('%3s') should
> produce ' '.
>

ok - in this case I can accept NULL as "ignore width"

>
>>>
>>> The documentation also needs to be updated. I'm thinking perhaps
>>> format() should now have its own separate sub-section in the manual,
>>> rather than trying to cram it's docs into a single table row. I can
>>> help with the docs if you like.
>>
>> please, if you can, write it. I am sure, so you do it significantly
>> better than me.
>>
>
> Here is my first draft. I've also attached the generated HTML page,
> because it's not so easy to read an SGML patch.
>

nice

I have only one point - I am think, so format function should be in
table 9-6 - some small text with reference to special section.

Regards

Pavel

> Regards,
> Dean

On 10 February 2013 12:37, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> >>
Here is my first draft. I've also attached the generated HTML page,
>> because it's not so easy to read an SGML patch.
>>
>
> nice
>
> I have only one point - I am think, so format function should be in
> table 9-6 - some small text with reference to special section.
>

It is already there in table 9-6, referring to the new section.

Here is a minor update though -- I changed the name of the first
optional argument from "str" to "formatarg", since they are no longer
necessarily strings.

Regards,
Dean

Hello

updated patch

* merged Dean's doc
* allow NULL as width

Regards

Pavel

2013/2/11 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 10 February 2013 12:37, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> >>
> Here is my first draft. I've also attached the generated HTML page,
>>> because it's not so easy to read an SGML patch.
>>>
>>
>> nice
>>
>> I have only one point - I am think, so format function should be in
>> table 9-6 - some small text with reference to special section.
>>
>
> It is already there in table 9-6, referring to the new section.
>
> Here is a minor update though -- I changed the name of the first
> optional argument from "str" to "formatarg", since they are no longer
> necessarily strings.
>
> Regards,
> Dean

On 11 February 2013 14:29, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> updated patch
>
> * merged Dean's doc
> * allow NULL as width
>

Hi,
I have not had time to look at this properly, but it doesn't look as
though you have fixed the other problem I mentioned up-thread, with %s
for NULL values:

SELECT format('|%s|', NULL);
Result: ||
SELECT format('|%5s|', NULL);
Result: ||

In the second case, I think it should produce | |.

Regards,
Dean

Hello

2013/2/13 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 11 February 2013 14:29, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Hello
>>
>> updated patch
>>
>> * merged Dean's doc
>> * allow NULL as width
>>
>
> Hi,
> I have not had time to look at this properly, but it doesn't look as
> though you have fixed the other problem I mentioned up-thread, with %s
> for NULL values:
>
> SELECT format('|%s|', NULL);
> Result: ||
> SELECT format('|%5s|', NULL);
> Result: ||
>
> In the second case, I think it should produce | |.

fixed

Regards

Pavel Stehule

>
> Regards,
> Dean

Hello, Could you let me review this patch?

> >> * merged Dean's doc
> >> * allow NULL as width

I understand that this patch aims pure expansion of format's
current behavior and to mimic the printf in SUS glibc (*1).

(*1) http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html

This patch seems to preserve the behaviors of current
implement. And also succeeds in mimicking almost of SUS without
very subtle difference.

Attached is the new patch which I've edited following the
comments below and some fixed of typos, and added a few regtests.

If you have no problem with this, I'll send this to committer.

What do you think of this?

My comments are below,

======================================
Following is a comment about the behavior.

- The minus('-') is a flag, not a sign nor a operator. So this
seems permitted to appear more than one time. For example,
printf(">>%-------10s<<", "hoge") yields the output
">>hoge______<<" safely. This is consistent with the behavior
when negative value is supplied to '-*' conversion.

Followings are some comments about coding,

in text_format_parse_digits,

- is_valid seems to be the primary return value so returning
this as function's return value should make the caller more
simple.

- Although the compiler should deal properly with that, I don't
think it proper to use the memory pointed by function
parameters as local working storage. *inum and *is_valid in
the while loop should be replaced with local variables and
set them after the values are settled.

for TEXT_FORMAT_NEXT_CHAR,

- This macro name sounds somewhat confusing and this could be
used also in text_format_parse_digits. I propose
FORWARD_PARSE_POINT instead. Also I removed end_ptr from
macro parameters but I'm not sure of the pertinence of that.

for text_format_parse_format:

- Using start_ptr as a working pointer makes the name
inappropriate.

- Out parameters seems somewhat redundant. indirect_width and
indirect_width_parameter could be merged using 0 to indicate
unnumbered.

for text_format:

- maximum number of function argument limited to FUNC_MAX_ARGS
(100), so no need to care of wrap around of argument index, I
suppose.

- Something seems confusing at the lines follow

| /* Not enough arguments? Deduct 1 to avoid counting format string. */
| if (arg > nargs - 1)

This expression does not have so special meaning. The maximum
index in an zero-based array should not be equal to or larger
than the number of the elements of it. If that's not your
intent, some rewrite would be needed..

- Only int4 is directly read for width value in the latest
patch, but int2 can also be directly readable and it should
be needed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Umm. sorry,

> If you have no problem with this, I'll send this to committer.

I just found that this patch already has a revewer. I've seen
only Status field in patch list..

Should I leave this to you, Dean?

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello

I have no objections,

Thank you for update

Regards

Pavel

2013/2/28 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
> Hello, Could you let me review this patch?
>
>> >> * merged Dean's doc
>> >> * allow NULL as width
>
> I understand that this patch aims pure expansion of format's
> current behavior and to mimic the printf in SUS glibc (*1).
>
> (*1) http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html
>
> This patch seems to preserve the behaviors of current
> implement. And also succeeds in mimicking almost of SUS without
> very subtle difference.
>
> Attached is the new patch which I've edited following the
> comments below and some fixed of typos, and added a few regtests.
>
> If you have no problem with this, I'll send this to committer.
>
> What do you think of this?
>
>
> My comments are below,
>
> ======================================
> Following is a comment about the behavior.
>
> - The minus('-') is a flag, not a sign nor a operator. So this
> seems permitted to appear more than one time. For example,
> printf(">>%-------10s<<", "hoge") yields the output
> ">>hoge______<<" safely. This is consistent with the behavior
> when negative value is supplied to '-*' conversion.
>
>
> Followings are some comments about coding,
>
> in text_format_parse_digits,
>
> - is_valid seems to be the primary return value so returning
> this as function's return value should make the caller more
> simple.
>
> - Although the compiler should deal properly with that, I don't
> think it proper to use the memory pointed by function
> parameters as local working storage. *inum and *is_valid in
> the while loop should be replaced with local variables and
> set them after the values are settled.
>
> for TEXT_FORMAT_NEXT_CHAR,
>
> - This macro name sounds somewhat confusing and this could be
> used also in text_format_parse_digits. I propose
> FORWARD_PARSE_POINT instead. Also I removed end_ptr from
> macro parameters but I'm not sure of the pertinence of that.
>
> for text_format_parse_format:
>
> - Using start_ptr as a working pointer makes the name
> inappropriate.
>
> - Out parameters seems somewhat redundant. indirect_width and
> indirect_width_parameter could be merged using 0 to indicate
> unnumbered.
>
> for text_format:
>
> - maximum number of function argument limited to FUNC_MAX_ARGS
> (100), so no need to care of wrap around of argument index, I
> suppose.
>
> - Something seems confusing at the lines follow
>
> | /* Not enough arguments? Deduct 1 to avoid counting format string. */
> | if (arg > nargs - 1)
>
> This expression does not have so special meaning. The maximum
> index in an zero-based array should not be equal to or larger
> than the number of the elements of it. If that's not your
> intent, some rewrite would be needed..
>
> - Only int4 is directly read for width value in the latest
> patch, but int2 can also be directly readable and it should
> be needed.
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

On 28 February 2013 11:25, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Umm. sorry,
>
>> If you have no problem with this, I'll send this to committer.
>
> I just found that this patch already has a revewer. I've seen
> only Status field in patch list..
>
> Should I leave this to you, Dean?
>

Sorry, I've been meaning to review this properly for some time, but
I've been swamped with other work, so I'm happy for you to take over.

My overall impression is that the patch is in good shape, and provides
valuable new functionality, and it is probably close to being ready
for committer.

I think that the only other behavioural glitch I spotted was that it
fails to catch one overflow case, which should generate an "out of
ranger" error:

select format('|%*s|', -2147483648, 'foo');
Result: |foo|

because -(-2147483648) = 0 in signed 32-bit integers.

Apart from that, I didn't find any problems during my testing.

Thanks for your review.

Regards,
Dean

Hello

2013/2/28 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 28 February 2013 11:25, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Umm. sorry,
>>
>>> If you have no problem with this, I'll send this to committer.
>>
>> I just found that this patch already has a revewer. I've seen
>> only Status field in patch list..
>>
>> Should I leave this to you, Dean?
>>
>
> Sorry, I've been meaning to review this properly for some time, but
> I've been swamped with other work, so I'm happy for you to take over.
>
> My overall impression is that the patch is in good shape, and provides
> valuable new functionality, and it is probably close to being ready
> for committer.
>
> I think that the only other behavioural glitch I spotted was that it
> fails to catch one overflow case, which should generate an "out of
> ranger" error:
>
> select format('|%*s|', -2147483648, 'foo');
> Result: |foo|
>
> because -(-2147483648) = 0 in signed 32-bit integers.

fixed - next other overflow check added

Regards

Pavel

>
> Apart from that, I didn't find any problems during my testing.
>
> Thanks for your review.
>
> Regards,
> Dean

Hello,

> > I think that the only other behavioural glitch I spotted was that it
> > fails to catch one overflow case, which should generate an "out of
> > ranger" error:
> >
> > select format('|%*s|', -2147483648, 'foo');
> > Result: |foo|
> >
> > because -(-2147483648) = 0 in signed 32-bit integers.

Ouch. Thanks for pointing out.

> fixed - next other overflow check added

Your change shown below seems assuming that the two's complement
of the most negative number in integer types is identical to
itself, and looks working as expected at least on
linux/x86_64. But C standard defines it as undefined, (As far as
I hear :-).

| if (width != 0)
| {
| int32 _width = -width;
|
| if (SAMESIGN(width, _width))
| ereport(ERROR,

Instead, I think we can deny it by simply comparing with
INT_MIN. I modified the patch like so and put some modifications
on styling.

Finally, enlargeStringInfo fails just after for large numbers. We
might should keep it under certain length to get rid of memory
exhaustion.

Anyway, I'll send this patch to committers as it is in this
message.

best wishes,

--
Kyotaro Horiguchi
NTT Open Source Software Center

2013/3/5 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
> Hello,
>
>> > I think that the only other behavioural glitch I spotted was that it
>> > fails to catch one overflow case, which should generate an "out of
>> > ranger" error:
>> >
>> > select format('|%*s|', -2147483648, 'foo');
>> > Result: |foo|
>> >
>> > because -(-2147483648) = 0 in signed 32-bit integers.
>
> Ouch. Thanks for pointing out.
>
>> fixed - next other overflow check added
>
> Your change shown below seems assuming that the two's complement
> of the most negative number in integer types is identical to
> itself, and looks working as expected at least on
> linux/x86_64. But C standard defines it as undefined, (As far as
> I hear :-).
>
> | if (width != 0)
> | {
> | int32 _width = -width;
> |
> | if (SAMESIGN(width, _width))
> | ereport(ERROR,
>

this pattern was used elsewhere in pg

> Instead, I think we can deny it by simply comparing with
> INT_MIN. I modified the patch like so and put some modifications
> on styling.

ook - I have not enough expirience with this topic and I cannot say
what is preferred.

>
> Finally, enlargeStringInfo fails just after for large numbers. We
> might should keep it under certain length to get rid of memory
> exhaustion.

I though about it, but I don't know a correct value - probably any
width specification higher 1MB will be bogus and can be blocked ?? Our
VARLENA max size is 1GB so with should not be higher than 1GB ever.

what do you thinking about these limits?

Regards

Pavel

>
> Anyway, I'll send this patch to committers as it is in this
> message.
>
> best wishes,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center

On 5 March 2013 13:46, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2013/3/5 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
>> Hello,
>>
>>> > I think that the only other behavioural glitch I spotted was that it
>>> > fails to catch one overflow case, which should generate an "out of
>>> > ranger" error:
>>> >
>>> > select format('|%*s|', -2147483648, 'foo');
>>> > Result: |foo|
>>> >
>>> > because -(-2147483648) = 0 in signed 32-bit integers.
>>
>> Ouch. Thanks for pointing out.
>>
>>> fixed - next other overflow check added
>>
>> Your change shown below seems assuming that the two's complement
>> of the most negative number in integer types is identical to
>> itself, and looks working as expected at least on
>> linux/x86_64. But C standard defines it as undefined, (As far as
>> I hear :-).
>>
>> | if (width != 0)
>> | {
>> | int32 _width = -width;
>> |
>> | if (SAMESIGN(width, _width))
>> | ereport(ERROR,
>>
>
> this pattern was used elsewhere in pg
>
>> Instead, I think we can deny it by simply comparing with
>> INT_MIN. I modified the patch like so and put some modifications
>> on styling.
>
> ook - I have not enough expirience with this topic and I cannot say
> what is preferred.
>
>>
>> Finally, enlargeStringInfo fails just after for large numbers. We
>> might should keep it under certain length to get rid of memory
>> exhaustion.
>
> I though about it, but I don't know a correct value - probably any
> width specification higher 1MB will be bogus and can be blocked ?? Our
> VARLENA max size is 1GB so with should not be higher than 1GB ever.
>
> what do you thinking about these limits?
>

I think it's fine as it is.

It's no different from repeat() for example. We allow repeat('a',
1000000000) so allowing format('%1000000000s', '') seems reasonable,
although probably not very useful.

Upping either beyond 1GB generates an out of memory error, which also
seems reasonable -- I can't imagine why you would want such a long
string.

Regards,
Dean

2013/3/5 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 5 March 2013 13:46, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2013/3/5 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
>>> Hello,
>>>
>>>> > I think that the only other behavioural glitch I spotted was that it
>>>> > fails to catch one overflow case, which should generate an "out of
>>>> > ranger" error:
>>>> >
>>>> > select format('|%*s|', -2147483648, 'foo');
>>>> > Result: |foo|
>>>> >
>>>> > because -(-2147483648) = 0 in signed 32-bit integers.
>>>
>>> Ouch. Thanks for pointing out.
>>>
>>>> fixed - next other overflow check added
>>>
>>> Your change shown below seems assuming that the two's complement
>>> of the most negative number in integer types is identical to
>>> itself, and looks working as expected at least on
>>> linux/x86_64. But C standard defines it as undefined, (As far as
>>> I hear :-).
>>>
>>> | if (width != 0)
>>> | {
>>> | int32 _width = -width;
>>> |
>>> | if (SAMESIGN(width, _width))
>>> | ereport(ERROR,
>>>
>>
>> this pattern was used elsewhere in pg
>>
>>> Instead, I think we can deny it by simply comparing with
>>> INT_MIN. I modified the patch like so and put some modifications
>>> on styling.
>>
>> ook - I have not enough expirience with this topic and I cannot say
>> what is preferred.
>>
>>>
>>> Finally, enlargeStringInfo fails just after for large numbers. We
>>> might should keep it under certain length to get rid of memory
>>> exhaustion.
>>
>> I though about it, but I don't know a correct value - probably any
>> width specification higher 1MB will be bogus and can be blocked ?? Our
>> VARLENA max size is 1GB so with should not be higher than 1GB ever.
>>
>> what do you thinking about these limits?
>>
>
> I think it's fine as it is.
>
> It's no different from repeat() for example. We allow repeat('a',
> 1000000000) so allowing format('%1000000000s', '') seems reasonable,
> although probably not very useful.
>
> Upping either beyond 1GB generates an out of memory error, which also
> seems reasonable -- I can't imagine why you would want such a long
> string.
>
> Regards,
> Dean

ok

Pavel

Kyotaro HORIGUCHI escribió:
> Umm. sorry,
>
> > If you have no problem with this, I'll send this to committer.
>
> I just found that this patch already has a revewer. I've seen
> only Status field in patch list..

Patches can be reviewed by more than one people. It's particularly
useful if they have different things to say. So don't hesitate to
review patches that have already been reviewed by other people.

In fact, you can even review committed patches; it's not unlikely that
you will be able to find bugs in those, too.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello,

> Patches can be reviewed by more than one people. It's particularly
> useful if they have different things to say. So don't hesitate to
> review patches that have already been reviewed by other people.

Thanks for the advice. I was anxious about who among the
reviewers is, and when to make a decisision if the patch has
reached the level or not, I'll take it more easy.

> In fact, you can even review committed patches; it's not unlikely that
> you will be able to find bugs in those, too.

Umm.. to be sure..

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> [ format-width-20130305.patch ]

Applied with some mostly-cosmetic adjustments. I also took the liberty
of changing some of the error message texts to line up more closely
with the expanded documentation (eg, use "format specifier" not
"conversion specifier" because that's the phrase used in the docs).

regards, tom lane

Thank you for committing this patch.

> Applied with some mostly-cosmetic adjustments. I also took the
> liberty of changing some of the error message texts to line up
> more closely with the expanded documentation (eg, use "format
> specifier" not "conversion specifier" because that's the phrase
> used in the docs).

I looked over the modifications. Thanks for refining rather large
portion of documentation and comments.. and code.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center