public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ilia Evdokimov <[email protected]>
To: David Rowley <[email protected]>
Cc: Lukas Fittl <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add estimated hit ratio to Memoize in EXPLAIN to explain cost adjustment
Date: Thu, 20 Mar 2025 15:04:43 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com>
References: <CAP53Pky29GWAVVk3oBgKBDqhND0BRBN6yTPeguV_qSivFL5N_g@mail.gmail.com>
<CAApHDvrBctjUFjY77BFd_MqPD+yf4nOZSKzbdzP70XntkknY8A@mail.gmail.com>
<[email protected]>
<CAP53Pkz-sK8En2c05rJDcV7pz8+OsbNqOnm_SO4tsuwm=n=3OQ@mail.gmail.com>
<[email protected]>
<CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com>
On 20.03.2025 13:37, David Rowley wrote:
> est_entries is a uint32, so %u is the correct format character for that type.
>
> I don't think you need to prefix all these properties with "Cache"
> just because the other two properties have that prefix. I also don't
> think the names you've chosen really reflect the meaning. How about
> something like: "Estimated Distinct Lookup Keys: 721 Estimated
> Capacity: 655", in that order. I think maybe having that as one line
> for format=text is better than 2 lines. The EXPLAIN output is already
> often taking up more lines in v18 than in v17, would be good to not
> make that even worse unnecessarily.
LGTM
> I see the existing code there could use ExplainPropertyText rather
> than have a special case for text and non-text formats. That's likely
> my fault. If we're touching this code, then we should probably tidy
> that up. Do you want to create a precursor fixup patch for that?
I fully agree with this suggestion. Then I'll begin with this on another
new thread.
> + double ndistinct; /* Estimated number of distinct memoization keys,
> + * used for cache size evaluation. Kept for EXPLAIN */
>
> Maybe this field in MemoizePath needs a better name. How about
> "est_unique_keys"? and also do the rename in struct Memoize.
LGTM
> I'm also slightly concerned about making struct Memoize bigger. I had
> issues with a performance regression [1] for 908a96861 when increasing
> the WindowAgg struct size last year and the only way I found to make
> it go away was to shuffle the fields around so that the struct size
> didn't increase. I think we'll need to see a benchmark of a query that
> hits Memoize quite hard with a small cache size to see if the
> performance decreases as a result of adding the ndistinct field. It's
> unfortunate that we'll not have the luxury of squeezing this double
> into padding if we do see a slowdown.
I tried rearranging the fields in the structure with 'ndistinct' field,
but unfortunately, sizeof(Memoize) did not remain the same. Therefore,
benchmarking Memoize is definitely necessary. Thanks for the information!
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
view thread (39+ 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], [email protected], [email protected]
Subject: Re: Add estimated hit ratio to Memoize in EXPLAIN to explain cost adjustment
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