DM-16227: Move collection integrity constraint into DB and fix transactions #117

TallJimbo · 2019-01-15T21:59:26Z

No description provided.

andy-slac

Looks good, few minor comments.

andy-slac · 2019-01-15T23:46:19Z

python/lsst/daf/butler/core/dimensions/dataId.py

+            elif isinstance(v, str):
+                message.update(v.encode('utf8'))
+            elif isinstance(v, datetime.datetime):
+                message.update(v.isoformat().encode('utf8'))


Does isoformat make unambiguous representation (I'm thinking about timezones)? Would it be easier to just take number of seconds in UTC?

I think it does, but I'm not certain, and I didn't see an easy way to get number of seconds in UTC from Python datetime. In any case, I expect this all to get fixed in DM-15890, which will switch all of the datetimes we use in the database to TAI MJD to avoid these kinds of problems entirely.

andy-slac · 2019-01-15T23:49:54Z

python/lsst/daf/butler/core/dimensions/dataId.py

+            argument to update the hash.
+        """
+        for k, v in self.items():
+            message.update(k.encode('utf8'))


utf-8 is the default for encode(), maybe just drop it rather than repeat everywhere?

andy-slac · 2019-01-15T23:52:04Z

python/lsst/daf/butler/core/registry.py

+        """Add existing Datasets to a collection, implicitly creating the
+        collection if it does not already exist.
+
+        If a DatasetRef with the same exact `dataset_id`` is already in a


One more backtick on the left

andy-slac · 2019-01-15T23:59:42Z

python/lsst/daf/butler/core/schema.py

+            return None
+        if not isinstance(value, bytes):
+            raise TypeError(f"Base64Bytes fields require 'bytes' values; got {value}")
+        return b64encode(value).decode('utf8')


Nitpick - base64 is guaranteed to make ascii bytes

andy-slac · 2019-01-16T00:26:16Z

config/schema.yaml

@@ -13,6 +13,7 @@ schema:
      -
        name: dataset_type_name
        type: string
+        length: 128


For future extension I'd probably suggest doing something like:

type: string typeKwargs: {length: 128}

but with just two of these new keywords this maybe an overkill. Can wait until we add third 🙂

I'd prefer to just get the schema definition out of YAML (DM-17154) and into direct SQLAlchemy Python code; we're on the road to trying to map the full complexity of SQLAlchemy schema definition to a home-brew YAML format, and I think we need to get off that road, especially before getting serious about an Oracle implementation.

andy-slac · 2019-01-16T17:25:03Z

config/schema.yaml

+        nbytes: 32
+        nullable: false
+        doc: >
+          Secture hash of the data ID (i.e. dimension link values) and


andy-slac · 2019-01-16T17:27:47Z

config/schema.yaml

        primary_key: true
        doc: >
          Name of the Instrument with which this filter is associated.
      -
        name: physical_filter
        type: string
+        length: 8


Is 8 characters enough for everyone?

Most filter names are single characters, and physical filters may contain instrument prefixes or version suffixes, that'll still be pretty small. That said, maybe the size of the instrument field + 8 would be better (i.e. 16).

andy-slac · 2019-01-16T17:39:32Z

python/lsst/daf/butler/core/registry.py

+
+        If a DatasetRef with the same exact `dataset_id`` is already in a
+        collection nothing is changed. If a `DatasetRef` with the same
+        `DatasetType1` and dimension values but with different ``dataset_id``


DatasetType1?

andy-slac · 2019-01-16T17:45:45Z

python/lsst/daf/butler/core/schema.py

+    impl = String
+
+    def __init__(self, nbytes, *args, **kwds):
+        length = 4*ceil(nbytes/3)


Should it divide by 4?

I don't think so: https://stackoverflow.com/questions/13378815/base64-length-calculation

Ok, I was confused by what nbytes mean - I though it is stored data size. Never mind.

andy-slac · 2019-01-16T17:47:44Z

python/lsst/daf/butler/core/schema.py

+        if value is None:
+            return None
+        if not isinstance(value, bytes):
+            raise TypeError(f"Base64Bytes fields require 'bytes' values; got {value}")


Maybe type(value)?

I'll include both the value and its type.

andy-slac

I think latest associate() update should be OK for sqlite, for non-sqlite backends we probably need different strategy.

andy-slac · 2019-01-25T17:56:30Z

python/lsst/daf/butler/registries/sqlRegistry.py

+                        ref.datasetType, ref.dataId, collection
+                    )
+                )
+            # If the same Dataset is already in this collection, do nothing.


I suppose this (select+check+insert) will work OK for SQLite because SQLite locks the whole database for the duration of transaction. For other backends which support concurrent transactions I think there is a race condition - if two clients do select at the same instant they both get empty set and will try to insert (one of them will fail).
I think more or less portable strategy for this kind of update is to just try to insert+commit and analyse DuplicateKey error that could happen on commit.
Some backends support select ... for update for row-level locking (and index gap locking) but it also needs special care so I would not recommend that as portable fix.

