On Wed, 27 Jul 2022 at 17:46, Mark Murawski <markm-li...@intellasoft.net>
wrote:
> Hi All!
>
> I'm working on a new feature for plperl in postgresql to populate some
> metadata in main::_FN for running plperl functions from postgres sql.
>
> I've snipped out some extra code that's unrelated to focus on the issue
> at hand. If the section labeled 'Leaking Section' is entirely
> commented out (and of course the related SvREFCNT_dec_current is
> commented as well), then there's no memory issue at all. If I do use
> this section of code, something is not freed and I'm not sure what that
> is, since I'm very new to perl-c
>
>
>
> /*
> * Decrement the refcount of the given SV within the active Perl
> interpreter
> *
> * This is handy because it reloads the active-interpreter pointer, saving
> * some notation in callers that switch the active interpreter.
> */
> static inline void
> SvREFCNT_dec_current(SV *sv)
> {
> dTHX;
>
> SvREFCNT_dec(sv);
> }
>
> static SV *
> plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
> {
> dTHX;
> dSP;
>
> HV *hv; // hash
> SV *FNsv; // scalar reference to the
> hash
> SV *svFN; // local reference to the
> hash?
>
> ENTER;
> SAVETMPS;
>
> /* Give functions some metadata about what's going on in $_FN
> (Similar to $_TD for triggers) */
>
> // Leaking Section {
> FNsv = get_sv("main::_FN", GV_ADD);
> save_item(FNsv); /* local $_FN */
>
> hv = newHV(); // create new hash
> hv_ksplit(hv, 12); /* pre-grow the hash */
> hv_store_string(hv, "name", cstr2sv(desc->proname));
>
> svFN = newRV_noinc((SV *) hv); // reference to the new hash
> sv_setsv(FNsv, svFN);
>
So at this point FNsv and svFN both are references to the same HV. But you
dont free svFN.
Likely a simple SvREFCNT_dec(svFN) would do it.
Basically you are nearly doing this:
for my $FNsv ($main::FN) {
my %hash;
my $svFN= \%hash; push @LEAK, \$svFN;
$FNsv= $svFN;
}
If you put a sv_2mortal(svFN) or an explicit refcount decrement on svFN
then I expect the leak would go away. Note the reason i use a for loop here
is I am trying to emphasize that $FNsv IS $main::FN, as the topic of a for
loop is an alias which is as close as you can get in Perl to the C level
operation of copying a pointer to an SV structure. (The difference being
that in Perl aliases are refcounted copies and C they are not refcounted).
Compare this to $svFN which is clearly a new variable.
What you should be doing is this:
my %hash;
$main::FN= \%hash;
Which would be something like this:
sv_upgrade(FNsv,SVtRV);
SvRV_set(FNsv,(SV*)newHV());
SvROK_on(FNsv);
// Leaking Section }
>
> // dostuff
>
> SvREFCNT_dec_current(hv);
>
> PUTBACK;
> FREETMPS;
> LEAVE;
>
> ...snip...
> }
>
>
> If anyone would like to see the full context, I've attached the entire
> file. My additions are between the 'New..........' sections
>
> My question is... the perl-c api docs do not make it clear for which
> allocations or accesses that you need to decrement the ref count.
>
I would say the docs actually do explain this. Most functions will specify
that you need to do the refcount incrementing yourself if it is relevant.
A general rule of thumb however is that any item you create yourself which
is not "owned by perl" by being attached to some data structure it exposes
is your problem to deal with. So for instance this:
FNsv = get_sv("main::_FN", GV_ADD);
is getting you a local pointer to the structure representing $main::_FN
which is a global var. Thus it is stored in the global stash, and thus Perl
knows about it and will clean it up, you don't need to worry about its
refcount unless you store it in something else that is refcount managed.
On the other hand
hv = newHV(); // create new hash
is a pointer to a new HV structure which is not stored in any place that
perl knows about. So if you dont arrange for it to be recount decremented
it will leak. However you then did this:
svFN = newRV_noinc((SV *) hv); // reference to the new hash
this is creating a new reference to the hash. The combination of the two
basically is equivalent to creating an anonymous hashref. The "noinc" is
there because the hv starts off with a refcount of 1, and the new reference
is the thing that will "own" that refcount. So at this point you no longer
need to manage 'hv' provided you correctly manage svFN.
You then do this:
sv_setsv(FNsv, svFN);
This results in the refcount of 'hv' being incremented, as there are now
two RV's pointing at it. You need to free up the temporary, or even better,
simply dont use it. You dont need it, you can simply turn FNsv into an RV,
and then set its RV field appropriately, the leak will go away and the code
will be more efficient.
Another comment is regarding the hv_ksplit() which seems redundant. The
initial number of buckets is 8, the new size up is 12 or 16. Setting it to
12 is likely just a waste. Either set it much larger, or dont use it at all.
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
