Hi, Give it a try!
One place to check carefully is how it affects ComDiagsArea objects sent over IPC (thanks to Selva for this pointer). When we send an object over IPC, the reference count loses its meaning and has to be re-initialized on the receiving end appropriately. Check that your change doesn't break anything in this area. Dave -----Original Message----- From: Zhu, Wen-Jun <[email protected]> Sent: Tuesday, August 14, 2018 7:52 PM To: [email protected] Subject: 答复: refCount on ComDiagsArea 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 <[email protected]> 发送时间: 2018年8月15日 0:23 收件人: [email protected] 主题: 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 <[email protected]> Sent: Tuesday, August 14, 2018 9:11 AM To: [email protected] 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 <[email protected]> Sent: Tuesday, August 14, 2018 10:10:43 AM To: [email protected] 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 <[email protected]> Sent: Tuesday, August 14, 2018 4:11 AM To: [email protected] 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
