Horton

Thread

Commits

  1. Correctly re-use hash tables in buildSubPlanHash().

  2. Reset, not recreate, execGrouping.c style hashtables.

  1. More issues with expressions always false (no patch)

    Ranier Vilela <ranier.vf@gmail.com> — 2019-12-20T00:01:11Z

    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.

regards,
Ranier Vilela

  2. Re: More issues with expressions always false (no patch)

    Andreas Karlsson <andreas@proxel.se> — 2019-12-20T00:54:53Z

    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

  3. Re: More issues with expressions always false (no patch)

    Andreas Karlsson <andreas@proxel.se> — 2019-12-20T15:35:25Z

    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

  4. Re: More issues with expressions always false (no patch)

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-20T21:34:38Z

    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

  5. Re: More issues with expressions always false (no patch)

    Andreas Karlsson <andreas@proxel.se> — 2020-01-08T18:41:55Z

    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