> On 29 Sep 2017, at 00:59, Alexander Korotkov <a.korot...@postgrespro.ru> > wrote: > > On Thu, Sep 28, 2017 at 2:22 PM, Alexander Korotkov > <a.korot...@postgrespro.ru <mailto:a.korot...@postgrespro.ru>> wrote: > On Fri, Jul 28, 2017 at 7:58 AM, Shubham Barai <shubhambara...@gmail.com > <mailto:shubhambara...@gmail.com>> wrote: > I am attaching a patch for predicate locking in SP-Gist index > > I took a look over this patch. > > newLeafBuffer = SpGistGetBuffer(index, > > GBUF_LEAF | (isNulls ? GBUF_NULLS : 0), > > Min(totalLeafSizes, > > SPGIST_PAGE_CAPACITY), > > &xlrec.initDest); > PredicateLockPageSplit(index, > BufferGetBlockNumber(current->buffer), > BufferGetBlockNumber(newLeafBuffer)); > > You move predicate lock during split only when new leaf page is allocated. > However SP-GiST may move items to the free space of another busy page during > split (see other branches in doPickSplit()). Your patch doesn't seem to > handle this case correctly. > > I've more thoughts about this patch. > > + * SPGist searches acquire predicate lock only on the leaf pages. > + During a page split, a predicate lock is copied from the original > + page to the new page. > > This approach isn't going to work. When new tuple is inserted into SP-GiST, > choose method can return spgAddNode. In this case new branch of the tree is > added. And this new branch could probably be used by scans we made in the > past. Therefore, you need to do predicate locking for internal pages too in > order to detect all the possible conflicts.
Based on this review, I’ve marked this patch Returned with feedback. Please re-submit a new version to an upcoming commitfest when ready. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
