On Mon, Oct 2, 2017 at 8:00 PM, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote: > What happen if exactly this "continue" fires? > >> if (GistFollowRight(stack->page)) >> { >> if (!xlocked) >> { >> LockBuffer(stack->buffer, GIST_UNLOCK); >> LockBuffer(stack->buffer, GIST_EXCLUSIVE); >> xlocked = true; >> /* someone might've completed the split when we unlocked */ >> if (!GistFollowRight(stack->page)) >> continue; > > > In this case we might get xlocked == true without calling > CheckForSerializableConflictIn(). Indeed! I've overlooked it. I'm remembering this issue, we were considering not fixing splits if in Serializable isolation, but dropped the idea.
Yeah, current insert algorithm assumes that split must be fixed before we can correctly traverse the tree downwards.
CheckForSerializableConflictIn() must be after every exclusive lock.
I'm not sure, that fixing split is the case to necessary call CheckForSerializableConflictIn(). This lock on leaf page is not taken to do modification of the page. It's just taken to ensure that nobody else is fixing this split the same this. After fixing the split, it might appear that insert would go to another page.
> I think it would be rather safe and easy for understanding to more > CheckForSerializableConflictIn() directly before gistinserttuple(). The difference is that after lock we have conditions to change page, and before gistinserttuple() we have actual intent to change page.
From the point of future development first version is better (if some new calls occasionally spawn in), but it has 3 calls while your proposal have 2 calls. It seems to me that CheckForSerializableConflictIn() before gistinserttuple() is better as for now.