Hi, 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. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
