On Wed, Nov 13, 2019 at 7:27 PM Ashwin Agrawal <aagra...@pivotal.io> wrote: > On Sun, Nov 10, 2019 at 8:21 PM Thomas Munro <thomas.mu...@gmail.com> wrote: >> but on another read-through of the main patch >> I didn't like the comments for CheckForSerializableConflictOut() or >> the fact that it checks SerializationNeededForRead() again, after I >> thought a bit about what the contract for this API is now. Here's a >> version with small fixup that I'd like to squash into the patch. >> Please let me know what you think, > > The thought or reasoning behind having SerializationNeededForRead() > inside CheckForSerializableConflictOut() is to keep that API clean and > complete by itself. Only if AM like heap needs to perform some AM > specific checking only for serialization needed case can do so but is > not forced. So, if AM for example Zedstore doesn't need to do any > specific checking, then it can directly call > CheckForSerializableConflictOut(). With the modified fixup patch, the > responsibility is unnecessarily forced to caller even if > CheckForSerializableConflictOut() can do it. I understand the intent > is to avoid duplicate check for heap.
OK, I kept only the small comment change from that little fixup patch, and pushed this. > I had proposed as alternative way in initial email and also later, > didn't receive comment on that, so re-posting. > typedef bool (*AMCheckForSerializableConflictOutCallback) (void *arg); ... > Just due to void* callback argument aspect I didn't prefer that > solution and felt AM performing checks and calling > CheckForSerializableConflictOut() seems better. Please let me know > how you feel about this. Yeah. We could always come back to this idea if it looks better once we have more experience with new table AMs.
