On Wed, Nov 13, 2019 at 7:27 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> On Sun, Nov 10, 2019 at 8:21 PM Thomas Munro <thomas.munro@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.