On Fri, 20 Sept 2024 at 13:20, Michael Harris <har...@gmail.com> wrote: > I have attached a new version of the patch with this feedback incorporated.
I looked over the v6 patch and I don't have any complaints. However, I
did make some minor adjustments:
* A few places said things like "and possibly partitions and/or child
tables thereof". It's not possible for a partition to have inheritance
children, so "and/" is wrong there, only "or" is possible.
* I spent a bit of time trying to massage the new text into the
documents. I just felt that sometimes the new text was a little bit
clumsy, for example:
<command>ANALYZE</command> gathers two sets of statistics: one on the rows
of the parent table only, and a second including rows of both the parent
table and all of its children. This second set of statistics is
needed when
- planning queries that process the inheritance tree as a whole. The child
- tables themselves are not individually analyzed in this case.
- The autovacuum daemon, however, will only consider inserts or
- updates on the parent table itself when deciding whether to trigger an
- automatic analyze for that table. If that table is rarely inserted into
- or updated, the inheritance statistics will not be up to date unless you
- run <command>ANALYZE</command> manually.
+ planning queries that process the inheritance tree as a whole. If the
+ <literal>ONLY</literal> keyword is used, child tables themselves are not
+ individually analyzed. The autovacuum daemon, however, will only consider
+ inserts or updates on the parent table itself when deciding whether to
+ trigger an automatic analyze for that table. If that table is rarely
+ inserted into or updated, the inheritance statistics will not be up to
+ date unless you run <command>ANALYZE</command> manually.
I kinda blame the existing text which reads "The child tables
themselves are not individually analyzed in this case." as that seems
to have led you to detail the new behaviour in the same place, but I
think that we really need to finish talking about autovacuum maybe not
analyzing the inheritance parent unless it receives enough changes
itself before we talk about what ONLY does.
* Very minor adjustments to the tests to upper case all the keywords
"is not null as" was all lowercase. I wrapped some longer lines too.
Also, I made some comment adjustments and I dropped the partitioned
table directly after we're done with it instead of waiting until after
the inheritance parent tests.
* A bunch of uses of "descendant tables" when you meant "inheritance children"
v7-0001 is the same as your v6 patch, but I adjusted the commit
message, which I'm happy to take feedback on. Nobody I've spoken to
seems to be concerned about VACUUM on inheritance parents now being
recursive by default. I included "release notes" and "incompatibility"
in the commit message in the hope that Bruce will stumble upon it and
write about this when he does the release notes for v18.
v7-0002 is all my changes.
I'd like to push this soon, so if anyone has any last-minute feedback,
please let me know in the next few days.
David
v7-0001-Add-ONLY-support-for-VACUUM-and-ANALYZE.patch
Description: Binary data
v7-0002-fixup-Implementation-of-the-ONLY-feature-for-ANAL.patch
Description: Binary data
