Le ven. 28 juin 2019 à 17:13, <xxxxxx@ancell-ent.com> a écrit :

[...]

A hook style provision here and/or in SRFI-168 on opening a database to allow enforcement of constraints would be nice, that's a major reason I like the RDBMSes I've used in anger, DB2,  PostgreSQL, and Oracle.  I suggest it at this SRFI level because making it boilerplate here makes it more likely all Scheme applications will open a database with the constraints and thus better maintain data integrity.

That is very difficult to do validation at this level. OKVS manipulates bytevectors, to validate keys and values it would require to unpack the bytevector which is not know at this level. I acknowledged that validation is missing piece in the whole framework (SRFI-167 + SRFI-168), I am thinking about ways to improve the situation, but that will not be part of any current SRFI.
 

The name of okvs-debug doesn't explain what it does, it could, for example, be a way of inserting hooks, or specifying tracing.  Something like okvs-map-(over-)all(-keys) would make it self-documenting.  While it's implicitly obvious, it should explicitly state that proc is a procedure that takes two arguments, a key and a value (bytevector).

It is a debug procedure. It is not meant to be used to actually manipulate the database. The intend use is the following:

(okvs-debug (lambda (key value) (display (unpack key)) (display (unpack value)))))
 

Does okvs-debug do its thing in an implicit transaction like the okvs-prefix explicitly does when called with an empty bytevector prefix?

I don't understand, can you show me some code?
 
If you view it as a general vs. debug function, as the suggested name change suggests, adding range arguments or adding an additional function "-range" would make it more efficient for more uses.  I'm assuming that because operating on a range of keys is required in other functions, adding this wouldn't be onerous.

This would be onerous in the sense that it will add more "moving pieces" to the specification. Of course, in real world scenario production database you would not use okvs-debug. And instead rely on okvs-prefix or okvs-range. But that would only small helpers on top of OKVS that doesn't change the spirit of the specification. One way to make easier to debug live databases would be to come up with a generator syntax that reduce the "boilerplate" code related to chaining generators:

(okvs-in-transaction db (lambda (txn) (generator-for-each (lambda (key value) (unpack key) (unpack value)) (okvs-prefix txn (pack 'my 'subspace)))))

 
I can see people preferring to map over a subset of keys vs. using the generator approach,

It rely on generator because that is what is the most performant over map / filter et al.
 
while SRFI-167 is oriented towards supporting SRFI-168,

That is unrelated. All existing okvs libraries bindings traverse the key space using somekind of lazy approach. Fetching a whole key range is only useful to debug.
 
that's not the only way people will use it, especially if they're manipulating OKVSes with schema defined by other non-Scheme applications. 

How is that related? If people share the same database with another language only the (lexicographic) packing procedure is interesting.
The packing / unpacking procedure was removed from the specification because of that very reason:

Do we want compatibility with existing packing algorithm or more Scheme objects support?

So it was removed from the SRFI-167 (also I think I will make `pack` and `unpack` procedures part of SRFI-168's `engine` interface).
 
A further enhancement would be an optional predicate on key to check before calling proc, so retrieving every value is not required.

IIUC more moving pieces.
 
The documentation for okvs-transaction-begin says "The transaction must be associated with a newly created hash-table with the default comparator."  This is the first time and only time a comparator is mentioned, and the first time for hash-table, and they're not explained at all here or elsewhere. 

I agree, I will fix that.
 
Particularly a R7RS hash table that's supposed to do what?

It used to extend to transaction behaviour somehow, it is used in fstore.
It is workaround the fact one can not use okvs-in-transaction with custom semantics.
Another way around that would be to override/extend okvs-in-transaction (or okvs-transaction-begin et al.)
from user code but that is not possible in R7RS.
 
It's necessary for a user to know this since okvs-transaction-metadata -> okvs-transaction-hash-table exposes the hash table (and I sense a potential for mischief...).

The "newly created" might imply you can only execute this function once, perhaps immediately after calling okvs/okvs-open, and before any calls to okvs-in-transaction.

It is not mentioned what happens if a transaction is not committed or rolled back, or if okvs-close is not called before the program exits.  Maybe say in okvs-close that this is "OKVS implementation dependent" since there are OKVSes that don't support transactions, and there might be ones that in practice fail to properly roll back.

Neither is explained in other documentation. Something should be done, something sensible.
 

okvs-transaction-metadata should indeed be renamed okvs-transaction-hash-table as previously discussed, and "Return the hash-table associated with TRANSACTION." is where the unexplained hash table gets problematic as mentioned above.

Ok
 

(okvs-transaction-commit transaction config) looks like it should be (okvs-transaction-commit transaction [config]).

Yes.
 
The [failure [success]] options in okvs-in-transaction look like they would be *really* useful in the begin and rollback or commit transaction approach.  Perhaps only add them to okvs-transaction-begin to avoid Don't Repeat Yourself (DRY)? 

I don't understand why it would be useful to add to transaction-begin.

The lack of [config] in okvs-in-transaction is another way these two methods for doing transactions aren't mirrored, if this is useful for them, it should be useful for okvs-in-transaction.

Yes
 

The use of "pair" and "pairs" in okvs-delete! and okvs-range-remove! should be prepended with "key-value" as it is in the other uses.

Ok
 

The okvs-range-remove! description should include the start-key, start-include?, end-key, and end-include? argument explanations of okvs-range transaction.

Ok


If okvs-maximum-key is not removed from the API, the result of trying to mutate it should be stated, for example raise an error or "undefined".

okvs-maximum-key should be a procedure AFAIU, so it can not be mutated.
 

- Harold


--
Amirouche ~ amz3 ~ https://hyper.dev