Conversation
| var flipKickModel event.InsertionModel | ||
|
|
||
| BeforeEach(func() { | ||
| flipKickModel = test_data.FlipKickModel() |
There was a problem hiding this comment.
Kind of annoying how we can't do this on the same line where we invoke the shared suite, but the problem is that there are some Expects in the CopyModel function, which break if they're not called from within an It or setup/teardown function.
| (SELECT get_latest_flip_bid_tab(NEW.bid_id)), | ||
| INTO maker.flip (bid_id, address_id, block_number, guy, tic, "end", lot, bid, gal, tab, updated, created) | ||
| VALUES (NEW.bid_id, NEW.address_id, (SELECT block_number FROM diff_block), NEW.guy, | ||
| get_latest_flip_bid_tic(NEW.bid_id), |
There was a problem hiding this comment.
Maybe I should have done some of this reformatting/refactoring in a separate commit 😅 but the only behavior change in all these insert_updated_x trigger functions are cascading changes from updating the flip table. I.e. removing block_hash and updating the PK.
rmulhol
left a comment
There was a problem hiding this comment.
Looks awesome!
Just to make sure I understand - this story means that we can have events/storage diffs arrive out of order and preserve data consistency, but we need additional work to enable reorg safety. Is that right? Also, do we need a separate story to gracefully handle diffs arriving out of order (e.g. if we got diffs from block 7, 1, 5, 3 - in that order)?
| ON CONFLICT (bid_id, block_number) DO UPDATE SET guy = NEW.guy; | ||
| return NEW; | ||
| flip_bid_time_created(NEW.address_id, NEW.bid_id)) | ||
| ON CONFLICT (block_number, bid_id, address_id) DO UPDATE SET guy = NEW.guy; |
| return NEW; | ||
| flip_bid_time_created(NEW.address_id, NEW.bid_id)) | ||
| ON CONFLICT (block_number, bid_id, address_id) DO UPDATE SET lot = NEW.lot; | ||
| RETURN NEW; |
There was a problem hiding this comment.
Just curious - why is RETURN NEW necessary? Seems like this function could just return void
There was a problem hiding this comment.
According to the docs
A trigger function must return either NULL or a record/row value having exactly the structure of the table the trigger was fired for.
So RETURN NULL would work just as well here (though I don't think it would work to omit the return entirely). I don't know of any reason to pick one over the other, so I randomly settled on NEW. 🤷♂
| RETURN NEW; | ||
| END | ||
| $$ | ||
| LANGUAGE plpgsql; |
There was a problem hiding this comment.
Seems like this is a legit use of plpgsql (and not added by this PR), but I was mildly surprised to see that sql is preferred for performance - just dropping here for knowledge sharing's sake
| func TestTriggers(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "Trigger Suite") | ||
| } |
There was a problem hiding this comment.
Does this suite emit any logs when you run make test from the cli?
There was a problem hiding this comment.
Now that I look again, I guess event.PersistModels can emit logs under the right conditions. I don't think we're likely to encounter those conditions, but I guess I might as well make sure they get discarded just in case
| }) | ||
|
|
||
| It("generates log note signature", func() { | ||
| It("generates log value signature", func() { |
|
@rmulhol Yeah exactly, this PR does not include the changes that will allow diffs to come in out of order-- that's covered by VDB-1144, which is in progress right now. |
6c09602 to
a28c6fd
Compare
This PR removes the implicit assumption that
kickevents will arrive before storage diffs. Now the data should stay up-to-date regardless of whether events or diffs are transformed first (though there is still a dependency on diffs arriving in order).