On Sun, 1 Sept 2024 at 13:32, Michael Harris <harmic@gmail.com> wrote:
> v3 of the patch is attached, and I will submit it to the commitfest.
I think this patch is pretty good.
I only have a few things I'd point out:
1. The change to ddl.sgml, technically you're removing the caveat, but
I think the choice you've made to mention the updated behaviour is
likely a good idea still. So I agree with this change, but just wanted
to mention it as someone else might think otherwise.
2. I think there's some mixing of tense in the changes to analyze.sgml:
"If <literal>ONLY</literal> was not specified, it will also recurse
into each partition and update its statistics."
You've written "was" (past tense), but then the existing text uses
"will" (future tense). I guess if the point in time is after parse and
before work has been done, then that's correct, but I think using "is"
instead of "was" is better.
3. In vacuum.sgml;
"If <literal>ONLY</literal> is not specified, the table and all its
descendant tables or partitions (if any) are vacuumed"
Maybe "are also vacuumed" instead of "are vacuumed" is more clear?
It's certainly true for inheritance parents, but I guess you could
argue that's wrong for partitioned tables.
4. A very minor detail, but I think in vacuum.c the WARNING you've
added should use RelationGetRelationName(). We seem to be very
inconsistent with using that macro and I see it's not used just above
for the lock warning, which I imagine you copied.
Aside from those, that just leaves me with the behavioural change. I
noted Tom was ok with the change in behaviour for ANALYZE (mentioned
in [1]). Tom, wondering if you feel the same for VACUUM too? If we're
doing this, I think we'd need to be quite clear about it on the
release notes.
David
[1] https://postgr.es/m/1919201.1724289136@sss.pgh.pa.us
