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.
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
Attachments:
fix-subplan-hash-reset.patchtext/x-patch; charset=UTF-8; name=fix-subplan-hash-reset.patchDownload
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 2c364bdd23..840f4033ac 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -495,8 +495,6 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
* need to store subplan output rows that contain NULL.
*/
MemoryContextReset(node->hashtablecxt);
- node->hashtable = NULL;
- node->hashnulls = NULL;
node->havehashrows = false;
node->havenullrows = false;
@@ -533,7 +531,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
}
if (node->hashnulls)
- ResetTupleHashTable(node->hashtable);
+ ResetTupleHashTable(node->hashnulls);
else
node->hashnulls = BuildTupleHashTableExt(node->parent,
node->descRight,
@@ -549,6 +547,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
node->hashtempcxt,
false);
}
+ else
+ node->hashnulls = NULL;
/*
* We are probably in a short-lived expression-evaluation context. Switch
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
Attachments:
remove-dead-code.patchtext/x-patch; charset=UTF-8; name=remove-dead-code.patchDownload
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 50f8912c13..1ae41f776d 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5187,12 +5187,6 @@ GetRelationPublicationActions(Relation relation)
break;
}
- if (relation->rd_pubactions)
- {
- pfree(relation->rd_pubactions);
- relation->rd_pubactions = NULL;
- }
-
/* Now save copy of the actions in the relcache entry. */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
relation->rd_pubactions = palloc(sizeof(PublicationActions));
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