Re: Check lateral references within PHVs for memoize cache keys

2024-03-18 Thread Richard Guo
On Fri, Feb 2, 2024 at 5:18 PM Richard Guo wrote: > The v4 patch does not apply any more. I've rebased it on master. > Nothing else has changed. > Here is another rebase over master so it applies again. I also added a commit message to help review. Nothing else has changed. Thanks Richard

Re: Check lateral references within PHVs for memoize cache keys

2024-02-02 Thread Richard Guo
On Mon, Dec 25, 2023 at 3:01 PM Richard Guo wrote: > On Thu, Jul 13, 2023 at 3:12 PM Richard Guo > wrote: > >> So I'm wondering if it'd be better that we move all this logic of >> computing additional lateral references within PHVs to get_memoize_path, >> where we can examine only PHVs that are

Re: Check lateral references within PHVs for memoize cache keys

2023-12-24 Thread Richard Guo
On Thu, Jul 13, 2023 at 3:12 PM Richard Guo wrote: > So I'm wondering if it'd be better that we move all this logic of > computing additional lateral references within PHVs to get_memoize_path, > where we can examine only PHVs that are evaluated at innerrel. And > considering that these lateral

Re: Check lateral references within PHVs for memoize cache keys

2023-07-13 Thread Richard Guo
On Sun, Jul 9, 2023 at 12:17 PM David Rowley wrote: > I just pushed a fix for this. Thanks for reporting it. BTW, I noticed a typo in the comment inside paraminfo_get_equal_hashops. foreach(lc, innerrel->lateral_vars) { Node *expr = (Node *) lfirst(lc);

Re: Check lateral references within PHVs for memoize cache keys

2023-07-13 Thread Richard Guo
On Sun, Jul 9, 2023 at 1:28 AM Tom Lane wrote: > More generally, it's not clear to me why we should need to look inside > lateral PHVs in the first place. Wouldn't the lateral PHV itself > serve fine as a cache key? Do you mean we use the lateral PHV directly as a cache key? Hmm, it seems to

Re: Check lateral references within PHVs for memoize cache keys

2023-07-13 Thread Richard Guo
On Sun, Jul 9, 2023 at 1:28 AM Tom Lane wrote: > Richard Guo writes: > > Rebase the patch on HEAD as cfbot reminds. > > This fix seems like a mess. The function that is in charge of filling > RelOptInfo.lateral_vars is extract_lateral_references; or at least > that was how it was done up to

Re: Check lateral references within PHVs for memoize cache keys

2023-07-08 Thread David Rowley
On Sat, 8 Jul 2023 at 17:24, Paul A Jungwirth wrote: > One adjacent thing I noticed is that when we renamed "Result Cache" to > "Memoize" this bit of the docs in config.sgml got skipped (probably > because of the line break): > > Hash tables are used in hash joins, hash-based aggregation,

Re: Check lateral references within PHVs for memoize cache keys

2023-07-08 Thread David Rowley
On Sun, 9 Jul 2023 at 05:28, Tom Lane wrote: > More generally, it's not clear to me why we should need to look inside > lateral PHVs in the first place. Wouldn't the lateral PHV itself > serve fine as a cache key? For Memoize specifically, I purposefully made it so the expression was used as a

Re: Check lateral references within PHVs for memoize cache keys

2023-07-08 Thread Tom Lane
Richard Guo writes: > Rebase the patch on HEAD as cfbot reminds. This fix seems like a mess. The function that is in charge of filling RelOptInfo.lateral_vars is extract_lateral_references; or at least that was how it was done up to now. Can't we compute these additional references there? If

Re: Check lateral references within PHVs for memoize cache keys

2023-07-07 Thread Paul A Jungwirth
On Tue, Jul 4, 2023 at 12:33 AM Richard Guo wrote: > > Rebase the patch on HEAD as cfbot reminds. All of this seems good to me. I can reproduce the problem, tests pass, and the change is sensible as far as I can tell. One adjacent thing I noticed is that when we renamed "Result Cache" to

Re: Check lateral references within PHVs for memoize cache keys

2023-07-04 Thread Richard Guo
On Fri, Dec 30, 2022 at 11:00 AM Richard Guo wrote: > On Fri, Dec 9, 2022 at 5:16 PM Richard Guo wrote: > >> Actually we do have checked PHVs for lateral references, earlier in >> create_lateral_join_info. But that time we only marked lateral_relids >> and direct_lateral_relids, without

Re: Check lateral references within PHVs for memoize cache keys

2023-01-30 Thread Richard Guo
On Tue, Jan 24, 2023 at 10:07 AM David Rowley wrote: > I'm surprised to see that it's only Memoize that ever makes use of > lateral_vars. I'd need a bit more time to process your patch, but one > additional thought I had was that I wonder if the following code is > still needed in nodeMemoize.c

Re: Check lateral references within PHVs for memoize cache keys

2023-01-23 Thread David Rowley
On Fri, 30 Dec 2022 at 16:00, Richard Guo wrote: > > On Fri, Dec 9, 2022 at 5:16 PM Richard Guo wrote: >> >> Actually we do have checked PHVs for lateral references, earlier in >> create_lateral_join_info. But that time we only marked lateral_relids >> and direct_lateral_relids, without

Re: Check lateral references within PHVs for memoize cache keys

2022-12-29 Thread Richard Guo
On Fri, Dec 9, 2022 at 5:16 PM Richard Guo wrote: > Actually we do have checked PHVs for lateral references, earlier in > create_lateral_join_info. But that time we only marked lateral_relids > and direct_lateral_relids, without remembering the lateral expressions. > So I'm wondering whether we

Check lateral references within PHVs for memoize cache keys

2022-12-09 Thread Richard Guo
If we intend to generate a memoize node atop a path, we need some kind of cache key. Currently we search the path's parameterized clauses and its parent's lateral_vars for that. ISTM this is not sufficient because their might be lateral references derived from PlaceHolderVars, which can also act