Hi,
I just saw this got committed and wanted to briefly play with it. It works
nicely!
Except that at first I tried this in a debugging build, and was briefly rather
dismayed by the performance. It was really slow. But it's not really related
to repack / the patches here.
Turns out that it is to a good part due to
heap_insert()
->CacheInvalidateHeapTuple()
->CacheInvalidateHeapTupleCommon()
->AssertCouldGetRelation()
not being cheap and running a *lot*.
Admittedly it's way worse if you build with -O0, which I tend to do to make
debugging easier.
In that config, the assert single-handled increases the time for a repack by
35% or so.
Noah, is there any reason we need to do the AssertCouldGetRelation() before
the !IsCatalogRelation(relation)? Given that the goal is to make
RelationGetRelid() safe, it doesn't seem there is?
It's totally valid to not have done so initially, this is a quite complicated
feature:
I saw this is using individual heap_insert()s during the
heapam_relation_copy_for_cluster(). Doing individual WAL logged inserts isn't
exactly cheap or efficient from a WAL volume perspective...
Is there anything other than round tuits preventing us from using
multi_insert?
That actually would also reduce the cost in the REPACK decoding worker, due to
having to parse far fewer WAL records.
Greetings,
Andres Freund
