Обсуждение: json_populate_record issue - TupleDesc reference leak

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

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

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. +

<div dir="ltr">Still issue is not fixed still<br /><br />create type pt as (a int, b int);<br />postgres=# select
json_populate_record('(10,20)'::pt,'{}');<br />WARNING:  TupleDesc reference leak: TupleDesc 0x7f413ca325b0 (16560,-1)
stillreferenced<br /><br /></div><div class="gmail_extra"><br /><div class="gmail_quote">2015-04-30 14:32 GMT+02:00
BruceMomjian <span dir="ltr"><<a href="mailto:bruce@momjian.us" target="_blank">bruce@momjian.us</a>></span>:<br
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="HOEnZb"><divclass="h5">On Thu, Feb 26, 2015 at 05:31:44PM -0500, Tom Lane wrote:<br /> > Andrew Dunstan
<<ahref="mailto:andrew@dunslane.net">andrew@dunslane.net</a>> writes:<br /> > > This doesn't look quite
right.Shouldn't we unconditionally release the<br /> > > Tupledesc before the returns at lines 2118 and 2127,
justas we do at<br /> > > the bottom of the function at line 2285?<br /> ><br /> > I think Pavel's patch is
probablyOK as-is, because the tupdesc returned<br /> > by get_call_result_type isn't reference-counted; but I agree
thecode<br /> > would look cleaner your way.  If the main exit isn't bothering to<br /> > distinguish this then
theearly exits should not either.<br /> ><br /> > What I'm wondering about, though, is this bit at line 2125:<br
/>><br /> >               /* same logic as for json */<br /> >               if (!have_record_arg &&
rec)<br/> >                       PG_RETURN_POINTER(rec);<br /> ><br /> > If that's supposed to be the same
logicas in the other path, then how<br /> > is it that !have_record_arg has anything to do with whether the JSON<br
/>> object is empty?  Either the code is broken, or the comment is.<br /><br /></div></div>Where are we on this?<br
/><spanclass="HOEnZb"><font color="#888888"><br /> --<br />   Bruce Momjian  <<a
href="mailto:bruce@momjian.us">bruce@momjian.us</a>>       <a href="http://momjian.us"
target="_blank">http://momjian.us</a><br/>   EnterpriseDB                             <a href="http://enterprisedb.com"
target="_blank">http://enterprisedb.com</a><br/><br />   + Everyone has their own god. +<br
/></font></span></blockquote></div><br/></div> 

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