Interesting - I think this indicates a fundamental misunderstanding by me on what transactions guarantee in general, which means I need to spend some time thinking about what those guarantees actually are.

I don't think "analyzing the DuplicateKey error" approach can really be done portably, either, because we don't have any guarantee about what kind of information the error message might contain (and we need to know which constraint failed), so ultimately I think this (and several other Registry methods) need to be left pure-abstract in SqlRegistry so they must be implemented by DB-specific derived classes.

As I said on Jira, I think it's best not to do that now because (for now) a solution that's subject to race conditions is better than no solution at all. I'll be sure to time the future changes carefully with NCSA to make sure we don't have a regression in Oracle support at an awkward time.

By analyzing I meant that you would need to select from database at that point, after failure. Relying on exception content is not possible of course.

andy-slac · 2019-01-25T18:13:36Z

python/lsst/daf/butler/registries/sqlRegistry.py

+                                           "collection": collection} for ref in refs])
+            elif row.dataset_id != ref.id:
+                # A different Dataset with this DatasetType and Data ID already
+                # exist in this collection.


This has been fully supplanted by SqlRegistryDatabaseDict, which handles transactions better.

It seems the SQLite Python module doesn't actually begin transactions when you ask it to; this can be worked around via SQLAlchemy event listeners that emit BEGIN statements directly. Recipe is from the SQLAlchemy docs: https://docs.sqlalchemy.org/en/latest/dialects/sqlite.html In order to make that work, we also need to make sure all Registry operations go through a single connection (i.e. disable SQLAlchemy's connection pooling and don't call engine.connect() multiple times). That's actually the right thing to do given our usage pattern for all DBs.

This was always part of the SqlRegistry implementation, and should not have been considered part of the core butler package.

andy-slac approved these changes Jan 16, 2019

View reviewed changes

TallJimbo force-pushed the tickets/DM-16227 branch 2 times, most recently from e7124ac to 40a530b Compare January 25, 2019 01:06

andy-slac approved these changes Jan 25, 2019

View reviewed changes

TallJimbo force-pushed the tickets/DM-16227 branch from de1cc8d to e85c780 Compare January 25, 2019 19:16

TallJimbo added 12 commits January 25, 2019 16:52

Support type length options in schema creation.

eeeb84b

Add type lengths for all string fields.

7b45da4

Add custom base64-encoded type for hashes.

37fd7d1

Add support for unique constraints in schema definition.

211ff50

Add DatasetRef hash and constraints utilizing it to Registry.

248da77

Remove SqlDatabaseDict.

a9bfac1

This has been fully supplanted by SqlRegistryDatabaseDict, which handles transactions better.

Move SqlRegistryDatabaseDict to registries subpackage.

b92f1be

This was always part of the SqlRegistry implementation, and should not have been considered part of the core butler package.

Fix nested transaction handling and remove old workarounds.

80d2dc8

Clean up associate docs.

760ba2a

Remove duplicate physical_filter field in Visit.

83abf37

Reimplement associate to use DatasetRef hash field.

a806dc4

TallJimbo force-pushed the tickets/DM-16227 branch from e85c780 to a806dc4 Compare January 25, 2019 21:54

TallJimbo merged commit eed5b55 into master Jan 25, 2019

TallJimbo deleted the tickets/DM-16227 branch January 25, 2019 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-16227: Move collection integrity constraint into DB and fix transactions #117

DM-16227: Move collection integrity constraint into DB and fix transactions #117

TallJimbo commented Jan 15, 2019

andy-slac left a comment

andy-slac Jan 15, 2019

TallJimbo Jan 18, 2019 •

edited

andy-slac Jan 15, 2019

andy-slac Jan 15, 2019

andy-slac Jan 15, 2019

andy-slac Jan 16, 2019

TallJimbo Jan 18, 2019 •

edited

andy-slac Jan 16, 2019

andy-slac Jan 16, 2019

TallJimbo Jan 18, 2019

andy-slac Jan 16, 2019

andy-slac Jan 16, 2019

TallJimbo Jan 18, 2019

andy-slac Jan 18, 2019

andy-slac Jan 16, 2019

TallJimbo Jan 18, 2019

andy-slac left a comment

andy-slac Jan 25, 2019

TallJimbo Jan 25, 2019

andy-slac Jan 25, 2019

andy-slac Jan 25, 2019

DM-16227: Move collection integrity constraint into DB and fix transactions #117

DM-16227: Move collection integrity constraint into DB and fix transactions #117

Conversation

TallJimbo commented Jan 15, 2019

andy-slac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Jan 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Jan 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-slac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Jan 18, 2019 •

edited

TallJimbo Jan 18, 2019 •

edited