json_populate_record issue - TupleDesc reference leak

by the way - this feature is undocumented - I though so only value used as
type holder is not used.

Should be documented better, - if I understand - it is base stone for
implementation #= hstore operator

some nice example

postgres=# select json_populate_record('(10,20)'::pt, '{"a":30}');
json_populate_record
----------------------
(30,20)
(1 row)

a mentioned bug is corner case - bugfix is simple

diff --git a/src/backend/utils/adt/jsonfuncs.c
b/src/backend/utils/adt/jsonfuncs.c
new file mode 100644
index a8cdeaa..6e83f78
*** a/src/backend/utils/adt/jsonfuncs.c
--- b/src/backend/utils/adt/jsonfuncs.c
*************** populate_record_worker(FunctionCallInfo
*** 2114,2119 ****
--- 2114,2122 ----
                 */
                if (hash_get_num_entries(json_hash) == 0 && rec)
                {
+                       if (have_record_arg)
+                               ReleaseTupleDesc(tupdesc);
+
                        hash_destroy(json_hash);
                        PG_RETURN_POINTER(rec);
                }

2015-02-23 18:27 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:

On 02/23/2015 12:56 PM, Pavel Stehule wrote:

by the way - this feature is undocumented - I though so only value
used as type holder is not used.

Should be documented better, - if I understand - it is base stone for
implementation #= hstore operator

some nice example

postgres=# select json_populate_record('(10,20)'::pt, '{"a":30}');
json_populate_record
----------------------
(30,20)
(1 row)

a mentioned bug is corner case - bugfix is simple

diff --git a/src/backend/utils/adt/jsonfuncs.c 
b/src/backend/utils/adt/jsonfuncs.c
new file mode 100644
index a8cdeaa..6e83f78
*** a/src/backend/utils/adt/jsonfuncs.c
--- b/src/backend/utils/adt/jsonfuncs.c
*************** populate_record_worker(FunctionCallInfo
*** 2114,2119 ****
--- 2114,2122 ----
*/
if (hash_get_num_entries(json_hash) == 0 && rec)
{
+                       if (have_record_arg)
+                               ReleaseTupleDesc(tupdesc);
+
hash_destroy(json_hash);
PG_RETURN_POINTER(rec);
}

This doesn't look quite right. Shouldn't we unconditionally release the
Tupledesc before the returns at lines 2118 and 2127, just as we do at
the bottom of the function at line 2285?

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

This doesn't look quite right. Shouldn't we unconditionally release the
Tupledesc before the returns at lines 2118 and 2127, just as we do at
the bottom of the function at line 2285?

I think Pavel's patch is probably OK as-is, because the tupdesc returned
by get_call_result_type isn't reference-counted; but I agree the code
would look cleaner your way. If the main exit isn't bothering to
distinguish this then the early exits should not either.

What I'm wondering about, though, is this bit at line 2125:

/* same logic as for json */
if (!have_record_arg && rec)
PG_RETURN_POINTER(rec);

If that's supposed to be the same logic as in the other path, then how
is it that !have_record_arg has anything to do with whether the JSON
object is empty? Either the code is broken, or the comment is.

regards, tom lane

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

On Thu, Feb 26, 2015 at 05:31:44PM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

This doesn't look quite right. Shouldn't we unconditionally release the
Tupledesc before the returns at lines 2118 and 2127, just as we do at
the bottom of the function at line 2285?

I think Pavel's patch is probably OK as-is, because the tupdesc returned
by get_call_result_type isn't reference-counted; but I agree the code
would look cleaner your way. If the main exit isn't bothering to
distinguish this then the early exits should not either.

What I'm wondering about, though, is this bit at line 2125:

/* same logic as for json */
if (!have_record_arg && rec)
PG_RETURN_POINTER(rec);

If that's supposed to be the same logic as in the other path, then how
is it that !have_record_arg has anything to do with whether the JSON
object is empty? Either the code is broken, or the comment is.

Where are we on this?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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

Still issue is not fixed still

create type pt as (a int, b int);
postgres=# select json_populate_record('(10,20)'::pt, '{}');
WARNING: TupleDesc reference leak: TupleDesc 0x7f413ca325b0 (16560,-1)
still referenced

2015-04-30 14:32 GMT+02:00 Bruce Momjian <bruce@momjian.us>:

On 04/30/2015 08:36 AM, Pavel Stehule wrote:

Still issue is not fixed still

create type pt as (a int, b int);
postgres=# select json_populate_record('(10,20)'::pt, '{}');
WARNING: TupleDesc reference leak: TupleDesc 0x7f413ca325b0
(16560,-1) still referenced

Looking at it now. (Please remember not to top-post.)

cheers

andrew

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