Обсуждение: more psprintf() use

Here is a more or less straightforward patch to add more use of
psprintf() in place of hardcoded palloc(N) + sprintf() and the like.

On Wed, Jan 1, 2014 at 10:14 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> Here is a more or less straightforward patch to add more use of
> psprintf() in place of hardcoded palloc(N) + sprintf() and the like.

Looks nifty.

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

On 01/02/2014 05:14 AM, Peter Eisentraut wrote:
> diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
> index 772a5ca..8331a56 100644
> --- a/contrib/hstore/hstore_io.c
> +++ b/contrib/hstore/hstore_io.c
> @@ -1114,11 +1114,7 @@
>       HEntry       *entries = ARRPTR(in);
>
>       if (count == 0)
> -    {
> -        out = palloc(1);
> -        *out = '\0';
> -        PG_RETURN_CSTRING(out);
> -    }
> +        PG_RETURN_CSTRING("");
>
>       buflen = 0;

Is it legal to return a constant with PG_RETURN_CSTRING? Grepping 
around, I don't see that being done anywhere else, but there are places 
that do PG_RETURN_CSTRING(pstrdup(<constant>))...

- Heikki

On 2014-01-02 09:49:48 +0200, Heikki Linnakangas wrote:
> On 01/02/2014 05:14 AM, Peter Eisentraut wrote:
> >diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
> >index 772a5ca..8331a56 100644
> >--- a/contrib/hstore/hstore_io.c
> >+++ b/contrib/hstore/hstore_io.c
> >@@ -1114,11 +1114,7 @@
> >      HEntry       *entries = ARRPTR(in);
> >
> >      if (count == 0)
> >-    {
> >-        out = palloc(1);
> >-        *out = '\0';
> >-        PG_RETURN_CSTRING(out);
> >-    }
> >+        PG_RETURN_CSTRING("");
> >
> >      buflen = 0;
> 
> Is it legal to return a constant with PG_RETURN_CSTRING? Grepping around, I
> don't see that being done anywhere else, but there are places that do
> PG_RETURN_CSTRING(pstrdup(<constant>))...

I don't see why it wouldn't be legal - constant strings have static
storage duration, i.e. the program lifetime. And I can see nothing that
would allow pfree()ing the return value of cstring returning functions
in the general case.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-01-02 09:49:48 +0200, Heikki Linnakangas wrote:
>> Is it legal to return a constant with PG_RETURN_CSTRING? Grepping around, I
>> don't see that being done anywhere else, but there are places that do
>> PG_RETURN_CSTRING(pstrdup(<constant>))...

> I don't see why it wouldn't be legal - constant strings have static
> storage duration, i.e. the program lifetime. And I can see nothing that
> would allow pfree()ing the return value of cstring returning functions
> in the general case.

Heikki is right and you are wrong.  There is an ancient supposition that
datatype output functions, in particular, always return palloc'd strings.

I recently got rid of the pfree's in the main output path, cf commit
b006f4ddb988568081f8290fac77f9402b137120, which might explain why this
patch passes regression tests; but there are still places in the code (and
even more likely in third-party code) that will try to pfree the results.
So -1 for this particular change.  The pstrdup that Heikki suggests would
be safer practice.
        regards, tom lane

Peter Eisentraut wrote:

> psprintf() in place of hardcoded palloc(N) + sprintf() and the like.
> 

> +    values[j++] = psprintf("%d", stat.blkno);
> +    values[j++] = psprintf("%c", stat.type);
> +    values[j++] = psprintf("%d", stat.live_items);
> +    values[j++] = psprintf("%d", stat.dead_items);
> +    values[j++] = psprintf("%d", stat.avg_item_size);
> +    values[j++] = psprintf("%d", stat.page_size);
> +    values[j++] = psprintf("%d", stat.free_size);
> +    values[j++] = psprintf("%d", stat.btpo_prev);
> +    values[j++] = psprintf("%d", stat.btpo_next);
> +    values[j++] = psprintf("%d", (stat.type == 'd') ? stat.btpo.xact : stat.btpo.level);
> +    values[j++] = psprintf("%d", stat.btpo_flags);
>  
>      tuple = BuildTupleFromCStrings(TupleDescGetAttInMetadata(tupleDesc),
>                                     values);

In cases such as this one, I have often wondered whether it'd be better
to write this as DatumGetSometype() plus heap_form_tuple, instead of
printing to strings and then building a tuple from those.

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

On 1/2/14, 9:28 AM, Tom Lane wrote:
> Heikki is right and you are wrong.  There is an ancient supposition that
> datatype output functions, in particular, always return palloc'd strings.
> 
> I recently got rid of the pfree's in the main output path, cf commit
> b006f4ddb988568081f8290fac77f9402b137120, which might explain why this
> patch passes regression tests; but there are still places in the code (and
> even more likely in third-party code) that will try to pfree the results.

Well, that seems kind of dangerous.  The next guys is going to write an
extension that is returning string constants directly, and there is no
straightforward way to detect this problem.  Perhaps we should have some
mode similar to the CLOBBER and COPY_*_TREES symbols to force a pfree()
in assertion-enabled builds?

On 1/2/14, 2:12 PM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
> 
>> psprintf() in place of hardcoded palloc(N) + sprintf() and the like.
>>
> 
>> +    values[j++] = psprintf("%d", stat.blkno);
>> +    values[j++] = psprintf("%c", stat.type);
>> +    values[j++] = psprintf("%d", stat.live_items);
>> +    values[j++] = psprintf("%d", stat.dead_items);
>> +    values[j++] = psprintf("%d", stat.avg_item_size);
>> +    values[j++] = psprintf("%d", stat.page_size);
>> +    values[j++] = psprintf("%d", stat.free_size);
>> +    values[j++] = psprintf("%d", stat.btpo_prev);
>> +    values[j++] = psprintf("%d", stat.btpo_next);
>> +    values[j++] = psprintf("%d", (stat.type == 'd') ? stat.btpo.xact : stat.btpo.level);
>> +    values[j++] = psprintf("%d", stat.btpo_flags);
>>  
>>      tuple = BuildTupleFromCStrings(TupleDescGetAttInMetadata(tupleDesc),
>>                                     values);
> 
> In cases such as this one, I have often wondered whether it'd be better
> to write this as DatumGetSometype() plus heap_form_tuple, instead of
> printing to strings and then building a tuple from those.

Probably.  As you can see, this style is only used in a few contrib
modules that all came from the same source, I think.

Peter Eisentraut <peter_e@gmx.net> writes:
> On 1/2/14, 9:28 AM, Tom Lane wrote:
>> Heikki is right and you are wrong.  There is an ancient supposition that
>> datatype output functions, in particular, always return palloc'd strings.
>> 
>> I recently got rid of the pfree's in the main output path, cf commit
>> b006f4ddb988568081f8290fac77f9402b137120, which might explain why this
>> patch passes regression tests; but there are still places in the code (and
>> even more likely in third-party code) that will try to pfree the results.

> Well, that seems kind of dangerous.  The next guys is going to write an
> extension that is returning string constants directly, and there is no
> straightforward way to detect this problem.  Perhaps we should have some
> mode similar to the CLOBBER and COPY_*_TREES symbols to force a pfree()
> in assertion-enabled builds?

Seems kinda backwards.  If we want to put any effort into this issue,
it'd be better to head in the direction of making the world safe for
output functions to return constants, ie deprecate rather than enforce
the practice of pfree'ing their results.  But see
http://www.postgresql.org/message-id/12646.1383420576@sss.pgh.pa.us
        regards, tom lane