Thread: More issues with expressions always false (no patch)
Continuing on always false expressions.
There are three difficult cases, whose solutions which needs to be well thought out.
This is not a case of simply removing the expressions, perhaps, but have to be sure.
First case:
src \ backend \ executor \ nodeSubplan.c (line 507)
if (node-> hashtable)
node-> hastable is assigned with NULL at line 498, so the test will always fail.
Second case:
Here the case is similar, but worse.
src \ backend \ executor \ nodeSubplan.c (line 535)
if (node-> hashnulls)
ResetTupleHashTable (node-> hashtable);
node-> hashnulls is assigned with NULL at line 499, so the test will always fail.
Otherwise, it would have already been discovered, because it would inevitably occur
an access violation, since > hashtable would be accessed.
Third case:
\ src \ backend \ utils \ cache \ relcache.c (line 5190)
if (relation-> rd_pubactions)
It will never be executed, because if relation-> rd_pubactions is true, the function returns on line 5154.
There are three difficult cases, whose solutions which needs to be well thought out.
This is not a case of simply removing the expressions, perhaps, but have to be sure.
First case:
src \ backend \ executor \ nodeSubplan.c (line 507)
if (node-> hashtable)
node-> hastable is assigned with NULL at line 498, so the test will always fail.
Second case:
Here the case is similar, but worse.
src \ backend \ executor \ nodeSubplan.c (line 535)
if (node-> hashnulls)
ResetTupleHashTable (node-> hashtable);
node-> hashnulls is assigned with NULL at line 499, so the test will always fail.
Otherwise, it would have already been discovered, because it would inevitably occur
an access violation, since > hashtable would be accessed.
Third case:
\ src \ backend \ utils \ cache \ relcache.c (line 5190)
if (relation-> rd_pubactions)
It will never be executed, because if relation-> rd_pubactions is true, the function returns on line 5154.
regards,
Ranier Vilela
On 12/20/19 1:01 AM, Ranier Vilela wrote:> First case: > src \ backend \ executor \ nodeSubplan.c (line 507) > > if (node-> hashtable) > > node-> hastable is assigned with NULL at line 498, so the test will > always fail. > > Second case: > Here the case is similar, but worse. > > src \ backend \ executor \ nodeSubplan.c (line 535) > if (node-> hashnulls) > ResetTupleHashTable (node-> hashtable); These two look like likely bugs. It looks like the code will always create new hash tables despite commit https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=356687bd825e5ca7230d43c1bffe7a59ad2e77bd intending to reset them if they already exist. Additionally it looks like the code would reset the wrong hash table in the second place if the bug was fixed. I have attached a patch. > Third case: > \ src \ backend \ utils \ cache \ relcache.c (line 5190) > if (relation-> rd_pubactions) > > It will never be executed, because if relation-> rd_pubactions is true, > the function returns on line 5154. I have not looked into this one in detail, but the free at line 5192 looks like potentially dead code. Andreas
Attachment
On 12/20/19 1:54 AM, Andreas Karlsson wrote: > On 12/20/19 1:01 AM, Ranier Vilela wrote:> First case: >> Third case: >> \ src \ backend \ utils \ cache \ relcache.c (line 5190) >> if (relation-> rd_pubactions) >> >> It will never be executed, because if relation-> rd_pubactions is >> true, the function returns on line 5154. > > I have not looked into this one in detail, but the free at line 5192 > looks like potentially dead code. I have looked at it now and it seems like this code has been dead since the function was originally implemented in 665d1fad99e. Peter, what do you think? Andreas
Attachment
Andreas Karlsson <andreas@proxel.se> writes:
> On 12/20/19 1:54 AM, Andreas Karlsson wrote:
>> On 12/20/19 1:01 AM, Ranier Vilela wrote:> First case:
>>> Third case:
>>> \ src \ backend \ utils \ cache \ relcache.c (line 5190)
>>> if (relation-> rd_pubactions)
>>>
>>> It will never be executed, because if relation-> rd_pubactions is
>>> true, the function returns on line 5154.
>> I have not looked into this one in detail, but the free at line 5192
>> looks like potentially dead code.
> I have looked at it now and it seems like this code has been dead since
> the function was originally implemented in 665d1fad99e.
I would not put a whole lot of faith in that. This argument supposes
that nothing else can touch the relcache entry while we are doing
GetRelationPublications and the pg_publication syscache accesses inside
the foreach loop. Now in practice, yeah, it's somewhat unlikely that
anything down inside there would take an interest in our relation's
publication actions, especially if our relation isn't a system catalog.
But there are closely related situations in other relcache functions
that compute cached values like this where we *do* have to worry about
reentrant/recursive use of the function. I think the "useless" free
is cheap insurance against a permanent memory leak, as well as more
like the coding in nearby functions like RelationGetIndexAttrBitmap.
I wouldn't change it.
regards, tom lane
On 12/20/19 10:34 PM, Tom Lane wrote: > I think the "useless" free > is cheap insurance against a permanent memory leak, as well as more > like the coding in nearby functions like RelationGetIndexAttrBitmap. > I wouldn't change it. Good point, if there is a pattern it is good to follow it. But I am pretty sure that the other issue Ranier's static analysis discovered is a real bug and not just about shaving off a virtually no clock cycles (but I am not 100% sure my fix is correct). Will submit it to the commitfest so people can take a look. Andreas