Content-Type: multipart/alternative;
 boundary="------------jOfq9FkU83D0jQ9rf1S8zx0G"
Message-ID: <9c9cb744-b8c7-4d75-acad-595b3faba187@gmail.com>
Date: Wed, 6 Aug 2025 14:01:18 +1000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: analyze-in-stages post upgrade questions
Content-Language: en-GB
To: Fujii Masao <masao.fujii@gmail.com>
Cc: Laurenz Albe <laurenz.albe@cybertec.at>,
 "Zechman, Derek S" <Derek.S.Zechman@snapon.com>,
 Adrian Klaver <adrian.klaver@aklaver.com>, pgsql-hackers@lists.postgresql.org
References: <CO1PR04MB8281387B9AD9DE30976966BBC045A@CO1PR04MB8281.namprd04.prod.outlook.com>
 <6add2a9a-7cf2-4d1b-8f3e-2e26a7ebe883@aklaver.com>
 <4d8122febd3007143504e4b6034b4253f7000761.camel@cybertec.at>
 <PH0PR04MB82943D67CD179002714AA503C044A@PH0PR04MB8294.namprd04.prod.outlook.com>
 <f2ebeef82fddaf8a8152383f8ff62e75f20aac3c.camel@cybertec.at>
 <PH0PR04MB8294E796D6297D4E3BE4EFB6C049A@PH0PR04MB8294.namprd04.prod.outlook.com>
 <08d943a83590308cbb9be594d80b4e19ca80e08e.camel@cybertec.at>
 <2505eef1-b6aa-4518-baff-861a2e6a507b@gmail.com>
 <b65126bf381d4b41f561afe691000b77ac7b8cc2.camel@cybertec.at>
 <a018a729-9b18-444f-baed-be0d6d8ecb80@gmail.com>
 <2c1f1834107045dfa8b32417771b56bab0cffc56.camel@cybertec.at>
 <2cce9851-327a-4b1a-ab8e-531c2f92532b@gmail.com>
 <CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com>
From: Mircea Cadariu <cadariu.mircea@gmail.com>
In-Reply-To: <CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/9c9cb744-b8c7-4d75-acad-595b3faba187%40gmail.com>
Precedence: bulk

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

Hi,

On 30/07/2025 12:49, Fujii Masao wrote:
> I've started reviewing the patch since it's marked as ready for committer.
Thanks!
> Overall, I like the change. But I have one question: should this be treated as
> a bug fix that we back-patch to supported branches, or is it more of
> an improvement that should only go into master?
I reckon it might make sense to back-patch it to previous versions, as 
users might not upgrade always to the latest version.
>           Only calculate statistics for use by the optimizer (no vacuum).
> +        If that option is specified, <command>vacuumdb</command> will also
> +        process partitioned tables.  Without that option, only the partitions
> +        will be considered, unless a partitioned table is explicitly specified
> +        with the <option>--table</option> option.
>
> This wording seems a bit out of place in the --analyze-only section,
> since it also describes the default behavior of vacuumdb without that option.
> Wouldn't it make more sense to move that explanation in the --table section?
>
> For example, we could add something like:
>
> ------------------
> If no tables are specified with the --table option, vacuumdb will
> clean all regular tables and materialized views in the connected
> database. If --analyze-only or --analyze-in-stages is also specified,
> it will analyze all regular tables, partitioned tables, and
> materialized views (but not foreign tables).
> ------------------
Yes, agreed.
> + /*
> + * VACUUMing partitioned tables would be unreasonably expensive, since
> + * that entails processing the partitions twice (once as part of the
> + * partitioned table, once as tables in their own right) for no
> + * benefit. But if we only ANALYZE, collecting statistics for
> + * partitioned tables is worth the effort.
> + */
>
> This is probably true. But isn't the main reason more about aligning with
> the behavior of the underlying VACUUM and ANALYZE commands? As the vacuumdb
> docs says, "There is no effective difference between vacuuming and analyzing
> databases via this utility and via other methods for accessing the server.",
> so its default target objects should match: VACUUM skips partitioned tables
> by default, while ANALYZE includes them. If that's the case, maybe the comment
> should reflect that instead.
I see what you mean. From that perspective, I wonder if we even need a 
comment there at all.
> + qr/statement:\s+ANALYZE\s+public\.parent_table/s,
> + '--analyze_only updates statistics for partitioned tables');
>
> A plain space might be sufficient instead of \s+.
> Also, I don't think the backslash before ".parent_table" is necessary.

Good catch! Indeed let's simplify that to contain strictly only what's 
necessary.

Kind regards,

Mircea Cadariu

--------------jOfq9FkU83D0jQ9rf1S8zx0G
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi,</p>
    <div class="moz-cite-prefix">On 30/07/2025 12:49, Fujii Masao wrote:<span
      style="white-space: pre-wrap">
</span></div>
    <blockquote type="cite"
cite="mid:CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">I've started reviewing the patch since it's marked as ready for committer.</pre>
    </blockquote>
    Thanks! 
    <blockquote type="cite"
cite="mid:CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">Overall, I like the change. But I have one question: should this be treated as
a bug fix that we back-patch to supported branches, or is it more of
an improvement that should only go into master?</pre>
    </blockquote>
    I reckon it might make sense to back-patch it to previous versions,
    as users might not upgrade always to the latest version.
    <blockquote type="cite"
cite="mid:CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">         Only calculate statistics for use by the optimizer (no vacuum).
+        If that option is specified, &lt;command&gt;vacuumdb&lt;/command&gt; will also
+        process partitioned tables.  Without that option, only the partitions
+        will be considered, unless a partitioned table is explicitly specified
+        with the &lt;option&gt;--table&lt;/option&gt; option.

This wording seems a bit out of place in the --analyze-only section,
since it also describes the default behavior of vacuumdb without that option.
Wouldn't it make more sense to move that explanation in the --table section?

For example, we could add something like:

------------------
If no tables are specified with the --table option, vacuumdb will
clean all regular tables and materialized views in the connected
database. If --analyze-only or --analyze-in-stages is also specified,
it will analyze all regular tables, partitioned tables, and
materialized views (but not foreign tables).
------------------</pre>
    </blockquote>
    Yes, agreed. 
    <blockquote type="cite"
cite="mid:CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">+ /*
+ * VACUUMing partitioned tables would be unreasonably expensive, since
+ * that entails processing the partitions twice (once as part of the
+ * partitioned table, once as tables in their own right) for no
+ * benefit. But if we only ANALYZE, collecting statistics for
+ * partitioned tables is worth the effort.
+ */

This is probably true. But isn't the main reason more about aligning with
the behavior of the underlying VACUUM and ANALYZE commands? As the vacuumdb
docs says, "There is no effective difference between vacuuming and analyzing
databases via this utility and via other methods for accessing the server.",
so its default target objects should match: VACUUM skips partitioned tables
by default, while ANALYZE includes them. If that's the case, maybe the comment
should reflect that instead.
</pre>
    </blockquote>
    I see what you mean. From that perspective, I wonder if we even need
    a comment there at all. 
    <blockquote type="cite"
cite="mid:CAHGQGwEvwpydO7hjnB51Q2zgS0ydv31X4HCFLNzMeqLr=poS8w@mail.gmail.com">
      <pre wrap="" class="moz-quote-pre">+ qr/statement:\s+ANALYZE\s+public\.parent_table/s,
+ '--analyze_only updates statistics for partitioned tables');

A plain space might be sufficient instead of \s+.
Also, I don't think the backslash before ".parent_table" is necessary.</pre>
    </blockquote>
    <p>Good catch! Indeed let's simplify that to contain strictly only
      what's necessary.  </p>
    <p>Kind regards,</p>
    <p>Mircea Cadariu</p>
  </body>
</html>

--------------jOfq9FkU83D0jQ9rf1S8zx0G--