-
-
Notifications
You must be signed in to change notification settings - Fork 212
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.
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
v3.2.0 #229
v3.2.0 #229
Conversation
…pdated/deleted rows. This greatly simplifies everything, however there's still some concurrency problem lurking...
…borted until the transaction actually commits, then they're deleted. This drastically reduces the number of docs we need to track in ES, and eliminates the "hints" type entirely, which is also great.
…Lion. It's not a usable library (maybe it is? I haven't actually tried), but being able to find compilation problems within CLion is nice
in elasticsearch.c#elasticsearch_vacuumCleanup() that causes an xid to be deleted that is otherwise still visible by a running transaction. Still looking into this. Also cleanup how we refresh a bit -- do a number of fewer refreshes now
important to account for the fact that they may represent xmin and/or xmax values. And we need to delete docs accordingly.
"data" and "xmax" along with the blocknumber as an integer to "data". VisibilityQueryHelper still has some debugging output that needs to be deleted.
…binary" to "data", and rename "_zdb_quick_lookup" in "xmax" to the same
found got deleted before we searched for it in "data"
we put things into the database.
… part of ES 1.7.5. Also get rid of the "known_visible" check -- it's just extra cycles and confusing and unnecessary.
that xmax doc was created because of an "U"pdate or "D"elete. We don't use this for anything, but can be useful for debugging purposes.
Version of the "xmax" doc we want to delete. Additionally go ahead and allow the transaction id to be deleted from "aborted" since it's known-to-be-considered-aborted-by-all-current-and-future-transactions inside Postgres. Also add some comments about what's going on and of course, call the method from zdbam.c#vacuumcleanup
that we think are invisible to us, rather than running through every possible ItemPointer
@@ -54,7 +54,7 @@ protected void handleRequest(RestRequest request, RestChannel channel, Client cl | |||
SearchRequestBuilder builder = new SearchRequestBuilder(client); | |||
String input = request.content().toUtf8(); | |||
final QueryRewriter rewriter = QueryRewriter.Factory.create(client, request.param("index"), request.param("preference"), input, true, true, true); | |||
QueryBuilder qb = rewriter.rewriteQuery(true); | |||
QueryBuilder qb = rewriter.rewriteQuery(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this boolean argument to QueryBuilder.rewriteQuery()
anymore
@@ -47,7 +48,7 @@ protected void handleRequest(RestRequest request, RestChannel channel, Client cl | |||
BytesRestResponse response; | |||
QueryAndIndexPair query; | |||
|
|||
query = PostgresTIDResponseAction.buildJsonQueryFromRequestContent(client, request, !isSelectivityQuery, true, true, true); | |||
query = PostgresTIDResponseAction.buildJsonQueryFromRequestContent(client, request, !isSelectivityQuery, true, true, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need last boolean argument to PostgresTIDResponseAction.buildJsonQueryFromRequestContent()
Number cmin = (Number) data.get("_cmin"); | ||
Number sequence = (Number) data.get("_zdb_seq"); // -1 means an index build (CREATE INDEX) | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: In a future release, move this tuple encoding to the Postgres side of things so as to avoid manipulating and copying large byte arrays for every row we're indexing.
|
||
client.admin().indices().refresh(new RefreshRequestBuilder(client.admin().indices()).setIndices(index).request()).actionGet(); | ||
|
||
SearchRequestBuilder search = new SearchRequestBuilder(client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document what we consider a possible vacuum-able transaction id, and why
|
||
client.admin().indices().refresh(new RefreshRequestBuilder(client.admin().indices()).setIndices(index).request()).actionGet(); | ||
|
||
SearchRequestBuilder search = new SearchRequestBuilder(client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document what we're querying for here, and why.
Answer: It's every doc we think is currently invisible to us.
for (int i = 0; i < tmp.length; i++) | ||
active[i] = Long.valueOf(tmp[i]); | ||
|
||
client.admin().indices().refresh(new RefreshRequestBuilder(client.admin().indices()).setIndices(index).request()).actionGet(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document why we need to refresh before we vacuum.
Answer: so that we don't miss any invisible tuples from aborted transactions when zombodb.batch_mode = on
that Postgres' VACUUM process thinks need to be vacuumed
@@ -0,0 +1,36 @@ | |||
DROP TABLE IF EXISTS suad; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this entire file -- didn't mean to ever commit it
Attached are artifacts for this Pull Request. This release is going to require that indexes be re-created from scratch :(, so the process should be something like:
|
|
||
final class VisibilityQueryHelper { | ||
|
||
private static final ConcurrentSkipListSet<Long> KNOWN_COMMITTED_XIDS = new ConcurrentSkipListSet<>(); | ||
private static class HeapTuple implements Comparable<HeapTuple> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull this out into a standalone class and put some unit tests around its .equals()
and .compareTo()
methods as they were a source of bugs during development
be easily unit tested
ItemPointers for every possible (BlockNumber, OffsetNumber) in the table, using Postgres' visibility_map to short-circuit blocks when we can. This fixes an issue with not enough docs being deleted during (auto)VACUUM when zombodb.batch_mode = on.
Updated artifacts that fix a bug with (auto)vacuum and |
when deleting "xmax" docs. This is actually a perfectly normal and expected response in cases where we try to delete an "xmax" doc that's been replaced by a new doc between when we thought we needed to delete it and when we actually do. This really should have been part of 34377b9 Also, change logging message so that a tail postgresql.log | grep "zombodb vacuum" will actually find this error.
it as an error anyways.
Updated artifacts that fix the "VersionConflictEngineException" during (auto)vacuum. |
A pull request for what will soon become v3.2.0. Release notes coming soon...