Thread: enable_resultcache confusion
Are we going to be forever explaining that enable_resultcache doesn't cache query results? Do we need a different name? enable_innerjoin_cache? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
On Thu, Jul 8, 2021 at 12:51 PM Bruce Momjian <bruce@momjian.us> wrote: > Are we going to be forever explaining that enable_resultcache doesn't > cache query results? Yes, I can see that causing ongoing confusion. Naming things is really hard... -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Jul 8, 2021 at 10:29 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jul 8, 2021 at 12:51 PM Bruce Momjian <bruce@momjian.us> wrote:
> Are we going to be forever explaining that enable_resultcache doesn't
> cache query results?
Yes, I can see that causing ongoing confusion. Naming things is really hard...
I agree that the chosen name is problematic. To borrow existing technical nomenclature, what we seem to be doing here is adding "Node Memoization" [1].
"enable_nodememoization" would work for me - by avoiding Result and using Node the focus should remain without the bowels of the planner's plan and not move to the output of the query as a whole. "Node Cache" would probably work just as well if a wholesale change to Memoization doesn't seem appealing, but the semantics of that term seem closer to what is happening here.
The description in the commit message suggests we can use this for a wide variety of nodes so adding any node specific typing to the name seems unwise.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> "enable_nodememoization" would work for me
That seems pretty unreadable. Maybe just "enable_memoization"?
Really if we're going to do something here, we can't merely mess
with the GUC name. David had expressed a willingness to rename
everything about ResultCache some time ago, but nobody stepped up
with a better name.
Maybe name the plan node type Memoize, and the GUC "enable_memoize"?
regards, tom lane
> On 8 Jul 2021, at 19:52, David G. Johnston <david.g.johnston@gmail.com> wrote: > "enable_nodememoization" would work for me Include "node" concatenated with other words risks users reading it as "enable no demomoization", with confusion following. -- Daniel Gustafsson https://vmware.com/
On Thu, Jul 08, 2021 at 12:51:45PM -0400, Bruce Momjian wrote: > Are we going to be forever explaining that enable_resultcache doesn't > cache query results? Do we need a different name? > enable_innerjoin_cache? See also https://www.postgresql.org/message-id/CAApHDvos7z90hyiuX3kcFe0Q_4WYWfwFABOygo4aeBbemyF9sQ@mail.gmail.com -- Justin
On Thu, Jul 8, 2021 at 11:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Maybe name the plan node type Memoize, and the GUC "enable_memoize"?
+1
David J.
On Fri, Jul 9, 2021 at 6:03 AM David G. Johnston <david.g.johnston@gmail.com> wrote: > On Thu, Jul 8, 2021 at 11:00 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Maybe name the plan node type Memoize, and the GUC "enable_memoize"? > > +1 +1
On Fri, 9 Jul 2021 at 06:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Maybe name the plan node type Memoize, and the GUC "enable_memoize"? I really like that name. I'll wait to see if anyone else wants to voice their opinion before I do any renaming work. David
On Fri, Jul 9, 2021 at 11:35 AM David Rowley <dgrowleyml@gmail.com> wrote: > I really like that name. > > I'll wait to see if anyone else wants to voice their opinion before I > do any renaming work. I like it, too. -- Robert Haas EDB: http://www.enterprisedb.com
On Sat, 10 Jul 2021 at 07:30, Robert Haas <robertmhaas@gmail.com> wrote: > > On Fri, Jul 9, 2021 at 11:35 AM David Rowley <dgrowleyml@gmail.com> wrote: > > I really like that name. > > > > I'll wait to see if anyone else wants to voice their opinion before I > > do any renaming work. > > I like it, too. Great. I've attached my first draft patch to do the renaming. It would be good to move fairly quickly on this before REL_14_STABLE and master diverge too much. At the moment the patch applies to both versions without any issues. Does anyone not like the proposed name? David
Attachment
On Mon, Jul 12, 2021 at 02:56:58AM +1200, David Rowley wrote: > On Sat, 10 Jul 2021 at 07:30, Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Fri, Jul 9, 2021 at 11:35 AM David Rowley <dgrowleyml@gmail.com> wrote: > > > I really like that name. > > > > > > I'll wait to see if anyone else wants to voice their opinion before I > > > do any renaming work. > > > > I like it, too. > > Great. I've attached my first draft patch to do the renaming. In REL_14, maybe you'd also want to update the release notes reference | This is useful if only a small percentage of rows is checked on | the inner side and is controlled by <xref | linkend="guc-enable-resultcache"/>. -- Justin
On Mon, 12 Jul 2021 at 03:22, Justin Pryzby <pryzby@telsasoft.com> wrote: > | This is useful if only a small percentage of rows is checked on > | the inner side and is controlled by <xref > | linkend="guc-enable-resultcache"/>. You might be right there, but I'm not too sure if I changed that that it might cause a mention of the rename to be missed in the changes since beta2 notes. Additionally, I was unsure about touching typedefs.list. In the patch I changed it, but wasn't too sure if that was the correct thing to do. In normal circumstances, i.e writing new code, I'd not touch it. David
David Rowley <dgrowleyml@gmail.com> writes:
> On Mon, 12 Jul 2021 at 03:22, Justin Pryzby <pryzby@telsasoft.com> wrote:
>> | This is useful if only a small percentage of rows is checked on
>> | the inner side and is controlled by <xref
>> | linkend="guc-enable-resultcache"/>.
> You might be right there, but I'm not too sure if I changed that that
> it might cause a mention of the rename to be missed in the changes
> since beta2 notes.
You need to change it, because IIUC that will be a dangling
cross-reference, causing the v14 docs to fail to build at all.
> Additionally, I was unsure about touching typedefs.list. In the patch
> I changed it, but wasn't too sure if that was the correct thing to do.
> In normal circumstances, i.e writing new code, I'd not touch it.
I'd suggest replacing it in typedefs.list, since there is unlikely to
be any further update to v14's copy otherwise, and even in HEAD I'm not
sure it'd get updated before we approach the v15 branch.
regards, tom lane
On Tue, 13 Jul 2021 at 01:38, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <dgrowleyml@gmail.com> writes: > > On Mon, 12 Jul 2021 at 03:22, Justin Pryzby <pryzby@telsasoft.com> wrote: > >> | This is useful if only a small percentage of rows is checked on > >> | the inner side and is controlled by <xref > >> | linkend="guc-enable-resultcache"/>. > > > You might be right there, but I'm not too sure if I changed that that > > it might cause a mention of the rename to be missed in the changes > > since beta2 notes. > > You need to change it, because IIUC that will be a dangling > cross-reference, causing the v14 docs to fail to build at all. Good point. I'll adjust that for PG14. I plan on pushing the patch to master and PG14 in 24 hours time. If anyone is still on the fence or wishes to object to the name, please let it be known before then. David
On Tue, 13 Jul 2021 at 12:01, David Rowley <dgrowleyml@gmail.com> wrote: > I plan on pushing the patch to master and PG14 in 24 hours time. If > anyone is still on the fence or wishes to object to the name, please > let it be known before then. Renaming complete. Result Cache is gone. Welcome Memoize. David
On Mon, Jul 12, 2021 at 11:47:27AM +1200, David Rowley wrote: > On Mon, 12 Jul 2021 at 03:22, Justin Pryzby <pryzby@telsasoft.com> wrote: > > | This is useful if only a small percentage of rows is checked on > > | the inner side and is controlled by <xref > > | linkend="guc-enable-resultcache"/>. > > You might be right there, but I'm not too sure if I changed that that > it might cause a mention of the rename to be missed in the changes > since beta2 notes. The commit will appear in the PG 14 git log, and someone will then see the rename is part of the changes since the previous beta. Also, when the release notes "as of" date is updated, all commits since the previous "as of" date will be reviewed. Yes, someone might try to update the release notes for this change and see it was already done, but that is easily handled. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.