> 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,
(BlockNumber)lfirst_int(cell),
+
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 =
OffsetNumberNext(i))
+ {
+ Buffer leafBuffer;
+ Page leafPage;
+
+ iid = PageGetItemId(page, i);
+ idxtuple = (IndexTuple) PageGetItem(page, iid);
+
+ leafBuffer = ReadBufferExtended(rel, MAIN_FORKNUM,
ItemPointerGetBlockNumber(&(idxtuple->t_tid)),
+ RBM_NORMAL,
info->strategy);
+ 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.
