more psprintf() use

Started by Peter Eisentrautover 12 years ago9 messageshackers

peter_e@gmx.net

over 12 years ago

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

Robert Haas

robertmhaas@gmail.com

over 12 years ago

In reply to: Peter Eisentraut (#1)

Re: more psprintf() use

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

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

Heikki Linnakangas

heikki.linnakangas@enterprisedb.com

over 12 years ago

In reply to: Peter Eisentraut (#1)

Re: more psprintf() use

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

--
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@anarazel.de

over 12 years ago

In reply to: Heikki Linnakangas (#3)

Re: more psprintf() use

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

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

Tom Lane

tgl@sss.pgh.pa.us

over 12 years ago

In reply to: Andres Freund (#4)

Re: more psprintf() use

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

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

Alvaro Herrera

alvherre@2ndquadrant.com

over 12 years ago

In reply to: Peter Eisentraut (#1)

Re: more psprintf() use

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

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

Peter Eisentraut

peter_e@gmx.net

over 12 years ago

In reply to: Tom Lane (#5)

Re: more psprintf() use

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?

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

Peter Eisentraut

peter_e@gmx.net

over 12 years ago

In reply to: Alvaro Herrera (#6)

Re: more psprintf() use

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.

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

Tom Lane

tgl@sss.pgh.pa.us

over 12 years ago

In reply to: Peter Eisentraut (#7)

Re: more psprintf() use

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
/messages/by-id/12646.1383420576@sss.pgh.pa.us

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

more psprintf() use

Attachments: