Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

single sequence for all metrics sub tables #48

Merged
merged 1 commit into from
Oct 21, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 9, 2017

Before:
Every metrics_** table had a different sequence.
So inserting directly into a subtable would result in overlapping id values.
We currently insert into the base metrics table, so this is not a problem.

 Column |  Type  |                           Modifiers                            
--------+--------+---------------------------------------------------------------
 id     | bigint | not null default nextval('metric_rollups_01_id_seq'::regclass)

After:
Everymetrics subtable uses the same sequence.
So inserting into any of the metrics tables will get a unique id.

 Column |  Type  |                           Modifiers                            
--------+--------+---------------------------------------------------------------
 id     | bigint | not null default nextval('metric_rollups_id_seq'::regclass)

Rollback does bring back to the previous value

 Column |  Type  |                           Modifiers                            
--------+--------+---------------------------------------------------------------
 id     | bigint | not null default nextval('metric_rollups_01_id_seq'::regclass)

This change allows us to insert directly into the subtable, which is 10x faster than inserting into the common parent table.

/cc @chrisarcand @Fryguy

@chrisarcand
Copy link
Member

Seems ok to me.

@Fryguy
Copy link
Member

Fryguy commented Aug 9, 2017

Can you also drop all of the individual sequences? metrics_##_id_seq and metrics_rollups_##_id_seq

Also, since this PR doesn't change the insertion into the main table, I'm assuming that still works as well?

@chessbyte
Copy link
Member

@kbrock @Fryguy what is the status of this PR?

@kbrock
Copy link
Member Author

kbrock commented Oct 6, 2017

@chessbyte thanks. I had thought this was merged already - will rebase and push again

@Fryguy
Copy link
Member

Fryguy commented Oct 9, 2017

@kbrock Can you address #48 (comment) ?

@kbrock
Copy link
Member Author

kbrock commented Oct 20, 2017

ugh. thanks @Fryguy

  1. drop and add sequences.
  2. yes inserts into the primary table work as it did before
  3. fixed cops & code climate duplicate warnings.

Before: there was a different sequence for each metrics_** table
After: all use the same sequence. so you can insert into any table
@miq-bot
Copy link
Member

miq-bot commented Oct 20, 2017

Checked commit kbrock@130d14c with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@Fryguy Fryguy merged commit e8b378a into ManageIQ:master Oct 21, 2017
@Fryguy Fryguy added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 21, 2017

def down
change_sequences("metrics", 0..23, true)
change_sequences("metric_rollups", 1..12, true)
Copy link
Member

@Fryguy Fryguy Oct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock I just realized that on the down migration I think you also have to update the nextval to the highest id in the table or you'll get duplicates.

Copy link
Member Author

@kbrock kbrock Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the old code, we can not create a record in the old tables.
The sequences will overlap.

So we have never created a record there.

Yes we will reset the sequence back to 1. But since they haven't been used up until now, I think they are all 1 anyway.
And if they weren't, then that would be kinda bad - it would mean we have bad data in our metrics tables

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhh I see. The only other thing we might want to do is ensure we set the value to the "min value for the region" as opposed to 1. If that happens automatically, then we are good.

@kbrock kbrock deleted the sequence_ids branch October 23, 2017 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants