This is an automated email from the ASF dual-hosted git repository. mblow pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/asterixdb.git
commit d4d7e610a27a9098fda9064bdb58a00170a8af3d Author: Michael Blow <[email protected]> AuthorDate: Mon Feb 26 19:19:42 2024 -0500 [NO ISSUE][HYR][STO] BufferCache lock fixes Change-Id: I37f06163bbf1c34392d83a8ccd27e777552eeac7 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/18181 Reviewed-by: Michael Blow <[email protected]> Tested-by: Michael Blow <[email protected]> --- .../hyracks/storage/am/btree/impls/BTree.java | 48 ++++++++-------------- .../am/btree/impls/BTreeCountingSearchCursor.java | 22 ++++++---- .../am/btree/impls/BTreeRangeSearchCursor.java | 9 +++- .../hyracks/storage/am/btree/impls/DiskBTree.java | 11 +---- .../common/freepage/VirtualFreePageManager.java | 11 +++-- 5 files changed, 47 insertions(+), 54 deletions(-) diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTree.java b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTree.java index 78faaff336..e085b5e4fb 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTree.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTree.java @@ -105,7 +105,7 @@ public class BTree extends AbstractTreeIndex { RangePredicate diskOrderScanPred = new RangePredicate(null, null, true, true, ctx.getCmp(), ctx.getCmp()); int maxPageId = freePageManager.getMaxPageId(ctx.getMetaFrame()); int currentPageId = bulkloadLeafStart; - ICachedPage page = bufferCache.pin(BufferedFileHandle.getDiskPageId(getFileId(), currentPageId), false); + final ICachedPage page = bufferCache.pin(BufferedFileHandle.getDiskPageId(getFileId(), currentPageId), false); page.acquireReadLatch(); try { cursor.setBufferCache(bufferCache); @@ -116,7 +116,7 @@ public class BTree extends AbstractTreeIndex { ctx.getCursorInitialState().setSearchOperationCallback(ctx.getSearchCallback()); ctx.getCursorInitialState().setOriginialKeyComparator(ctx.getCmp()); cursor.open(ctx.getCursorInitialState(), diskOrderScanPred); - } catch (Exception e) { + } catch (Throwable e) { page.releaseReadLatch(); bufferCache.unpin(page); throw HyracksDataException.create(e); @@ -202,8 +202,7 @@ public class BTree extends AbstractTreeIndex { } // we use this loop to deal with possibly multiple operation restarts // due to ongoing structure modifications during the descent - boolean repeatOp = true; - while (repeatOp && ctx.getOpRestarts() < MAX_RESTARTS) { + while (true) { performOp(rootPage, null, true, ctx); // if we reach this stage then we need to restart from the (possibly // new) root @@ -211,7 +210,7 @@ public class BTree extends AbstractTreeIndex { ctx.getPageLsns().removeLast(); // pop the restart op indicator continue; } - repeatOp = false; + break; } cursor.setBufferCache(bufferCache); cursor.setFileId(getFileId()); @@ -231,12 +230,8 @@ public class BTree extends AbstractTreeIndex { bufferCache.unpin(smPage); } } - if (ctx.getSmPages().size() > 0) { - if (ctx.getSmoCount() == Integer.MAX_VALUE) { - smoCounter.set(0); - } else { - smoCounter.incrementAndGet(); - } + if (!ctx.getSmPages().isEmpty()) { + smoCounter.updateAndGet(i -> i == Integer.MAX_VALUE ? 0 : i + 1); treeLatch.writeLock().unlock(); ctx.getSmPages().clear(); } @@ -599,8 +594,7 @@ public class BTree extends AbstractTreeIndex { // We use this loop to deal with possibly multiple operation // restarts due to ongoing structure modifications during // the descent. - boolean repeatOp = true; - while (repeatOp && ctx.getOpRestarts() < MAX_RESTARTS) { + while (true) { int childPageId = ctx.getInteriorFrame().getChildPageId(ctx.getPred()); performOp(childPageId, node, isReadLatched, ctx); node = null; @@ -615,6 +609,10 @@ public class BTree extends AbstractTreeIndex { if (node != null) { isReadLatched = true; // Descend the tree again. + if (ctx.getOpRestarts() >= MAX_RESTARTS) { + throw HyracksDataException.create(ErrorCode.OPERATION_EXCEEDED_MAX_RESTARTS, + MAX_RESTARTS); + } continue; } else { // Pop pageLsn of this page (version seen by this op during descent). @@ -662,7 +660,7 @@ public class BTree extends AbstractTreeIndex { } } // Operation completed. - repeatOp = false; + break; } // end while } else { // smFlag ctx.setOpRestarts(ctx.getOpRestarts() + 1); @@ -734,21 +732,8 @@ public class BTree extends AbstractTreeIndex { ctx.getPageLsns().add(FULL_RESTART_OP); } } - } catch (HyracksDataException e) { - if (!ctx.isExceptionHandled()) { - if (node != null) { - if (isReadLatched) { - node.releaseReadLatch(); - } else { - node.releaseWriteLatch(true); - } - bufferCache.unpin(node); - ctx.setExceptionHandled(true); - } - } - throw e; - } catch (Exception e) { - if (node != null) { + } catch (Throwable e) { + if (!ctx.isExceptionHandled() && node != null) { if (isReadLatched) { node.releaseReadLatch(); } else { @@ -756,9 +741,8 @@ public class BTree extends AbstractTreeIndex { } bufferCache.unpin(node); } - HyracksDataException wrappedException = HyracksDataException.create(e); ctx.setExceptionHandled(true); - throw wrappedException; + throw HyracksDataException.create(e); } } @@ -785,7 +769,7 @@ public class BTree extends AbstractTreeIndex { ICachedPage node = bufferCache.pin(BufferedFileHandle.getDiskPageId(getFileId(), pageId), false); node.acquireReadLatch(); try { - if (parent != null && unpin == true) { + if (parent != null && unpin) { parent.releaseReadLatch(); bufferCache.unpin(parent); } diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeCountingSearchCursor.java b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeCountingSearchCursor.java index 149142401b..71df593056 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeCountingSearchCursor.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeCountingSearchCursor.java @@ -119,13 +119,21 @@ public class BTreeCountingSearchCursor extends EnforcedIndexCursor implements IT private void fetchNextLeafPage(int nextLeafPage) throws HyracksDataException { do { - ICachedPage nextLeaf = bufferCache.pin(BufferedFileHandle.getDiskPageId(fileId, nextLeafPage), false); - if (exclusiveLatchNodes) { - nextLeaf.acquireWriteLatch(); - page.releaseWriteLatch(isPageDirty); - } else { - nextLeaf.acquireReadLatch(); - page.releaseReadLatch(); + final ICachedPage nextLeaf; + try { + nextLeaf = bufferCache.pin(BufferedFileHandle.getDiskPageId(fileId, nextLeafPage), false); + if (exclusiveLatchNodes) { + nextLeaf.acquireWriteLatch(); + } else { + nextLeaf.acquireReadLatch(); + } + } finally { + // release latches in finally, don't leak locks on pin failure + if (exclusiveLatchNodes) { + page.releaseWriteLatch(isPageDirty); + } else { + page.releaseReadLatch(); + } } bufferCache.unpin(page); page = nextLeaf; diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeRangeSearchCursor.java b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeRangeSearchCursor.java index 9ccd881ec0..cf3727a754 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeRangeSearchCursor.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeRangeSearchCursor.java @@ -110,8 +110,13 @@ public class BTreeRangeSearchCursor extends EnforcedIndexCursor implements ITree protected void fetchNextLeafPage(int nextLeafPage) throws HyracksDataException { do { - ICachedPage nextLeaf = acquirePage(nextLeafPage); - releasePage(); + ICachedPage nextLeaf; + try { + nextLeaf = acquirePage(nextLeafPage); + } finally { + // release page in finally, don't leak lock on pin failure + releasePage(); + } page = nextLeaf; isPageDirty = false; frame.setPage(page); diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/DiskBTree.java b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/DiskBTree.java index ae6bbaae97..282aad9511 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/DiskBTree.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/DiskBTree.java @@ -143,19 +143,12 @@ public class DiskBTree extends BTree { ctx.getCursorInitialState().setPageId(childPageId); ctx.getLeafFrame().setPage(currentPage); cursor.open(ctx.getCursorInitialState(), ctx.getPred()); - } catch (HyracksDataException e) { - if (!ctx.isExceptionHandled() && currentPage != null) { - bufferCache.unpin(currentPage); - ctx.setExceptionHandled(true); - } - throw e; } catch (Exception e) { - if (currentPage != null) { + if (!ctx.isExceptionHandled() && currentPage != null) { bufferCache.unpin(currentPage); } - HyracksDataException wrappedException = HyracksDataException.create(e); ctx.setExceptionHandled(true); - throw wrappedException; + throw HyracksDataException.create(e); } } diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/freepage/VirtualFreePageManager.java b/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/freepage/VirtualFreePageManager.java index 9d62c0db0b..0e4f835c89 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/freepage/VirtualFreePageManager.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/freepage/VirtualFreePageManager.java @@ -104,10 +104,13 @@ public class VirtualFreePageManager implements IPageManager { page = bufferCache.pin(BufferedFileHandle.getDiskPageId(fileId, currentPageId.get()), true); if (leafFrameFactory != null) { page.acquireWriteLatch(); - ITreeIndexFrame leafFrame = leafFrameFactory.createFrame(); - leafFrame.setPage(page); - leafFrame.initBuffer((byte) 0); - page.releaseWriteLatch(true); + try { + ITreeIndexFrame leafFrame = leafFrameFactory.createFrame(); + leafFrame.setPage(page); + leafFrame.initBuffer((byte) 0); + } finally { + page.releaseWriteLatch(true); + } } bufferCache.unpin(page); }
