Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-5059 remove calls to E(...) when the return value is already being checked #5066

Merged
merged 2 commits into from
Jul 9, 2024
Merged

GH-5059 remove calls to E(...) when the return value is already being checked #5066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
935f515
GH-5059 remove calls to E(...) when the return value is already being…
hmottestad Jun 28, 2024
e5faa2c
GH-5059 improve error handling and logging
hmottestad Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbContextIdIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ public long[] next() {
Varint.writeUnsigned(minKeyBuf, record[0]);
minKeyBuf.flip();
keyData.mv_data(minKeyBuf);
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET));
if (lastResult != 0) {
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET);
if (lastResult != MDB_SUCCESS) {
// use MDB_SET_RANGE if key was deleted
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
}
if (lastResult != 0) {
if (lastResult != MDB_SUCCESS) {
closeInternal(false);
return null;
}
Expand All @@ -119,16 +119,16 @@ public long[] next() {
}

if (fetchNext) {
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
fetchNext = false;
} else {
if (minKeyBuf != null) {
// set cursor to min key
keyData.mv_data(minKeyBuf);
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
} else {
// set cursor to first item
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
}
}

Expand Down
18 changes: 8 additions & 10 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbRecordIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,12 @@ public long[] next() {
index.toKey(minKeyBuf, quad[0], quad[1], quad[2], quad[3]);
minKeyBuf.flip();
keyData.mv_data(minKeyBuf);
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET));
if (lastResult != 0) {
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET);
if (lastResult != MDB_SUCCESS) {
// use MDB_SET_RANGE if key was deleted
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
}
if (lastResult != 0) {
if (lastResult != MDB_SUCCESS) {
closeInternal(false);
return null;
}
Expand All @@ -153,16 +153,16 @@ public long[] next() {
}

if (fetchNext) {
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
fetchNext = false;
} else {
if (minKeyBuf != null) {
// set cursor to min key
keyData.mv_data(minKeyBuf);
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
} else {
// set cursor to first item
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
}
}

Expand All @@ -172,7 +172,7 @@ public long[] next() {
lastResult = MDB_NOTFOUND;
} else if (groupMatcher != null && !groupMatcher.matches(keyData.mv_data())) {
// value doesn't match search key/mask, fetch next value
lastResult = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
lastResult = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
} else {
// Matching value found
index.keyToQuad(keyData.mv_data(), quad);
Expand All @@ -183,8 +183,6 @@ public long[] next() {
}
closeInternal(false);
return null;
} catch (IOException e) {
throw new SailException(e);
} finally {
txnLock.unlockRead(stamp);
}
Expand Down
19 changes: 14 additions & 5 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbSailStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,12 +572,14 @@ public void approve(Resource subj, IRI pred, Value obj, Resource ctx) throws Sai

@Override
public void approveAll(Set<Statement> approved, Set<Resource> approvedContexts) {
Statement last = null;

sinkStoreAccessLock.lock();
try {
startTransaction(true);

for (Statement statement : approved) {
last = statement;
Resource subj = statement.getSubject();
IRI pred = statement.getPredicate();
Value obj = statement.getObject();
Expand All @@ -604,13 +606,20 @@ public void approveAll(Set<Statement> approved, Set<Resource> approvedContexts)
}

}
} catch (IOException e) {
} catch (IOException | RuntimeException e) {
rollback();
if (multiThreadingActive) {
logger.error("Encountered an unexpected problem while trying to add a statement.", e);
} else {
logger.error(
"Encountered an unexpected problem while trying to add a statement. Last statement that was attempted to be added: [ {} ]",
last, e);
}

if (e instanceof RuntimeException) {
throw (RuntimeException) e;
}
throw new SailException(e);
} catch (RuntimeException e) {
rollback();
logger.error("Encountered an unexpected problem while trying to add a statement", e);
throw e;
} finally {
sinkStoreAccessLock.unlock();
}
Expand Down
11 changes: 9 additions & 2 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/LmdbUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static org.lwjgl.system.MemoryStack.stackPush;
import static org.lwjgl.system.MemoryUtil.NULL;
import static org.lwjgl.util.lmdb.LMDB.MDB_DBS_FULL;
import static org.lwjgl.util.lmdb.LMDB.MDB_KEYEXIST;
import static org.lwjgl.util.lmdb.LMDB.MDB_NOTFOUND;
import static org.lwjgl.util.lmdb.LMDB.MDB_RDONLY;
Expand All @@ -39,12 +40,16 @@
import org.lwjgl.system.Pointer;
import org.lwjgl.util.lmdb.MDBCmpFuncI;
import org.lwjgl.util.lmdb.MDBVal;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Utility class for working with LMDB.
*/
final class LmdbUtil {

private static final Logger logger = LoggerFactory.getLogger(LmdbUtil.class);

/**
* Minimum free space in an LMDB db before automatically resizing the map.
*/
Expand All @@ -61,7 +66,9 @@ private LmdbUtil() {

static int E(int rc) throws IOException {
if (rc != MDB_SUCCESS && rc != MDB_NOTFOUND && rc != MDB_KEYEXIST) {
throw new IOException(mdb_strerror(rc));
IOException ioException = new IOException(mdb_strerror(rc));
logger.info("Possible LMDB error: {}", mdb_strerror(rc), ioException);
throw ioException;
}
return rc;
}
Expand Down Expand Up @@ -105,7 +112,7 @@ static <T> T transaction(long env, Transaction<T> transaction) throws IOExceptio
int err;
try {
ret = transaction.exec(stack, txn);
err = E(mdb_txn_commit(txn));
err = mdb_txn_commit(txn);
} catch (Throwable t) {
mdb_txn_abort(txn);
throw t;
Expand Down
30 changes: 28 additions & 2 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/PersistentSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import static org.eclipse.rdf4j.sail.lmdb.LmdbUtil.E;
import static org.lwjgl.system.MemoryUtil.NULL;
import static org.lwjgl.util.lmdb.LMDB.MDB_MAP_FULL;
import static org.lwjgl.util.lmdb.LMDB.MDB_NEXT;
import static org.lwjgl.util.lmdb.LMDB.MDB_NOOVERWRITE;
import static org.lwjgl.util.lmdb.LMDB.MDB_SET;
Expand All @@ -24,6 +25,7 @@
import static org.lwjgl.util.lmdb.LMDB.mdb_del;
import static org.lwjgl.util.lmdb.LMDB.mdb_drop;
import static org.lwjgl.util.lmdb.LMDB.mdb_put;
import static org.lwjgl.util.lmdb.LMDB.mdb_strerror;
import static org.lwjgl.util.lmdb.LMDB.mdb_txn_abort;
import static org.lwjgl.util.lmdb.LMDB.mdb_txn_begin;

Expand All @@ -43,12 +45,16 @@
import org.lwjgl.PointerBuffer;
import org.lwjgl.system.MemoryStack;
import org.lwjgl.util.lmdb.MDBVal;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* A LMDB-based persistent set.
*/
class PersistentSet<T extends Serializable> extends AbstractSet<T> {

private static final Logger logger = LoggerFactory.getLogger(PersistentSet.class);

private PersistentSetFactory<T> factory;
private final int dbi;
private int size;
Expand Down Expand Up @@ -126,15 +132,35 @@ private synchronized boolean update(Object element, boolean add) throws IOExcept
keyVal.mv_data(keyBuf);

if (add) {
if (E(mdb_put(factory.writeTxn, dbi, keyVal, dataVal, MDB_NOOVERWRITE)) == MDB_SUCCESS) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this to some degree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some simple retry logic to this one.

int rc = mdb_put(factory.writeTxn, dbi, keyVal, dataVal, MDB_NOOVERWRITE);
if (rc == MDB_SUCCESS) {
size++;
return true;
} else if (rc == MDB_MAP_FULL) {
factory.ensureResize();
if (mdb_put(factory.writeTxn, dbi, keyVal, dataVal, MDB_NOOVERWRITE) == MDB_SUCCESS) {
size++;
return true;
}
return false;
} else {
logger.debug("Failed to add element due to error {}: {}", mdb_strerror(rc), element);
}
} else {
// delete element
if (mdb_del(factory.writeTxn, dbi, keyVal, dataVal) == MDB_SUCCESS) {
int rc = mdb_del(factory.writeTxn, dbi, keyVal, dataVal);
if (rc == MDB_SUCCESS) {
size--;
return true;
} else if (rc == MDB_MAP_FULL) {
factory.ensureResize();
if (mdb_del(factory.writeTxn, dbi, keyVal, dataVal) == MDB_SUCCESS) {
size--;
return true;
}
return false;
} else {
logger.debug("Failed to remove element due to error {}: {}", mdb_strerror(rc), element);
}
}
return false;
Expand Down
40 changes: 20 additions & 20 deletions core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/TripleStore.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep the checks for the put methods to detect any MDB_MAP_FULL errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have access to my computer for a good week. Feel free to modify the PR and merge when you're happy with it. Otherwise I'll look at it when I'm back.

Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ protected void filterUsedIds(Collection<Long> ids) throws IOException {
keyBuf.clear();
Varint.writeUnsigned(keyBuf, id);
keyData.mv_data(keyBuf.flip());
if (E(mdb_get(txn, contextsDbi, keyData, valueData)) == MDB_SUCCESS) {
if (mdb_get(txn, contextsDbi, keyData, valueData) == MDB_SUCCESS) {
it.remove();
}
}
Expand All @@ -587,15 +587,15 @@ protected void filterUsedIds(Collection<Long> ids) throws IOException {

if (fullScan) {
long[] quad = new long[4];
int rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_FIRST));
int rc = mdb_cursor_get(cursor, keyData, valueData, MDB_FIRST);
while (rc == MDB_SUCCESS && !ids.isEmpty()) {
index.keyToQuad(keyData.mv_data(), quad);
ids.remove(quad[0]);
ids.remove(quad[1]);
ids.remove(quad[2]);
ids.remove(quad[3]);

rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
}
} else {
for (Iterator<Long> it = ids.iterator(); it.hasNext();) {
Expand Down Expand Up @@ -625,15 +625,15 @@ protected void filterUsedIds(Collection<Long> ids) throws IOException {

// set cursor to min key
keyData.mv_data(keyBuf);
int rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
int rc = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
boolean exists = false;
while (!exists && rc == 0) {
while (!exists && rc == MDB_SUCCESS) {
if (mdb_cmp(txn, dbi, keyData, maxKey) > 0) {
// id was not found
break;
} else if (!matcher.matches(keyData.mv_data())) {
// value doesn't match search key/mask, fetch next value
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
} else {
exists = true;
}
Expand Down Expand Up @@ -708,24 +708,24 @@ protected double cardinality(long subj, long pred, long obj, long context) throw

// set cursor to min key
keyData.mv_data(keyBuf);
int rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
if (rc != 0 || mdb_cmp(txn, dbi, keyData, maxKey) >= 0) {
int rc = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
if (rc != MDB_SUCCESS || mdb_cmp(txn, dbi, keyData, maxKey) >= 0) {
break;
} else {
Varint.readListUnsigned(keyData.mv_data(), s.minValues);
}

// set cursor to max key
keyData.mv_data(maxKeyBuf);
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
if (rc != 0) {
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
if (rc != MDB_SUCCESS) {
// directly go to last value
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_LAST));
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_LAST);
} else {
// go to previous value of selected key
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_PREV));
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_PREV);
}
if (rc == 0) {
if (rc == MDB_SUCCESS) {
Varint.readListUnsigned(keyData.mv_data(), s.maxValues);
// this is required to correctly estimate the range size at a later point
s.startValues[s.MAX_BUCKETS] = s.maxValues;
Expand All @@ -747,7 +747,7 @@ protected double cardinality(long subj, long pred, long obj, long context) throw
keyData.mv_data(keyBuf);

int currentSamplesCount = 0;
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE));
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_SET_RANGE);
while (rc == MDB_SUCCESS && currentSamplesCount < s.MAX_SAMPLES_PER_BUCKET) {
if (mdb_cmp(txn, dbi, keyData, maxKey) >= 0) {
endOfRange = true;
Expand Down Expand Up @@ -776,8 +776,8 @@ protected double cardinality(long subj, long pred, long obj, long context) throw
}
}
}
rc = E(mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT));
if (rc != 0) {
rc = mdb_cursor_get(cursor, keyData, valueData, MDB_NEXT);
if (rc != MDB_SUCCESS) {
// no more elements are available
endOfRange = true;
}
Expand Down Expand Up @@ -873,14 +873,14 @@ public boolean storeTriple(long subj, long pred, long obj, long context, boolean
return recordCache.storeRecord(quad, explicit);
}

int rc = E(mdb_put(writeTxn, mainIndex.getDB(explicit), keyVal, dataVal, MDB_NOOVERWRITE));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this to some degree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually checked right below.

int rc = mdb_put(writeTxn, mainIndex.getDB(explicit), keyVal, dataVal, MDB_NOOVERWRITE);
if (rc != MDB_SUCCESS && rc != MDB_KEYEXIST) {
throw new IOException(mdb_strerror(rc));
}
stAdded = rc == MDB_SUCCESS;
boolean foundImplicit = false;
if (explicit && stAdded) {
foundImplicit = E(mdb_del(writeTxn, mainIndex.getDB(false), keyVal, dataVal)) == MDB_SUCCESS;
foundImplicit = mdb_del(writeTxn, mainIndex.getDB(false), keyVal, dataVal) == MDB_SUCCESS;
}

if (stAdded) {
Expand Down Expand Up @@ -920,7 +920,7 @@ private void incrementContext(MemoryStack stack, long context) throws IOExceptio
idVal.mv_data(bb);
MDBVal dataVal = MDBVal.calloc(stack);
long newCount = 1;
if (E(mdb_get(writeTxn, contextsDbi, idVal, dataVal)) == MDB_SUCCESS) {
if (mdb_get(writeTxn, contextsDbi, idVal, dataVal) == MDB_SUCCESS) {
// update count
newCount = Varint.readUnsigned(dataVal.mv_data()) + 1;
}
Expand All @@ -944,7 +944,7 @@ private boolean decrementContext(MemoryStack stack, long context) throws IOExcep
bb.flip();
idVal.mv_data(bb);
MDBVal dataVal = MDBVal.calloc(stack);
if (E(mdb_get(writeTxn, contextsDbi, idVal, dataVal)) == MDB_SUCCESS) {
if (mdb_get(writeTxn, contextsDbi, idVal, dataVal) == MDB_SUCCESS) {
// update count
long newCount = Varint.readUnsigned(dataVal.mv_data()) - 1;
if (newCount <= 0) {
Expand Down
Loading
Loading