More consistency checking for relationship group records #2035

Merged
merged 1 commit into from Mar 10, 2014

Projects

None yet

3 participants

@tinwelint
Neo4j member

Added the following checks:
o RelationshipGroupRecord has invalid rel type id
o RelationshipGroupRecord has invalid next pointer
o RelationshipGroupRecord chain is unsorted
o RelationshipGroupRecord refers to relationship that is not in use
o RelationshipGroupRecord refers to relationship that is not first in its
chain
o RelationshipGroupRecord refers to relationship that is of a different
relationship type than its group

@tinwelint tinwelint closed this Feb 24, 2014
@tinwelint tinwelint reopened this Feb 24, 2014
@chrisvest

Needs rebase.

@tinwelint tinwelint More consistency checking for relationship group records
Added the following checks:
o RelationshipGroupRecord has invalid rel type id
o RelationshipGroupRecord has invalid next pointer
o RelationshipGroupRecord chain is unsorted
o RelationshipGroupRecord refers to relationship that is not in use
o RelationshipGroupRecord refers to relationship that is not first in its
  chain
o RelationshipGroupRecord refers to relationship that is of a different
  relationship type than its group
67d5f27
@chrisvest chrisvest commented on the diff Mar 5, 2014
.../kernel/impl/nioneo/store/RelationshipGroupStore.java
RelationshipGroupRecord record = new RelationshipGroupRecord( id, type );
- record.setInUse( true );
+ record.setInUse( inUse );
@chrisvest
chrisvest Mar 5, 2014

inUse is always true as far as I can tell, because we have an early escape higher up the method if it is false. So this change makes no difference.

@tinwelint
tinwelint Mar 6, 2014

It's deceptive, I know... we have a switch above, but we let the FORCE load through, which means that it can actually be false. Some consistency tests failed when it was always setting it to true, since it thought it found an in use record and began consistency checking its fields.

For reference look at any other getRecord method in f.ex. NodeStore

@chrisvest
chrisvest Mar 6, 2014

Aha, good point.

@chrisvest chrisvest commented on the diff Mar 5, 2014
...a/org/neo4j/consistency/checking/full/OwnerCheck.java
@@ -370,6 +375,14 @@ public void checkChange( DynamicRecord oldRecord, DynamicRecord newRecord,
};
}
+ @Override
+ public RecordCheck<RelationshipGroupRecord, RelationshipGroupConsistencyReport> decorateRelationshipGroupChecker(
+ RecordCheck<RelationshipGroupRecord, RelationshipGroupConsistencyReport> checker )
+ {
+ // TODO implement owner checking for relationship groups?
@chrisvest
chrisvest Mar 5, 2014

Good question.

@tinwelint
tinwelint Mar 6, 2014

Yeah, that also poses the question whether or not we should store that information in each and every relationship group record, for the sole reason to aid the consistency checker.

@chrisvest
chrisvest Mar 6, 2014

I think it would be useful should we ever run into an inconsistency where a node somehow forgets about a group of relationships it had. Otherwise we have to check that all RelationshipGroupRecords that are marked as in use, are also referenced by a NodeRecord somewhere. Perhaps you're already doing that and I'm just not seeing it?

@jakewins
jakewins Mar 10, 2014

+1. If nobody minds though, I'm going to merge this, and put the card back in the backlog with a note to resolve this last bit.

@jakewins jakewins merged commit 79ff1c8 into neo4j:master Mar 10, 2014

1 check passed

Details default Merged build finished.
@jakewins jakewins deleted the tinwelint:2.1-cc-relgroups branch Mar 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment