public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: David Rowley <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: A performance issue with Memoize
Date: Thu, 25 Jan 2024 13:32:42 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvoTyLG35Rm26F7-MFmX3kv-uAs9FD4XF7fqy-B0W6o_UQ@mail.gmail.com>
References: <CAMbWs48XHJEK1Q1CzAQ7L9sTANTs9W1cepXu8=Kc0quUL+tg4Q@mail.gmail.com>
	<CAApHDvoTyLG35Rm26F7-MFmX3kv-uAs9FD4XF7fqy-B0W6o_UQ@mail.gmail.com>

David Rowley <[email protected]> writes:
> I think fixing it your way makes sense.  I don't really see any reason
> why we should have two. However...

> Another way it *could* be fixed would be to get rid of pull_paramids()
> and change create_memoize_plan() to set keyparamids to all the param
> IDs that match are equal() to each param_exprs.  That way nodeMemoize
> wouldn't purge the cache as we'd know the changing param is accounted
> for in the cache.  For the record, I don't think that's better, but it
> scares me a bit less as I don't know what other repercussions there
> are of applying your patch to get rid of the duplicate
> NestLoopParam.paramval.

> I'd feel better about doing it your way if Tom could comment on if
> there was a reason he put the function calls that way around in
> 5ebaaa494.

I'm fairly sure I thought it wouldn't matter because of the Param
de-duplication done in paramassign.c.  However, Richard's example
shows that's not so, because process_subquery_nestloop_params is
picky about the param ID assigned to a particular Var while
replace_nestloop_params is not.  So flipping the order makes sense.
I'd change the comment though, maybe to

    /*
     * Replace any outer-relation variables with nestloop params.
     *
     * We must provide nestloop params for both lateral references of
     * the subquery and outer vars in the scan_clauses.  It's better
     * to assign the former first, because that code path requires
     * specific param IDs, while replace_nestloop_params can adapt
     * to the IDs assigned by process_subquery_nestloop_params.
     * This avoids possibly duplicating nestloop params when the
     * same Var is needed for both reasons.
     */

However ... it seems like we're not out of the woods yet.  Why
is Richard's proposed test case still showing

+         ->  Memoize (actual rows=5000 loops=N)
+               Cache Key: t1.two, t1.two

Seems like there is missing de-duplication logic, or something.

> I also feel like we might be getting a bit close to the minor version
> releases to be adjusting this stuff in back branches.

Yeah, I'm not sure I would change this in the back branches.

			regards, tom lane

view thread (20+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: A performance issue with Memoize
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox