On Fri, Jul 11, 2025 at 7:42 PM Mircea Cadariu <cadariu.mir...@gmail.com> wrote:
>
> On 11/07/2025 10:51, Laurenz Albe wrote:
>
> > Good idea; done in the attached version 2 of the patch.
>
> Thanks! Looks good. I have set the status of the Commitfest entry to
> "Ready for Committer".
I've started reviewing the patch since it's marked as ready for committer.
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?
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).
------------------
+ /*
+ * 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.
+ 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.
Regards,
--
Fujii Masao
