ksAppendKey locks name, ksPop does not unlock name

[mpranj]

It makes sense that ksAppendKey() locks the key name, since it determines the position of the key in the keyset. However, I did not find an API function that clears the KEY_FLAG_RO_NAME flag.

It would seem intuitive to me, that ksPop (or lookup with KDB_O_POP) would unlock the name again. Is a key not meant to be re-used? Am I missing some important part of the API design, or would my suggestion be of interest.

The question comes from the context of writing the storage plugin tests/scenarios. I moved many tests out of my mmapstorage plugin to ctest. Currently, it tests dump and mmapstorage only.

[e1528532]

I've encountered the same issue in the pluginprocess library and worked around by copying the relevant stuff manually after i finally understood what caused the issue :) To me this behavior was also unintuitive.

[markus2330]

Yes, you are right. If the ref counter is back to its original value, unlocking a key's name should be safe. Are there unit tests checking for this?

[mpranj]

Are there unit tests checking for this?

I'm not sure what you mean. I encountered it when writing tests. There is however, no unit test, checking whether a key name is unlocked when the ref counter is 0 again.

[markus2330]

There is however, no unit test, checking whether a key name is unlocked when the ref counter is 0 again.

I meant if there is a unit test checking the other way round (if the key name stays locked, even after removing it from every KeySet). This would break the ABI tests then. If the test suite says nothing after changing this behavior, we are fine about that.

Another problem might be with applications manually tempting with the ref counter. One could add a key to a keyset, then decrease the ref counter, then change the name, breaking the KeySet's invariant (which now got even more important because of the OPMHP from @KurtMi). To solve this problem we would need a second ref counter which only remembers the number of KeySet's a key is in.

[mpranj]

For me the problem is "solved". I work around this by clearing the flag by hand (since in the unit tests I know I hold the only reference to it). I wanted to be able to pop a key from a keyset, change the name, and then let the storage plugin store it again. In the case of mmapstorage, it's a very interesting test case since it changes a lot in the data and reveals memory handling problems.

The code change seems trivial. If it really has large implications, we can put it to future ideas or close it.

[markus2330]

Yes, it is no problem if you clear the flag in the unit test (it is, however, not an ABI test then).

[markus2330]

Might be good to fix this for 1.0 as it is an incompatible change.

[stale]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

[stale]

I closed this issue now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

[mpranj]

I think we can do this as part of the breaking changes series.

[markus2330]

Yes, I agree!

[markus2330]

Maybe we can simplify it by combining the locking with the ref counter: With increased ref counter the key name gets locked, with a ref counter 1 it gets unlocked again. This basically reserves the ref counter to be used only with data structures and not for bindings. To make both things work (unlocking names + ref counting for bindings) we need two ref counters.

[mpranj]

To make both things work (unlocking names + ref counting for bindings) we need two ref counters.

Are bindings really using the ref counter? From what I see they mostly only wrap the incRef / decRef functions ..

If we can simplify it to one refcounter it sounds like a nice solution. If we need to add a second refcounter it seems to be more complex than just using a flag to lock the name ...

[markus2330]

Hopefully we will get a better API, like keyBorrow for incrementing the ref counter and the keyLock API for inc/dec of available locks.

I am still not totally convinced that this bug here alone is enough to justify two counters.

[markus2330]

Are bindings really using the ref counter?

At least C++ does heavily and as such all SWIG bindings do, too.

[mpranj]

I am still not totally convinced that this bug here alone is enough to justify two counters.

Yes, maybe it is a non-issue, especially if we remove/rename ksPop.
We could probably just improve the documentation and state that a key name can not be changed even after a ksPop. That was my main issue, I was under the impression that i could reuse a key after ksPop.

[markus2330]

A different perspective on this issue: if renaming of a key is allowed is a complicated matter and actually cannot be decided from a key being in a KeySet. There are (many) valid rename operations also on keys within keysets, e.g., keysets with 1 key or by adding the same basename to every key in a keyset.

I now tend towards having less enforced checks: in particular in situations where we actually cannot correctly check if an operation is allowed. Maybe we should simply allow unlocking, document the properties we require for KeySets (which are very simple to understand: do not destroy the order) and trust that people know what they are doing if they are explicitly doing the unlock.

If we would completely remove the lock, we would actually also get rid of the problem #3972, won't we?

[stale]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

[kodebach]

I can't find the issue now, but IIRC the decision was to make key names read-only based on a second reference counter that only counts references from KeySets.

[markus2330]

I was not really in favor of two reference counters but if we have/get two reference counters, it would be a good idea that the one counter is to count key-name-is-read-only and the other one counts key-can-be-freed.

In any case: we should only provide a solution if it is correct. Otherwise it is much better to not lock/unlock, see above.

[kodebach]

Yes, of course the simplest solution is to not lock/unlock. But then it is kinda hard to keep track whether a Key is contained in a KeySet, i.e. whether it is safe to change its name.

one counter is to count key-name-is-read-only and the other one counts key-can-be-freed.

Yes, that's basically what I had in mind. The way I would phrase it is that the lock for the keyname is not a simple binary lock (locked/unlocked), but is more like a semaphore. If you lock the keyname N times, you also need to unlock it N times before it is writeable again. I wouldn't actually call it a second "reference counter", because then you need to distinguish different kinds of references and that just complicates everything.

we should only provide a solution if it is correct.

I don't see how this "counted lock" could be incorrect.

[github-actions]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

[github-actions]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