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.

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

1.1.0-rc1: count selector at root ignores given value #3840

Closed
mehdiym opened this issue Aug 20, 2019 · 9 comments · Fixed by #3874
Closed

1.1.0-rc1: count selector at root ignores given value #3840

mehdiym opened this issue Aug 20, 2019 · 9 comments · Fixed by #3874
Assignees
Labels
kind/bug Something is broken. priority/P0 Critical issue that requires immediate attention. status/accepted We accept to investigate/work on it.
Milestone

Comments

@mehdiym
Copy link

mehdiym commented Aug 20, 2019

  • What version of Dgraph are you using?
    1.1.0-RC1

  • Steps to reproduce the issue (command/config used to run Dgraph).
    Set in the schema a predicate with @reverse and @count, eg:

game_answer: uid @reverse @count			.

Add nodes and edges with that predicate, then do a query based on the count, eg:

{
  me(func: eq(count(~game_answer), 1)) {
        count(~game_answer)
  }
}
  • Expected behaviour and actual result.
    I would expect results having only one reverse edge ~game_answer.

My result:

{
  "data": {
    "me": [
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 0
      },
      {
        "count(~game_answer)": 2
      },
      {
        "count(~game_answer)": 1
      }
    ]
  },
  "extensions": {
    "server_latency": {
      "parsing_ns": 13885,
      "processing_ns": 1943052,
      "encoding_ns": 43871,
      "assign_timestamp_ns": 622187
    },
    "txn": {
      "start_ts": 92816
    }
  }
}

Oddly, if I add that selector as a post filter, it works, eg:

{
  me(func: eq(count(~game_answer), 1)) @filter(eq(count(~game_answer), 1)) {
        count(~game_answer)
  }
}

Gives:

{
  "data": {
    "me": [
      {
        "count(~game_answer)": 1
      }
    ]
  },
  "extensions": {
    "server_latency": {
      "parsing_ns": 9825,
      "processing_ns": 1392139,
      "encoding_ns": 9564,
      "assign_timestamp_ns": 660592
    },
    "txn": {
      "start_ts": 93770
    }
  }
}
@danielmai
Copy link
Contributor

Hi @mehdiym can you provide a sample data set and exact steps you did starting from a fresh cluster that led to the result you're seeing?

@mehdiym
Copy link
Author

mehdiym commented Aug 20, 2019

I'll send you a dump of my database on slack.
I only use one alpha instance, 4Go lru. I reproduce it with my integration tests, even from a fresh new database (by removing p, w and zw dirs).
Restoring the database from a standard rdf export/import removes the bug.

@danielmai
Copy link
Contributor

I'll send you a dump of my database on slack.

Great, looking forward to your message on Slack.

@danielmai
Copy link
Contributor

I can reproduce the issue with the database p, w, and zw dirs you shared. I suspect this issue is due to how your integration tests load data into Dgraph. I can't reproduce this with live loader, bulk loader, or constructing my own sample data set by loading via mutations.

If you can share the Dgraph data loading steps that's done by your tests we should be able to reproduce this issue.

@danielmai danielmai added the kind/bug Something is broken. label Aug 20, 2019
@danielmai danielmai self-assigned this Aug 20, 2019
@danielmai danielmai added the status/more-info-needed The issue has been sent back to the reporter asking for clarifications label Aug 20, 2019
@danielmai
Copy link
Contributor

danielmai commented Aug 21, 2019

In Dgraph v1.1.0-rc1 we are not updating the count index for delete mutations. Specifically, the count index for reverse edges.

@danielmai danielmai added status/accepted We accept to investigate/work on it. and removed status/more-info-needed The issue has been sent back to the reporter asking for clarifications labels Aug 21, 2019
@mehdiym
Copy link
Author

mehdiym commented Aug 21, 2019

I'm glad you found it out.

@animesh2049
Copy link
Contributor

@mangalaman93 ran git bisect to find out where did this start. Below is the output of that.

aman@dgraph:~/gocode/src/github.com/dgraph-io/dgraph$ git bisect good
d697ca0898f0ac951fb041bfc517f77377e0cd28 is the first bad commit
commit d697ca0898f0ac951fb041bfc517f77377e0cd28
Author: Martin Martinez Rivera <[email protected]>
Date:   Tue Jul 23 12:06:16 2019 -0700

    Don't read posting lists from disk when mutating indices. (#3695)
    
    Adding index mutations can be performed without reading the posting
    lists in disk. This change modifies the indexing process to avoid
    reading any list from disk. It should result in performance
    improvements, specially when trying to modify big index posting lists.

