On Mon, 29 Mar 2021 at 15:56, Zhihong Yu <zyu@yugabyte.com> wrote:
> 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.
OK. I change that.
> + * 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.
I've done a bit of rewording in that paragraph.
> + rcstate->mem_used -= freed_mem;
>
> Should there be assertion that after the subtraction, mem_used stays non-negative ?
I'm not sure. I ended up adding one and also adjusting the #ifdef in
remove_cache_entry() which had some code to validate the memory
accounting so that it compiles when USE_ASSERT_CHECKING is defined.
I'm unsure if that's a bit too expensive to enable during debugs but I
didn't really want to leave the code in there unless it's going to get
some exercise on the buildfarm.
> + if (found && entry->complete)
> + {
> + node->stats.cache_hits += 1; /* stats update */
>
> Once inside the if block, we would return.
OK change.
> + else
> + {
> The else block can be unindented (dropping else keyword).
changed.
> + * 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.
I changed the comment, but I did leave a mention that I'm still not
sure if it should be an Assert() or an elog.
The attached patch is an updated version of the Result Cache patch
containing the changes for the things you highlighted plus a few other
things.
I pushed the change to simplehash.h and the estimate_num_groups()
change earlier, so only 1 patch remaining.
Also, I noticed the CFBof found another unstable parallel regression
test. This was due to some code in show_resultcache_info() which
skipped parallel workers that appeared to not help out. It looks like
on my machine the worker never got a chance to do anything, but on one
of the CFbot's machines, it did. I ended up changing the EXPLAIN
output so that it shows the cache statistics regardless of if the
worker helped or not.
David
