Content-Type: multipart/alternative;
 boundary="------------9KLaFY4bPPBBBlzeaAIO0C6d"
Message-ID: <fbf77241-e4c5-4f70-ac71-8bf8cff89f39@tantorlabs.com>
Date: Thu, 20 Mar 2025 15:04:43 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add estimated hit ratio to Memoize in EXPLAIN to explain cost
 adjustment
To: David Rowley <dgrowleyml@gmail.com>
Cc: Lukas Fittl <lukas@fittl.com>, Daniel Gustafsson <daniel@yesql.se>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CAP53Pky29GWAVVk3oBgKBDqhND0BRBN6yTPeguV_qSivFL5N_g@mail.gmail.com>
 <CAApHDvrBctjUFjY77BFd_MqPD+yf4nOZSKzbdzP70XntkknY8A@mail.gmail.com>
 <51A15CD7-E31B-483D-B911-D0EB9F5FF952@yesql.se>
 <CAP53Pkz-sK8En2c05rJDcV7pz8+OsbNqOnm_SO4tsuwm=n=3OQ@mail.gmail.com>
 <bea2a2ae-3a95-4740-8926-837bacc7bd4e@tantorlabs.com>
 <CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com>
Content-Language: en-US
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
In-Reply-To: 
 <CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/fbf77241-e4c5-4f70-ac71-8bf8cff89f39%40tantorlabs.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------9KLaFY4bPPBBBlzeaAIO0C6d
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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.

--------------9KLaFY4bPPBBBlzeaAIO0C6d
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 20.03.2025 13:37, David Rowley
      wrote:<span style="white-space: pre-wrap">
</span></div>
    <blockquote type="cite"
cite="mid:CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">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.</pre>
    </blockquote>
    <p><br>
    </p>
    <p>LGTM<br>
    </p>
    <p><span style="white-space: pre-wrap">
</span></p>
    <blockquote type="cite"
cite="mid:CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">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?</pre>
    </blockquote>
    <p><br>
    </p>
    <p>I fully agree with this suggestion. Then I'll begin with this on
      another new thread.<br>
    </p>
    <p><span style="white-space: pre-wrap">
</span></p>
    <blockquote type="cite"
cite="mid:CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">+ 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.</pre>
    </blockquote>
    <p><br>
    </p>
    <p>LGTM<br>
    </p>
    <p><span style="white-space: pre-wrap">
</span><span style="white-space: pre-wrap">
</span></p>
    <blockquote type="cite"
cite="mid:CAApHDvpq2gOt3gnzkd4Ud=wrT7YRGrF3zb29jgWzDsYEhETrOA@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">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.</pre>
    </blockquote>
    <p><br>
    </p>
    <p>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!</p>
    <p>--<br>
      Best regards,<br>
      Ilia Evdokimov,<br>
      Tantor Labs LLC.<br>
    </p>
  </body>
</html>

--------------9KLaFY4bPPBBBlzeaAIO0C6d--