:040000 040000 3f69fd25fa855d1ac1239cdda0be7d48581781d0 9e2c628acbaf9042f6e501cf24400f7e51a34bf1 M    posting
aman@dgraph:~/gocode/src/github.com/dgraph-io/dgraph$ git bisect log
git bisect start
# good: [35538d1d706e984e62daf8669ba2057c1a266677] Update HTTP API Content-Type to application/graphql+-
git bisect good 35538d1d706e984e62daf8669ba2057c1a266677
# bad: [9af081d10c35e68942edf0c3672f77f5329f0c97] Vendor in missing dependencies. (#3855)
git bisect bad 9af081d10c35e68942edf0c3672f77f5329f0c97
# good: [60b2b15240c676639f80428837344c3cb19c2fdb] Fix crash when trying to use shortest path with a password predicate. (#3662)
git bisect good 60b2b15240c676639f80428837344c3cb19c2fdb
# bad: [58820c0124a76e26e0b3c4acf99666234eb3ff48] Add more documentation for backup and restore features. (#3660)
git bisect bad 58820c0124a76e26e0b3c4acf99666234eb3ff48
# bad: [58820c0124a76e26e0b3c4acf99666234eb3ff48] Add more documentation for backup and restore features. (#3660)
git bisect bad 58820c0124a76e26e0b3c4acf99666234eb3ff48
# good: [bdca82adca74041bae5fcf0bc0db19c2a6aa6b6f] Revert "fix race conditions in bulk loader and export (#3697)" (#3704)
git bisect good bdca82adca74041bae5fcf0bc0db19c2a6aa6b6f
# bad: [d0767bcb6569f0c65cef283d47bf77239d8d8104] Modify loader_test.go to use an existing docker cluster. (#3689)
git bisect bad d0767bcb6569f0c65cef283d47bf77239d8d8104
# bad: [d697ca0898f0ac951fb041bfc517f77377e0cd28] Don't read posting lists from disk when mutating indices. (#3695)
git bisect bad d697ca0898f0ac951fb041bfc517f77377e0cd28
# good: [4b41d9c907f3cef4ab83bbbdc1f9ba86f354134d] Decrease rate of Raft heartbeat messages. (#3708)
git bisect good 4b41d9c907f3cef4ab83bbbdc1f9ba86f354134d
# good: [b7beb08dc839cb7e4d478c8c00eabbfbe79f6342] In backup tests, check the number of manifests equals number of dirs. (#3706)
git bisect good b7beb08dc839cb7e4d478c8c00eabbfbe79f6342
# first bad commit: [d697ca0898f0ac951fb041bfc517f77377e0cd28] Don't read posting lists from disk when mutating indices. (#3695)
aman@dgraph:~/gocode/src/github.com/dgraph-io/dgraph$ 

@animesh2049
Copy link
Contributor

I think there is some bug in GetFromDelta function.

plist, err := txn.cache.GetFromDelta(key)

Changing it to txn.Get pases the test in Aman's PR

@manishrjain manishrjain added the priority/P0 Critical issue that requires immediate attention. label Aug 26, 2019
@manishrjain manishrjain added this to the Dgraph v1.1 milestone Aug 26, 2019
animesh2049 added a commit that referenced this issue Aug 27, 2019
We cannot use txn.cache.GetFromDeltas for reverse mutations becasue
cache will not have deltas corresponding to reverse key. Fixes #3840
manishrjain pushed a commit that referenced this issue Aug 28, 2019
…3874)

We cannot use txn.cache.GetFromDeltas for reverse mutations becasue
cache will not have deltas corresponding to reverse key. Fixes #3840

* Only retrieve the full PL if needed for reverse count. If no count needed, don't read the PL from disk.
@mehdiym
Copy link
Author

mehdiym commented Aug 28, 2019

Working great now, all my integration tests pass, thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something is broken. priority/P0 Critical issue that requires immediate attention. status/accepted We accept to investigate/work on it.
4 participants