Re: Hybrid Hash/Nested Loop joins and caching results from subplans

Hi,
For show_resultcache_info()

+   if (rcstate->shared_info != NULL)
+   {

The negated condition can be used with a return. This way, the loop can be
unindented.

+ * ResultCache nodes are intended to sit above a parameterized node in the
+ * plan tree in order to cache results from them.

Since the parameterized node is singular, it would be nice if 'them' can be
expanded to refer to the source of result cache.

+   rcstate->mem_used -= freed_mem;

Should there be assertion that after the subtraction, mem_used stays
non-negative ?

+               if (found && entry->complete)
+               {
+                   node->stats.cache_hits += 1;    /* stats update */

Once inside the if block, we would return.
+               else
+               {
The else block can be unindented (dropping else keyword).

+                * return 1 row.  XXX is this worth the check?
+                */
+               if (unlikely(entry->complete))

Since the check is on a flag (with minimal overhead), it seems the check
can be kept, with the question removed.

Cheers

On Sun, Mar 28, 2021 at 7:21 PM David Rowley <dgrowleyml@gmail.com> wrote:

> On Wed, 24 Mar 2021 at 00:42, David Rowley <dgrowleyml@gmail.com> wrote:
> > I've now cleaned up the 0001 patch. I ended up changing a few places
> > where we pass the RestrictInfo->clause to contain_volatile_functions()
> > to instead pass the RestrictInfo itself so that there's a possibility
> > of caching the volatility property for a subsequent call.
> >
> > I also made a pass over the remaining patches and for the 0004 patch,
> > aside from the name, "Result Cache", I think that it's ready to go. We
> > should consider before RC1 if we should have enable_resultcache switch
> > on or off by default.
> >
> > Does anyone care to have a final look at these patches? I'd like to
> > start pushing them fairly soon.
>
> I've now pushed the 0001 patch to cache the volatility of PathTarget
> and RestrictInfo.
>
> I'll be looking at the remaining patches over the next few days.
>
> Attached are a rebased set of patches on top of current master. The
> only change is to the 0003 patch (was 0004) which had an unstable
> regression test for parallel plan with a Result Cache.  I've swapped
> the unstable test for something that shouldn't fail randomly depending
> on if a parallel worker did any work or not.
>
> David
>