Specification for external key table

[clevinson]

As discussed in Architecture Review Meeting (2/6/2020) with Ethan, Aaron, Alex.

[aaronc]

The constructor for this would basically be:

func NewExternalKeyTableBuilder(prefixData, prefixSeq, prefixIndex byte, key sdk.StoreKey, model Persistent)

and instead of the Create and Save methods on NaturalKeyTable there would just be:

func (a ExternalKeyTable) Save(ctx HasKVStore, key []byte, newValue Persistent) error

I know we discussed possible UX cons on the call today, but I think there are cases where this comes in handy and is worth the gas and disk storage savings. So my vote would be to just have it and let developers choose either NaturalKeyTable or ExternalKeyTable by weighing the tradeoffs themselves. I think for bank this is what I would want. Thoughts @ethanfrey and @alpe?

[alpe]

With the refactorings in #3 towards a more Gas efficient natural key table the underlying table struct can be easily used for storing objects under external keys without changes.

In this issue you have defined a new access interface though where a new Save is equivalent to an Upsert operation. I am wondering now if we

• accept the extra Gas for a store.Has(obj) or old = store.Get(obj) (1000 gas (30 optimized)) operation in the function in order to do the proper create or update switch for the indexes?
• can we call the method Set or Put instead to not conflict with the existing save?
• make this the same interface for natural key tables? (The AutoUInt64Table would still benefit from separate Create/ Save as the seq number is not known on create.

The simplest thing to do would be not adding a new interface but let the caller decide on create/save with his context. Maybe renaming Save to Update would be helpful.

[aaronc]

With the refactorings in #3 towards a more Gas efficient natural key table the underlying table struct can be easily used for storing objects under external keys without changes.

In this issue you have defined a new access interface though where a new Save is equivalent to an Upsert operation. I am wondering now if we

* accept the extra Gas for a `store.Has(obj)` or `old = store.Get(obj)` (1000 gas (30 optimized)) operation in the function in order to do the proper create or update switch for the indexes?

So which version would save gas? In general, I'm in favor of first optimizing for performance (which gas should be some indication of) and second gas, but in general we should be thinking of performance as a concern.

* can we call the method `Set` or `Put` instead to not conflict with the existing `save`?

How about Upsert?

* make this the same interface for natural key tables? (The AutoUInt64Table would still benefit from separate Create/ Save as the seq number is not known on create.

Well Save in natural key tables would have a different signature. My general preference would be for all tables to share a common interface TableBase and then differing on the Save and Delete function.

[alpe]

So which version would save gas

Separate "Create", "Save" methods are more efficient in terms of Gas than a single "Put". The Put would have to do the switch if store.Has(key) { update code } else { insert code}

[aaronc]

So which version would save gas

Separate "Create", "Save" methods are more efficient in terms of Gas than a single "Put". The Put would have to do the switch if store.Has(key) { update code } else { insert code}

But with an external key table, if you call create and there is an existing entry and you don't call store.Has, then you risk putting the database in a bad state because secondary indexes won't get cleaned up. It seems like Create is only safe to do without doing a Has check when the key is auto-generated.

[alpe]

It seems like Create is only safe to do without doing a Has check when the key is auto-generated.

Ja, I had documented this in the Table.Create function.

The NaturalKeyTable calls the Has() function before persisting via table.Create. In Weave the sequence based buckets were the most common use case. Therefore it made sense for me to optimize for this scenario and have the check not inside Table.Create. I also assumed that the caller has more context from the process and knows when to call create or save. The "Has() check" may have happened already by loading an entry for modifications or AuthZ.

With the External Key table the question is still do we need a new api or can we go with the current Table functions? In weave we had moved to a single Put in the ModelBucket for most scenarios but there were not extra Gas costs for the Has() call that we want to avoid here.

I think when we start building modules based on this more puristic ORM design we will evolve into new APIs as we go and learn

[aaronc]

Let's see how things go with the group module first. Also I wouldn't take the current gas costs as set in stone to be honest. I think performance should be a higher consideration and gas costs should be an accurate reflection of that. If they aren't we should document and raise an issue upstream.