答复: refCount on ComDiagsArea

[Zhu, Wen-Jun]

Hi,

It is true that there are some other functions incrementing the reference 
count, like in atp_struct::copyAtp:

  if (from->getDiagsArea())
    from->getDiagsArea()->incrRefCount();

  setDiagsArea(from->getDiagsArea());

incrRefCount() is called to increase the ref count.

But when I check the result of command
    grep setDiagsArea * -B4 -A4 -rw
I find lots of code have not invoked incrRefCount(), like 
core/sql/exp/exp_eval.cpp:
 479                   if (retcode == ex_expr::EXPR_ERROR)
 480                 {
 481                   if (diagsArea != atp1->getDiagsArea())
 482                     atp1->setDiagsArea(diagsArea);
 483
 484                   return retcode;
 485                 }
There's bug here.

So, how about to wrap operator= invocation in atp_struct::setDiagsArea(), and 
in operator= we deal with the ref count, like Qifan suggested?


-----邮件原件-----
发件人: Selva Govindarajan <selva.govindara...@esgyn.com> 
发送时间: 2018年8月15日 0:23
收件人: dev@trafodion.apache.org
主题: RE: refCount on ComDiagsArea

I second Dave's  comment. I was about to comment in similar lines. When I saw 
Dave's message, I had no second thoughts to second it.

Selva

-----Original Message-----
From: Qifan Chen <qifan.c...@esgyn.com> 
Sent: Tuesday, August 14, 2018 9:11 AM
To: dev@trafodion.apache.org
Subject: Re: refCount on ComDiagsArea

Hi,


I personally also like the 2nd option which is to overload the operator= for 
ComDiagsArea.


But because of high number of calls to atp_struct::setDiagsArea() in the 
executor, it is better to hide the call to operator=(ComDiagsArea&) inside 
atp_struct::setDiagsArea().


Also keep in mind that we can not ubiquitously use C11 structs such as 
shared_ptr or make_shared in our code base yet since we still need to support 
platforms such as CentOS6 that do not have C11 support package.


Thanks --Qifan

________________________________
From: Dave Birdsall <dave.birds...@esgyn.com>
Sent: Tuesday, August 14, 2018 10:10:43 AM
To: dev@trafodion.apache.org
Subject: RE: refCount on ComDiagsArea

Hi,

I have not researched this area, but it strikes me as one that could be very 
delicate. It may be that in most code paths it is assumed that some other 
function is incrementing the reference count. Great care should be taken in 
modifying this otherwise it may lead to memory leaks. I am hoping others who 
are more knowledgeable will add to this discussion.

Can you give more insight into what problem led you here?

Dave

-----Original Message-----
From: Zhu, Wen-Jun <wenjun....@esgyn.cn>
Sent: Tuesday, August 14, 2018 4:11 AM
To: dev@trafodion.apache.org
Subject: refCount on ComDiagsArea

hi,

When setting a ComDiagsArea, I find the refCount did not increase, in function 
atp_struct::setDiagsArea of file core/sql/exp/ExpAtp.h:

inline void atp_struct::setDiagsArea(ComDiagsArea* diagsArea) {
  if (diagsArea_)
    diagsArea_->decrRefCount();

  diagsArea_ = diagsArea;
}

I guess this is a problem.

There are two solutions to fix this:

1.     Invoke incrRefCount for ComDiagsArea to increase, just after the 
assignment.

2.     Overload the operator= for ComDiagsArea, to increase within the 
operator=,
I find operator= is declared in ComDiags.h, but there is no implementation.

The 2nd solution may be better, as the both increment for the left-hand side 
ComDiagsArea and the decrement for the right-hand side ComDiagsAre can be 
handled within a single operator=, which is friendly to users, like shared_ptr 
in C++.

Regards,
Wenjun Zhu