On Mon, Mar 27, 2023 at 10:17 AM Robert Haas <robertmh...@gmail.com> wrote: > > What about aborted speculative insertions? See > > heap_abort_speculative(), which directly sets the speculatively > > inserted heap tuple's xmin to InvalidTransactionId/zero. > > Oh, dear. I didn't know about that case.
A big benefit of having extensive amcheck coverage is that it effectively centralizes information about the on-disk format, in an easy to understand way, and (over time) puts things on a more rigorous footing. Now it'll be a lot harder for somebody else to overlook that case in the future, which is good. Things are trending in the right direction. > > It probably does make sense to keep something close to this check -- > > it just needs to account for speculative insertions to avoid false > > positive reports of corruption. We could perform cross-checks against > > a tuple whose xmin is InvalidTransactionId/zero to verify that it > > really is from an aborted speculative insertion, to the extent that > > that's possible. For example, such a tuple can't be a heap-only tuple, > > and it can't have any xmax value other than InvalidTransactionId/zero. > > Since this was back-patched, I think it's probably better to just > remove the error. We can introduce new validation if we want, but that > should probably be master-only. That makes sense. I don't think that it's particularly likely that having refined aborted speculative insertion amcheck coverage will make a critical difference to any user, at any time. But "amcheck as documentation of the on-disk format" is reason enough to have it. -- Peter Geoghegan