Add support for context.Context

[tie]

This PR adds context.Context support in Genji, starting with the engine package and then fixing compile errors everywhere. It doesn’t introduce any cancellation behavior though—that should be a separate, smaller, PR.

• engine.Iterator usage now requires a it.Err() check after the loop.

  it := st.Iterator(engine.IteratorOptions{})
  defer it.Close()
  
  for it.Seek(ctx, nil); it.Valid(); it.Next(ctx) {
  	…
  }
  if err := it.Err(); err != nil {
  	…
  }
  

  Notice that Seek and Next now accept a context.Context parameter. If an error occurs, Valid returns false.

• database.Table no longer directly implements document.Iterator since iteration may be I/O bound.

  // Before
  func (*Table) Iterate(func(d document.Document) error) error
  // After
  func (*Table) Iterator(context.Context) document.IteratorFunc
  func (*Table) Iterate(context.Context, func(d document.Document) error) error
  

Closes #224 and #206

[codecov-io]

Codecov Report

Merging #231 into master will decrease coverage by 0.15%.
The diff coverage is 70.76%.

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
- Coverage   61.34%   61.18%   -0.16%     
==========================================
  Files          68       68              
  Lines        6327     6351      +24     
==========================================
+ Hits         3881     3886       +5     
- Misses       1933     1949      +16     
- Partials      513      516       +3     

Impacted Files	Coverage Δ
document/iterator.go	33.80% <ø> (+0.70%)	⬆️
sql/planner/sort.go	15.27% <ø> (ø)
sql/planner/tree.go	37.57% <0.00%> (ø)
sql/query/expr/comparison.go	25.65% <0.00%> (-0.79%)	⬇️
sql/planner/input.go	37.70% <20.00%> (-0.63%)	⬇️
sql/planner/replacement.go	15.38% <20.00%> (-0.62%)	⬇️
sql/planner/deletion.go	29.62% <50.00%> (ø)
db.go	41.79% <62.50%> (-3.21%)	⬇️
sql/planner/binder.go	56.25% <75.00%> (ø)
sql/planner/optimizer.go	84.15% <75.00%> (ø)
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a3cc7b...a2a632a. Read the comment docs.

[asdine]

I know the PR is still a draft but since it contains pretty heavy changes I'd like to comment now to prevent you from doing unnecessary work if we agree on a different path.

Basically, I would like to avoid context pollution where it is not necessary.

IMO the benefit of context in the case of Genji is essentially for canceling anything that relies on IO, even indirectly.

Some APIs are designed to be one time in-memory procedures. For these, handling cancelation from within the function is the same as handling it from outside, so passing the context is not necessary. I'm thinking about the planner package for example, which takes a finite, not so big AST. I think it's ok to let it finish before checking if a context is canceled. If it ever become too big of course, we might consider passing a context. Nevermind, I realize that Bind needs the transaction, but not Optimize apparently.

Even on IO related APIs, I believe we can avoid context pollution by having some indirection. For example, instead of passing the context to any engine/store method, we could pass it only to the Begin method and leave the implementation deal with the context for that particular transaction.
From a high level point of view, we want to be able to cancel a transaction, we are not interested in the lifecycle of each particular call to Get or Put.
This is pretty much what the Golang team is doing with virtual filesystem golang/go#41190 (comment)

Concerning the document.Iterator type, it is returned as a result of db.Query, which itself takes a context.
I would be in favor of not passing the context to Iterate and have iterate return ctx.Err() if ctx is done.
By itself, document.Iterator lets the caller control the flow of the stream. If a user wants to stop, it simply needs to stop iterating.

[tie]

Thanks for the input!

I agree on the context pollution issue, but I’d rather avoid indirection as it doesn’t add much value since you either already have context.Context instance e.g. from HTTP request, or can create a new one using context.Background(). Explicit contexts also allow passing some engine-specific per-operation values via context, which could be useful in cases like #58.

In Genji we can have the root genji package hide the explicit context passing using WithContext pattern, e.g.

var db *genji.DB
var tx *genji.Tx

db, _ := genji.Open(ctx, …)          // sets default context to ctx
tx, _ = db.Begin(…)                  // uses default context
db = db.WithContext(ctx)             // overrides default context
tx, _ = db.WithContext(ctx).Begin(…) // overrides per function call
tx.Exec(…)                           // uses context inherited on Begin
tx = tx.WithContext(ctx)
tx.WithContext(ctx).Exec(…)

Here, WithContext returns a shallow copy with a new embedded context. It’s cheap since DB and Tx just wrap the corresponding types in database package, and I’d even expect the compiler to optimize WithContext call if it’s possible to allocate a copy on stack.

The reason why document.Iterator.Iterate and Optimize accept context is that the former is implemented by database.Table (that is, the main concern here is that iteration may be I/O bound), and the latter has UseIndexBasedOnSelectionNodeRule which calls I/O bound APIs.

https://github.com/genjidb/genji/blob/950009f414495518d5ca510dd604ec81385dfa7a/sql/planner/optimizer.go#L281

[tie]

Regarding document.Iterator, I can think of two options.

1. We can add a layer of indirection in database.Table and keep document.Iterator unaware of the context.

   func (t *Table) Iterator(ctx context.Context) document.IteratorFunc
   func (t *Table) Iterate(ctx context.Context, fn func(d document.Document) error) error
   

   I’ve tried implementing this approach and it’s pretty easy to lose track of the context flow in sql/planner.

2. Since db.Query actually returns query.Result, we can use WithContext pattern (see the previous message). This would also make the API consistent in a sense that only lower-level APIs accept explicit context.

[asdine]

@tie I think adding context to Genji requires some discussion because it has a very big impact on the project.
I have written an RFC to express my vision of at least the first iteration of this feature. I think it will be a good place to discuss this and iterate until we find a good solution.

[tie]

@asdine I’ll update this PR to conform with the RFC001 and mark it as ready then. Let me know if you want this in a separate PR though.

[asdine]

@tie Let's start over on a separate PR if you don't mind

[tie]

Sure!