On Mon, Jan 9, 2023 at 12:51 PM Robert Haas <robertmh...@gmail.com> wrote: > I feel that you should at least have a reproducer for these problems > posted to the thread, and ideally a regression test, before committing > things. I think it's very hard to understand what the problems are > right now.
Hard to understand relative to what, exactly? We're talking about a very subtle race condition here. I'll try to come up with a reproducer, but I *utterly* reject your assertion that it's a hard requirement, sight unseen. Why should those be the parameters of the discussion? For one thing I'm quite confident that I'm right, with or without a reproducer. And my argument isn't all that hard to follow, if you have relevant expertise, and actually take the time. But even this is unlikely to matter much. Even if I somehow turn out to have been completely wrong about the race condition, it is still self-evident that the problem of uselessly WAL logging non-changes to the VM exists. That doesn't require any concurrent access at all. It's a natural consequence of calling visibilitymap_set() with VISIBILITYMAP_ALL_FROZEN-only flags. You need only look at the code for 2 minutes to see it. > I don't particularly have a problem with the idea of 0001, because if > we use InvalidTransactionId to mean that there cannot be any > conflicts, we do not need FrozenTransactionId to mean the same thing. > Picking one or the other makes sense. We've already picked one, many years ago -- InvalidTransactionId. This is a long established convention, common to all REDO routines that are capable of creating granular conflicts. I already committed 0001 over a week ago. We were calling ResolveRecoveryConflictWithSnapshot with FrozenTransactionId arguments before now, which was 100% guaranteed to be a waste of cycles. I saw no need to wait more than a few days for a +1, given that this particular issue was so completely clear cut. -- Peter Geoghegan