This repository has been archived by the owner. It is now read-only.

use id as primary key instead of username #835

Closed
chadwhitacre opened this Issue Apr 11, 2013 · 25 comments

Comments

Projects
None yet
7 participants
@chadwhitacre
Contributor

chadwhitacre commented Apr 11, 2013

We're adopting a zero-downtime approach (#3864 (comment)) here. Here are the basic steps for migrating from a column foo to foo_id without having to turn on maintenance mode:

Step branch.sql Code changes to ... Deploy which first?
1 add a new foo_id column write to both foo and foo_id branch.sql
2 backfill foo_id based on foo read from foo_id branch.sql
3 drop foo no longer write to foo code changes
After step ... We are writing to ... We are reading from ...
0 foo foo
1 foo and foo_id foo
2 foo and foo_id foo_id
3 foo_id foo_id

Todo

  • elsewhere
    • Writes
    • Reads
    • Dropped old column
  • payment_instructions
  • payments
    • Writes
    • Reads
    • Dropped old column
  • exchanges
    • Writes
    • Reads
    • Dropped old column
  • absorptions
    • Writes
    • Reads
    • Dropped old column
  • takes
    • Writes
    • Reads
    • Dropped old column
  • tips (can we just drop this table entirely?)
  • teams.owner
    • Writes
    • Reads
    • Dropped old column
  • emails

See #503 and #287.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Apr 11, 2013

Contributor

I'd be surprised if there weren't an exploitable race condition hidden in there somewhere.

Contributor

chadwhitacre commented Apr 11, 2013

I'd be surprised if there weren't an exploitable race condition hidden in there somewhere.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Apr 13, 2013

Contributor

What if we keep the username as the foreign key in the db but add joins and constraints on id when modifying data? We can either do joins when inspecting the db interactively (pita) or in the application when modifying the data (complicates the application somewhat).

Contributor

chadwhitacre commented Apr 13, 2013

What if we keep the username as the foreign key in the db but add joins and constraints on id when modifying data? We can either do joins when inspecting the db interactively (pita) or in the application when modifying the data (complicates the application somewhat).

@zwn

This comment has been minimized.

Show comment
Hide comment
@zwn

zwn Sep 29, 2013

Contributor

Why do we want it?

Contributor

zwn commented Sep 29, 2013

Why do we want it?

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Sep 30, 2013

Contributor

I don't have a big problem right now with using username as the foreign key through the database. I could imagine reasons related to sharding at scale but we're not there yet. I'm closing this ticket. If someone cares about this we can reopen.

Contributor

chadwhitacre commented Sep 30, 2013

I don't have a big problem right now with using username as the foreign key through the database. I could imagine reasons related to sharding at scale but we're not there yet. I'm closing this ticket. If someone cares about this we can reopen.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
Contributor

chadwhitacre commented Apr 2, 2015

IRC

@chadwhitacre chadwhitacre reopened this Apr 2, 2015

@chadwhitacre chadwhitacre added the TeamX label Apr 2, 2015

@Changaco

This comment has been minimized.

Show comment
Hide comment
@Changaco

Changaco Apr 3, 2015

Contributor

We currently have 7 tables referencing username (elsewhere, tips, transfers, exchanges, absorptions, takes, emails) and 5 tables referencing id (exchange_routes, community_members, statements, email_queue, balances_at).

Contributor

Changaco commented Apr 3, 2015

We currently have 7 tables referencing username (elsewhere, tips, transfers, exchanges, absorptions, takes, emails) and 5 tables referencing id (exchange_routes, community_members, statements, email_queue, balances_at).

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Apr 3, 2015

Closed

let's talk about process #163

@chadwhitacre chadwhitacre changed the title from use id as pk instead of username to use id as primary key instead of username Aug 29, 2015

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Aug 29, 2015

Contributor

I'd be surprised if there weren't an exploitable race condition hidden in there somewhere.

There are certainly race conditions, because our web app process(es) is (are) multi-threaded. A Participant object on one thread doesn't know about username changes in the database. Yes, at the database level we shouldn't have any race conditions, but we do at the web app level.

Contributor

chadwhitacre commented Aug 29, 2015

I'd be surprised if there weren't an exploitable race condition hidden in there somewhere.

There are certainly race conditions, because our web app process(es) is (are) multi-threaded. A Participant object on one thread doesn't know about username changes in the database. Yes, at the database level we shouldn't have any race conditions, but we do at the web app level.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Aug 29, 2015

Contributor

Example: if one thread is changing a slug, and another thread is updating a payment instruction based on the slug, then the payment instruction will fail with an error because it won't find a Team with that slug, or—what's worse—it will silently fail by assigning the payment instruction to a Team that has managed to "steal" the old slug of the Team that is changing slugs.

Contributor

chadwhitacre commented Aug 29, 2015

Example: if one thread is changing a slug, and another thread is updating a payment instruction based on the slug, then the payment instruction will fail with an error because it won't find a Team with that slug, or—what's worse—it will silently fail by assigning the payment instruction to a Team that has managed to "steal" the old slug of the Team that is changing slugs.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Aug 29, 2015

Contributor

Upgrading to TeamX ★.

Contributor

chadwhitacre commented Aug 29, 2015

Upgrading to TeamX ★.

@chadwhitacre chadwhitacre added TeamX ★ and removed TeamX labels Aug 29, 2015

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Oct 27, 2015

