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

Fixes #4832: Add migration script to changed id type from integer to #316

Conversation

VinceMacBuche
Copy link
Member

@@ -424,6 +425,18 @@ if [ $RES -eq 0 ]; then
psql -q -U rudder -h localhost -d rudder -f ${RUDDER_UPGRADE_TOOLS}/dbMigration-2.5-2.6-unexpanded-value.sql > /dev/null
fi

# Upgrade database schema from 2.6 to 2.6 if necessary - first part: transform id type in ruddersysevents table to bigint.
RES=$(su - postgres -c "psql -t -d rudder -c \"select data_type from information_schema.columns where table_name='ruddersysevents' and column_name='id';\"")
if [ "z$RES" == "z integer" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure abuot this space?

And can you please always refer to variables in bash using the ${x} syntax, not just $x?
(same question and request below)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of this; every time we needed to detect something we relied on counting; could we do the same ?
RES=$(su - postgres -c "psql -t -d rudder -c "select count(*) from information_schema.columns where table_name='ruddersysevents' and column_name='id' and data_type='integer'")

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure about the space, the result is ''' integer''' don't knwo where i got that damn space though ...)

Adding the curly braces, i was sure i added them ...

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems a good idea, thanks Nico !!

@VinceMacBuche
Copy link
Member Author

Thanks Nico and Jon, I made corrections on the PR (now compare with a integer, not a string anymore, fixes version numbers and add curly braces)

@jooooooon
Copy link
Member

OK, this code now looks fine.

However, I am a little concerned about what we're doing here: modifying the DB schema is not an anodine operation, should this really be happening in a patch release on a stable branch? What motivates this?

Also, if we need a BigInt, I'm assuming this will affect users that have a LOT of data. How long does this type change take?

@VinceMacBuche
Copy link
Member Author

hmm... the convertion is almost instant on postgres 9.1+ but can be very long on older versions : http://blog.endpoint.com/2012/11/postgres-alter-column-problems-and.html

@VinceMacBuche
Copy link
Member Author

on postgres 9.1 we can use that command ...

on older ... alter table lock the database and rewrite the whole table, which can be very long. There is another way to do this, but it's quite dangerous ..

It needs to modify directly the type in postgres internal databases (table pg_attribute) instead of using postgres basic command ...

@VinceMacBuche
Copy link
Member Author

Ok, i misread ... it will always be long to use 'alter table ... set type bigint' since it has to rewite table ... It's only on certian type (varchar to text )... However i cant find the whole text explaining this ...

@VinceMacBuche
Copy link
Member Author

faster ways are :

  • dump the table, truncate it, modify the column, import datas
  • crete a colmumn '''newid''' of type bigint, copy all datas from '''id''' to '''newid''' , drop '''id', rename '''newid''' to '''id'''

@VinceMacBuche
Copy link
Member Author

Jon, we found another way to do it:

We decided to do:
In a transaction:
Rename table to old_table
drop all indexes
create new "table" with bigint
Create new indexes
End of transaction
Copy all lines of "old_table" int "table"
Drop "old_table"

That process can still be long (insertion of million of lines can take some times), but this will not block the database ...

But still the question is a stake if we need to do this in 2.6 or 2.11 ...

I think this is quite important, since we can't force people with big installation to not use a stable version ...

@VinceMacBuche
Copy link
Member Author

Closing that pull request, becasue of two things:

First: we decided to move it in 2.10
Secondly: We decided to not provide a migration script that will automatically do the migration, but a script that should run manually due to the huge impact. In the migration script we should only add a Warning to alert eh user oon the need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants