On Fri, 2025-05-23 at 15:18 +0200, Erik Nordström wrote:
> I am wondering if the fix shouldn't be outside heap's copy_for_cluster
> implementation?
> I guess it depends on the semantics of num_tuples, but the cluster code seems
> to
> allude to interpreting num_tuples as the number of non-removable tuples.
> If you subtract recently dead from that number within the heap implementation,
> then it will no longer reflect non-removable tuples and the log message in the
> cluster function "found %.0f removable, %.0f nonremovable row versions" will
> no
> longer be correct.
>
> Surprisingly, tableam.h does not document the num_tuples parameter in the
> table_relation_copy_for_cluster() function although all other output
> parameters
> are documented. So, it is not clear what the intended semantics are. Maybe
> other hackers on the mailing list have opinions on how to interpret
> num_tuples?
I don't know how it was intended, but the lack of comments indicates to me that
perhaps nobody thought about it too hard, and your interpretation is fine.
> In any case, assuming num_tuples is supposed to return non-removable tuples,
> then the fix should be to subtract recently dead tuples when updating
> pg_class.reltuples. Other TAM's need to treat num_tuples as non-removable
> tuples as well, and update recently dead if applicable.
>
> I am attaching a patch with these changes, while also including the isolation
> test in that patch.
The patch applies fine and passes regression tests, and I see nothing wrong
with your approach.
There is one nit: "git am" complained about trailing whitespace in this line:
--- /dev/null
+++ b/src/test/isolation/specs/vacuum-full-stats.spec
[...]
+step s2_update { UPDATE stats SET a = 1 WHERE a > 6; }
Yours,
Laurenz Albe
