Hi Andy,
Thanks for your feedback.
On Fri, Sep 22, 2023 at 8:22 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:
>
> 1). The commit message of patch 1 just says how it does but doesn't
> say why it does. After reading through the discussion, I suggest making
> it clearer to others.
>
> ...
> After the planning is done, it may still occupy lots of memory which is
> allocated but not pfree-d. All of these memories can't be used in the later
> stage. This patch will report such memory usage in order to making some
> future mistakes can be found in an easier way.
> ...
That's a good summary of how the memory report can be used. Will
include a line about usage in the commit message.
>
> Planning Memory: used=15088 bytes allocated=16384 bytes
>
> Word 'Planning' is kind of a time duration, so the 'used' may be the
> 'used' during the duration or 'used' after the duration. Obviously you
> means the later one but it is not surprising others think it in the other
> way. So I'd like to reword the metrics to:
We report "PLanning Time" hence used "Planning memory". Would
"Planner" be good instead of "Planning"?
I agree "Planner" is better than "Planning" personally.
>
> Memory Occupied (Now): Parser: 1k Planner: 10k
>
> 'Now' is a cleaner signal that is a time point rather than a time
> duration. 'Parser' and 'Planner' also show a timeline about the
> important time point. At the same time, it cost very little coding
> effort based on patch 01. Different people may have different
> feelings about these words, I just hope I describe the goal clearly.
Parsing happens before planning and that memory is not measured by
this patch. May be separately but it's out of scope of this work.
"used" and "allocated" are MemoryContext terms indicating memory
actually used vs memory allocated.
Yes, I understand the terms come from MemoryContext, the complex
thing here is between time duration vs time point case. Since English
is not my native language, so I'm not too confident about insisting on this.
So I think we can keep it as it is.
>
> Personally I am pretty like patch 1, we just need to refactor some words
> to make everything clearer.
Thanks.
David challenged something at the begining, but IIUC he also agree the
value of patch 01 as the previous statement after discussion. Since the patch
is mild itself, so I will mark this commitfest entry as "Ready for committer".
Please correct me if anything is wrong.