Thanks for sharing your thoughts. Please find my comments inline below:
I think here we should report that we haven't done what was asked. + /* Nothing to do if the itemid is unused or already dead. */ + if (!ItemIdIsUsed(itemid) || ItemIdIsDead(itemid)) + continue;
Okay. Will add a log message saying "skipping tid ... because ..."
Also, should we try to fix VM along the way?
I think we should let VACUUM do that.
Are there any caveats with concurrent VACUUM? (I do not see any, just asking)
As of now, I don't see any.
It would be good to have some checks for interrupts in safe places.
I think I have already added those wherever I felt it was required. If you feel it's missing somewhere, it could be good if you could point it out.
I think we should not trust user entierly here. I'd prefer validation and graceful exit, not a core dump. + Assert(noffs <= PageGetMaxOffsetNumber(page));
Yeah, sounds reasonable. Will do that.
For some reason we had unlogged versions of these functions. But I do not recall exact rationale.. Also, I'd be happy if we had something like "Restore this tuple iff this does not break unique constraint". To do so we need to sort tids by xmin\xmax, to revive most recent data first.
I didn't get this point. Could you please elaborate.