switch to {participant,team}_id in payment_instructions

[chadwhitacre]

Part of #835.

[chadwhitacre]

Ready for review @rohitpaulk @aandis.

[chadwhitacre]

@kaguillera and I used #3893 as a pattern here.

[chadwhitacre]

@kaguillera and I used #3893 as a pattern here.

But we ended up doing it all in one PR instead of three. Ready for review @rohitpaulk @aandis! :-)

[aandis]

This is going to require some downtime on prod so that while this migration runs, no one creates a new payment_instruction or runs into error while creating them. I am not sure how big our prod payment_instructions table is, which would decide how long this migration takes. Is that an acceptable downtime @whit537 ?

[aandis]

#3864 (comment) for reference

[aandis]

Can we club the two UPDATEs into a single UPDATE statement so that this takes lesser time?

[chadwhitacre]

This is going to require some downtime on prod so that while this migration runs, no one creates a new payment_instruction or runs into error while creating them.

I suppose that's a reason to split this across multiple PRs.

I am not sure how big our prod payment_instructions table is, which would decide how long this migration takes.

It's small:

gratipay::MAROON=> select count(*) from payment_instructions;
┌───────┐
│ count │
├───────┤
│  3796 │
└───────┘
(1 row)

(The old tips table from Gittipay 1.0 has 82,911 records, but we're not dealing with that here.)

Testing on a local backup, I'm seeing < 2 seconds to apply branch.sql:

[gratipay] $ time psql gratipay-bak < sql/branch.sql 
Null display is "¤".
Line style is unicode.
Border style is 2.
BEGIN
ALTER TABLE
ALTER TABLE
DROP VIEW
UPDATE 3787
UPDATE 3787
ALTER TABLE
ALTER TABLE
ALTER TABLE
ALTER TABLE
CREATE VIEW
CREATE TRIGGER
COMMIT

real    0m1.695s
user    0m0.005s
sys     0m0.006s
[gratipay] $

Is that an acceptable downtime @whit537 ?

I could go either way. Based on the above test, it looks like the downtime is going to be very brief, but I could also see aiming for zero downtime in the interest of professional pride. :-)

[chadwhitacre]

Can we club the two UPDATEs into a single UPDATE statement so that this takes lesser time?

Done in 7a0c2d9, and it looks like indeed it's twice as fast (even though, for simplicity, it uses a subselect instead of a FROM):

[gratipay] $ time psql gratipay-bak < sql/branch.sql
Null display is "¤".
Line style is unicode.
Border style is 2.
BEGIN
ALTER TABLE
ALTER TABLE
DROP VIEW
UPDATE 3787
ALTER TABLE
ALTER TABLE
ALTER TABLE
ALTER TABLE
CREATE VIEW
CREATE TRIGGER
COMMIT

real    0m0.827s
user    0m0.005s
sys     0m0.005s
[gratipay] $

[aandis]

Removed usage from some more places. Not sure if we still use some of those.

[chadwhitacre]

@aandis Cool. Looks like we need some better test coverage tho. ;-)

[chadwhitacre]

... because the way @kaguillera and I approached this was to make the schema change, and then fix whatever the test suite surfaced. The fact that the tests didn't surface the cases you found indicates that we have an opportunity to improve our test coverage here.

[chadwhitacre]

The fact that the tests didn't surface the cases you found indicates that we have an opportunity to improve our test coverage here.

bin/migrate-tips.py coverage handled in #4049, which is against master (we'll need to rebase here once that lands).

[chadwhitacre]

Hack close to try restarting the build. Can't see how to do it in the Travis UI. 😕

[chadwhitacre]

Rebased on master to pick up #4049 and insert 355b271. Previous head was 5badb96 (though 62f6f9d includes some additional fixes caught by the tests in 355b271). Old history showing previous Travis results:

[chadwhitacre]

Okay! @kaguillera and I added some tests in #4049 and 355b271 to cover the additional fixes (now) in 62f6f9d. Ready for re-review here, @aandis @rohitpaulk! 😤

[chadwhitacre]

Rebased on master. Previous head was 62f6f9d.

[aandis]

Rebased on master. Previous head was f00ddfe

[aandis]

Looks good to me.

[aandis]

I'll leave it to @whit537 or @rohitpaulk to merge, deploy and test this on prod in one go. Let's make sure deployment is done right. Run the migration first and then push application changes.

[rohitpaulk]

We should also turn maintenance mode on when doing this. (Our release.sh script supports this)

@whit537 - Can you have a look at Google Analytics Piwik and figure out what time of the day we usually receive the least traffic at?

[chadwhitacre]

Can you have a look at Google Analytics Piwik and figure out what time of the day we usually receive the least traffic at?

Right. Soooooooo ... we don't have Piwik on right now either (gratipay/inside.gratipay.com#437). 😁

[chadwhitacre]

Rebased on master. Previous head was 4f303d4.

[chadwhitacre]

Alright, @kaguillera and I have digested and taken on board the desired approach of zero-downtime migrations at #3864 (comment) (and the links following). We're now planning to split this into a multi-step migration as recommended there and as implemented in #3893.