Re: Broken resetting of subplan hash tables

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> The code for resetting the hash tables of the SubPlanState node in
> buildSubPlanHash() looks like it can never run, and additionally it
> would be broken if it would ever run. This issue was introduced in
> commit 356687bd825e5ca7230d43c1bffe7a59ad2e77bd.

Right. Justin Pryzby also noted this a couple weeks back, but we
didn't deal with it at that point because we were up against a
release deadline.

> As far as I gather the following is true:

> 1. It sets node->hashtable and node->hashnulls to NULL a few lines
> before checking if they are not NULL which means the code for resetting
> them cannot ever be reached.

Yeah. Those lines should have been removed when the ResetTupleHashTable
logic was added.

> 2. But if we changed to code so that the ResetTupleHashTable() calls are
> reachable we would hit a typo. It resets node->hashtable twice and never
> resets node->hashnulls which would cause non-obvious bugs.

Right.

> 3. Additionally since the memory context used by the hash tables is
> reset in buildSubPlanHash() if we start resetting hash tables we will
> get a use after free bug.

Nope, not right. The hash table metadata is now allocated in the
es_query_cxt; what is in the hashtablecxt is just tuples, and that
does need to be cleared, per the comment for ResetTupleHashTable.
Your patch as given results in a nasty memory leak, which is easily
demonstrated with a small mod to the regression test case I added:

select sum(ss.tst::int) from
generate_series(1,10000000) o cross join lateral (
select i.ten in (select f1 from int4_tbl where f1 <= o) as tst,
random() as r
from onek i where i.unique1 = o%1000 ) ss;

> Since the current behavior of the code in HEAD is not actually broken,
> it is just an optimization which is not used, this fix does not have to
> be backpatched.

Unfortunately ... this test case also leaks memory like mad in
HEAD, v12, and v11, because all of them are rebuilding the hash
table from scratch without clearing the old one. So this is
indeed broken and a back-patch is necessary.

I noted while looking at this that most of the calls of
ResetTupleHashTable are actually never reached in the regression
tests (per code coverage report) so I made up some test cases
that do reach 'em, and included those in the commit.

TBH, I think that this tuple table API is seriously misdesigned;
it is confusing and very error-prone to have the callers need to
reset the tuple context separately from calling ResetTupleHashTable.
Also, the callers all look like their resets are intended to destroy
the whole hashtable not just its contents (as indeed they were doing,
before the faulty commit). But I didn't attempt to fix that today.

regards, tom lane