Lists: | pgsql-hackers |
---|
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | more psprintf() use |
Date: | 2014-01-02 03:14:43 |
Message-ID: | 1388632483.20392.1.camel@vanquo.pezone.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Here is a more or less straightforward patch to add more use of
psprintf() in place of hardcoded palloc(N) + sprintf() and the like.
Attachment | Content-Type | Size |
---|---|---|
0001-Add-more-use-of-psprintf.patch | text/x-patch | 14.1 KB |
From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-02 03:27:10 |
Message-ID: | CA+TgmoZjtvXKsoHP4hssWz_eEL1XDRpHT9j27H-sLsoOyhyeYw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
On Wed, Jan 1, 2014 at 10:14 PM, Peter Eisentraut <peter_e(at)gmx(dot)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
From: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-02 07:49:48 |
Message-ID: | 52C51A1C.90904@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Andres Freund <andres(at)2ndquadrant(dot)com> |
---|---|
To: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-02 08:53:34 |
Message-ID: | 20140102085334.GB2683@awork2.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Andres Freund <andres(at)2ndquadrant(dot)com> |
Cc: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-02 14:28:19 |
Message-ID: | 27309.1388672899@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Andres Freund <andres(at)2ndquadrant(dot)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
From: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-02 19:12:45 |
Message-ID: | 20140102191245.GA7035@eldon.alvh.no-ip.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)2ndquadrant(dot)com> |
Cc: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-05 20:25:57 |
Message-ID: | 52C9BFD5.6080903@gmx.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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?
From: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-05 20:30:27 |
Message-ID: | 52C9C0E3.7000307@gmx.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
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.
From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | Andres Freund <andres(at)2ndquadrant(dot)com>, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: more psprintf() use |
Date: | 2014-01-05 20:41:49 |
Message-ID: | 31344.1388954509@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Lists: | pgsql-hackers |
Peter Eisentraut <peter_e(at)gmx(dot)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