> 12 июля 2018 г., в 20:40, Heikki Linnakangas <hlinn...@iki.fi> написал(а):
> On 12/07/18 19:06, Andrey Borodin wrote:
>>> 11 июля 2018 г., в 0:07, Heikki Linnakangas <hlinn...@iki.fi>
>>> написал(а):
>>> This seems misplaced. This code deals with internal pages, and as
>>> far as I can see, this patch never marks internal pages as deleted,
>>> only leaf pages. However, we should have something like this in the
>>> leaf-page branch, to deal with the case that an insertion lands on
>>> a page that was concurrently deleted. Did you have any tests, where
>>> an insertion runs concurrently with vacuum, that would exercise
>>> this?
>> That bug could manifest only in case of crash between removing
>> downlinks and marking pages deleted.
> Hmm. The downlink is removed first, so I don't think you can see that 
> situation after a crash. After a crash, you might have some empty, orphaned, 
> pages that have already been unlinked from the parent, but a search/insert 
> should never encounter them.
> Actually, now that I think about it more, I'm not happy with leaving orphaned 
> pages like that behind. Let's WAL-log the removal of the downlink, and 
> marking the leaf pages as deleted, in one WAL record, to avoid that.

OK, will do this. But this will complicate WAL replay seriously, and I do not 
know a proper way to test that (BTW there is GiST amcheck in progress, but I 
decided to leave it for a while).

> But the situation in gistdoinsert(), where you encounter a deleted leaf page, 
> could happen during normal operation, if vacuum runs concurrently with an 
> insert. Insertion locks only one page at a time, as it descends the tree, so 
> after it has released the lock on the parent, but before it has locked the 
> child, vacuum might have deleted the page. In the latest patch, you're 
> checking for that just before swapping the shared lock for an exclusive one, 
> but I think that's wrong; you need to check for that after swapping the lock, 
> because otherwise vacuum might delete the page while you're not holding the 
> lock.
Looks like a valid concern, I'll move that code again.
>> I do not know how to test this
>> reliably. Internal pages are locked before leafs and locks are
>> coupled. No cuncurrent backend can see downlinks to pages being
>> deleted, unless crash happens.
> Are you sure? At a quick glance, I don't think the locks are coupled.

Sorry for overquoting

+       /* rescan inner pages that had empty child pages */
+       foreach(cell,rescanList)
+       {
+               Buffer          buffer;
+               Page            page;
+               OffsetNumber i,
+                                       maxoff;
+               IndexTuple      idxtuple;
+               ItemId          iid;
+               OffsetNumber todelete[MaxOffsetNumber];
+               Buffer          buftodelete[MaxOffsetNumber];
+               int                     ntodelete = 0;
+               buffer = ReadBufferExtended(rel, MAIN_FORKNUM, 
RBM_NORMAL, info->strategy);
+               LockBuffer(buffer, GIST_EXCLUSIVE);
Here's first lock
+               gistcheckpage(rel, buffer);
+               page = (Page) BufferGetPage(buffer);
+               Assert(!GistPageIsLeaf(page));
+               maxoff = PageGetMaxOffsetNumber(page);
+               for (i = OffsetNumberNext(FirstOffsetNumber); i <= maxoff; i = 
+               {
+                       Buffer          leafBuffer;
+                       Page            leafPage;
+                       iid = PageGetItemId(page, i);
+                       idxtuple = (IndexTuple) PageGetItem(page, iid);
+                       leafBuffer = ReadBufferExtended(rel, MAIN_FORKNUM, 
+                                                               RBM_NORMAL, 
+                       LockBuffer(leafBuffer, GIST_EXCLUSIVE);
now locks are coupled in a top-down descent

> We do need some way of testing this..

Can we test replication of concurrent VACUUM and inserts in existing test suit? 
I just do not know.
I can do this tests manually if this is enough.

Best regards, Andrey Borodin.

Reply via email to