Closed

Radar 30 #383

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Nov 17, 2015

Closed

Radar 33 #402

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Dec 4, 2015

Closed

onboard @sammyshj #424

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Dec 4, 2015

Contributor

@sammyshj Looking at #835 (comment), maybe emails would be a good table to start with? Want to put together a PR changing emails.participant to reference participants.id instead of .username?

Contributor

chadwhitacre commented Dec 4, 2015

@sammyshj Looking at #835 (comment), maybe emails would be a good table to start with? Want to put together a PR changing emails.participant to reference participants.id instead of .username?

@sammyshj

This comment has been minimized.

Show comment
Hide comment
@sammyshj

sammyshj Dec 4, 2015

@whit537 Yes, working on it right away!

sammyshj commented Dec 4, 2015

@whit537 Yes, working on it right away!

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Dec 4, 2015

Contributor

@sammyshj Here's our high-level description:

We write SQL, specifically the PostgreSQL variant. We keep our database schema in schema.sql, and we write schema changes for each PR branch in a sql/branch.sql file, which then gets run against production and merged into sql/schema.sql during deployment.

So, your PR should at least have a sql/branch.sql file. When you run the test suite you may find parts of the application that break due to the schema change, so your PR should address those issues as well.

Is that enough to get you started? What other questions can we answer?

Contributor

chadwhitacre commented Dec 4, 2015

@sammyshj Here's our high-level description:

We write SQL, specifically the PostgreSQL variant. We keep our database schema in schema.sql, and we write schema changes for each PR branch in a sql/branch.sql file, which then gets run against production and merged into sql/schema.sql during deployment.

So, your PR should at least have a sql/branch.sql file. When you run the test suite you may find parts of the application that break due to the schema change, so your PR should address those issues as well.

Is that enough to get you started? What other questions can we answer?

@sammyshj

This comment has been minimized.

Show comment
Hide comment
@sammyshj

sammyshj Dec 4, 2015

Thanks for pointing out these things. If I face any problems later on regarding this issue, I will let you know.

sammyshj commented Dec 4, 2015

Thanks for pointing out these things. If I face any problems later on regarding this issue, I will let you know.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Dec 4, 2015

Contributor

Awesome, thanks @sammyshj! :-)

!m @sammyshj ← that's our goofy way of saying, "You're doing a good job!" :)

Contributor

chadwhitacre commented Dec 4, 2015

Awesome, thanks @sammyshj! :-)

!m @sammyshj ← that's our goofy way of saying, "You're doing a good job!" :)

@sammyshj

This comment has been minimized.

Show comment
Hide comment
@sammyshj

sammyshj Dec 4, 2015

@whit537 Currently emails.participant refers to participants, https://github.com/gratipay/gratipay.com/blob/master/sql/schema.sql#L320
Can you point out where it refers to participants.username?

sammyshj commented Dec 4, 2015

@whit537 Currently emails.participant refers to participants, https://github.com/gratipay/gratipay.com/blob/master/sql/schema.sql#L320
Can you point out where it refers to participants.username?

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Dec 4, 2015

Contributor

It references the participants table, and without an explicit column specified, it references the primary key, which is username. We're not ready to change the primary key on participants yet.

Contributor

chadwhitacre commented Dec 4, 2015

It references the participants table, and without an explicit column specified, it references the primary key, which is username. We're not ready to change the primary key on participants yet.

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Dec 7, 2015

Closed

Radar 36 #429

@chadwhitacre chadwhitacre referenced this issue in gratipay/inside.gratipay.com Dec 28, 2015

Closed

Radar 39 #452

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Jun 15, 2016

Contributor

@kaguillera and I added some notes in the description about how to perform these changes with zero downtime.

Contributor

chadwhitacre commented Jun 15, 2016

@kaguillera and I added some notes in the description about how to perform these changes with zero downtime.

@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
Contributor

chadwhitacre commented Jun 15, 2016

@gratipay-bot gratipay-bot referenced this issue in gratipay/inside.gratipay.com Aug 14, 2016

Closed

Tech Radar 17 #772

@gratipay-bot gratipay-bot referenced this issue in gratipay/inside.gratipay.com Aug 21, 2016

Closed

Tech Radar 18 #783

@gratipay-bot gratipay-bot referenced this issue in gratipay/inside.gratipay.com Oct 2, 2016

Closed

Core Radar 79 #840

@gratipay-bot gratipay-bot referenced this issue in gratipay/inside.gratipay.com Nov 6, 2016

Closed

Core Radar 84 #884

@kaguillera kaguillera referenced this issue May 25, 2017

Closed

Clean up exchanges #4442

6 of 15 tasks complete

@chadwhitacre chadwhitacre referenced this issue Jun 13, 2017

Merged

Add bulk claiming at /on/npm/ #4488

13 of 13 tasks complete
@chadwhitacre

This comment has been minimized.

Show comment
Hide comment
@chadwhitacre

chadwhitacre Nov 2, 2017

Contributor

Closing in light of our decision to shut down Gratipay.

Thank you all for a great run, and I'm sorry it didn't work out! 😞 💃

Contributor

chadwhitacre commented Nov 2, 2017

Closing in light of our decision to shut down Gratipay.

Thank you all for a great run, and I'm sorry it didn't work out! 😞 💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.