Conversation
Signed-off-by: Tisham Dhar <tisham.dhar@ga.gov.au>
Signed-off-by: Tisham Dhar <tisham.dhar@ga.gov.au>
Codecov Report
@@ Coverage Diff @@
## develop #951 +/- ##
===========================================
+ Coverage 92.96% 92.98% +0.01%
===========================================
Files 96 97 +1
Lines 9630 9648 +18
===========================================
+ Hits 8953 8971 +18
Misses 677 677
Continue to review full report at Codecov.
|
jeremyh
left a comment
There was a problem hiding this comment.
Nice work 👍
Minor optional comment below
|
|
||
| # Post 1.8 DB Federation triggers | ||
| from datacube.drivers.postgres._triggers import install_timestamp_trigger | ||
| _LOG.info("Adding Update Triggers") |
There was a problem hiding this comment.
This is going to log every time they run init, even if they already have applied this update.
By convention, I usually wrapped them in a check previously:
if not pg_column_exists(engine, schema_qualified('dataset'), 'updated'):
_LOG.info("Adding Update Triggers")
with engine.connect() as c:
c.execute('begin')
install_timestamp_trigger(c)
.... etc(of course, no need to check other columns exist as they're all applied together in one transaction.)
The actual SQL looks idempotent so this is all an extremely minor gripe.
|
|
||
| for name in TABLE_NAMES: | ||
| # Add update_at columns | ||
| # HACK: Make this more SQLAlchemy with add_column on Table objects |
There was a problem hiding this comment.
For these sort of db-specific changes I prefer raw sql. Not a hack in my view 🙂 (it's also what we did in the past)
|
I'm not familiar with DB side of things, so can't really review in a meaningfull way. But what does this mean from user point of view?
|
|
Kirill888
left a comment
There was a problem hiding this comment.
If it's just for init then write permissions are assumed anyway
Reason for this pull request
Add updated column + triggers to core postgresql tables to ease incremental Datacube federation.
Proposed changes
Add
updatedcolumn to all tablesAdd triggers on these columns to capture update query timestamps.
Closes Datacube Database Federation and ETL using timestamps #947
Tests added / passed
Fully documented, including
docs/about/whats_new.rstfor all changes