Re: Add mention of execution time memory for enable_partitionwise_* GUCs

On Thu, Jul 18, 2024 at 4:24 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 18 Jul 2024 at 22:28, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Thu, Jul 18, 2024 at 3:33 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > > hmm? please tell me what word other than "can" best describes
> > > something that is possible to happen but does not always happen under
> > > all circumstances.
> >
> > May I suggest "may"? :) [1], [2], [3].
>
> Is this a wind-up?
>
> If it's not, I disagree that "may" is a better choice.  The
> possibility example in your first link says "It may rain tomorrow.
> (possibility)", but that's something someone would only say if there
> was some evidence to support that, e.g. ominous clouds on the horizon
> at dusk, or >0% chance of precipitation on the weather forecast.
> Nobody is going to say that unless there's some supporting evidence.
> For the executor using work_mem * nparts, we've no evidence either.
> It's just a >0% possibility with no supporting evidence.

I am not a native English speaker and might have made a mistake when
interpreting the definitions. Will leave that aside.

>
> > My point is, we need to highlight the role of work_mem. So modify both
> > the descriptions.
>
> I considered writing about work_mem, but felt I wanted to keep it as
> brief as possible and just have some words that might make someone
> think twice.  The details in the work_mem documentation should inform
> the reader that work_mem is per executor node.  It likely wouldn't
> hurt to have more documentation around which executor node types can
> use a work_mem, which use work_mem * hash_mem_multiplier and which use
> neither. We tend to not write too much about executor nodes in the
> documents, so I'm not proposing that for this patch.

Something I didn't write in my first reply but wanted to discuss was
the intention of adding those GUCs. Sorry for missing it in my first
email. According to [1] these GUCs were added because of increased
memory consumption during planning and time taken to plan the query.
The execution time memory consumption issue was known even back then
but the GUC was not set to default because of that. But your patch
proposes to change the narrative. In the same thread [1], you will
find the discussion about turning the default to ON once we have fixed
planner's memory and time consumption. We have patches addressing
those issues [2] [3]. With your proposed changes we will almost never
have a chance to turn those GUCs ON by default. That seems a rather
sad prospect.

I am fine if we want to mention that the executor may consume a large
amount of memory when these GUCs are turned ON. Users may decide to
turn those OFF if they can not afford to spend that much memory during
execution. But I don't like tying execution time consumption with
default OFF.

[1] https://www.postgresql.org/message-id/CA+TgmoYggDp6k-HXNAgrykZh79w6nv2FevpYR_jeMbrORDaQrA@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAExHW5stmOUobE55pMt83r8UxvfCph%2BPvo5dNpdrVCsBgXEzDQ%40mail.gmail.com
[3] https://www.postgresql.org/message-id/flat/CAJ2pMkZNCgoUKSE+_5LthD+KbXKvq6h2hQN8Esxpxd+cxmgomg@mail.gmail.com


--
Best Wishes,
Ashutosh Bapat