)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":true,"context_lines":[{"line_number":15,"context_line":"Note that we don\u0027t actually resolve all of the mismatches. A large"},{"line_number":16,"context_line":"number of these mismatches were caused by the addition of"},{"line_number":17,"context_line":"\u0027nullable\u003dFalse\u0027 to a column in a main table but not the equivalent"},{"line_number":18,"context_line":"shadow table. With the benefit of hindsight, this is probably wise and"},{"line_number":19,"context_line":"possibly even intended. If we were to apply the new constraint to the"},{"line_number":20,"context_line":"shadow table also, we\u0027d need a data migration to populate any remaining"},{"line_number":21,"context_line":"NULL fields in the shadow tables. This data migration could be very"},{"line_number":22,"context_line":"costly and may not even possible as we don\u0027t necessarily have all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"94979a2a_29d6402d","line":19,"range":{"start_line":18,"start_character":14,"end_line":19,"end_character":23},"updated":"2021-10-11 12:13:42.000000000","message":"i would guess it was not added to the shadow table because in the main db we did not want to allow null but in the shadow tabels its possible we had instance that had the colume set to null  so we did not apply it or did not want to have the performace overhead of making this change on a large db with lot of rows in the shadow tables.","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4454ef218ff4e92b8a91b0edae25ad9b6960c0f7","unresolved":false,"context_lines":[{"line_number":15,"context_line":"Note that we don\u0027t actually resolve all of the mismatches. A large"},{"line_number":16,"context_line":"number of these mismatches were caused by the addition of"},{"line_number":17,"context_line":"\u0027nullable\u003dFalse\u0027 to a column in a main table but not the equivalent"},{"line_number":18,"context_line":"shadow table. With the benefit of hindsight, this is probably wise and"},{"line_number":19,"context_line":"possibly even intended. If we were to apply the new constraint to the"},{"line_number":20,"context_line":"shadow table also, we\u0027d need a data migration to populate any remaining"},{"line_number":21,"context_line":"NULL fields in the shadow tables. This data migration could be very"},{"line_number":22,"context_line":"costly and may not even possible as we don\u0027t necessarily have all"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"0327697f_7387b7d7","line":19,"range":{"start_line":18,"start_character":14,"end_line":19,"end_character":23},"in_reply_to":"94979a2a_29d6402d","updated":"2021-10-12 08:26:13.000000000","message":"\u003e i would guess it was not added to the shadow table because in the main db we did not want to allow null but in the shadow tabels its possible we had instance that had the colume set to null  so we did not apply it or did not want to have the performace overhead of making this change on a large db with lot of rows in the shadow tables.\n\nYeah, that\u0027s exactly what I said in the commit messsage 😜:\n\n\u003e Note that we don\u0027t actually resolve all of the mismatches. A large\n\u003e number of these mismatches were caused by the addition of\n\u003e \u0027nullable\u003dFalse\u0027 to a column in a main table but not the equivalent\n\u003e shadow table. With the benefit of hindsight, this is probably wise and\n\u003e possibly even intended. If we were to apply the new constraint to the\n\u003e shadow table also, we\u0027d need a data migration to populate any remaining\n\u003e NULL fields in the shadow tables. This data migration could be very\n\u003e costly and may not even possible as we don\u0027t necessarily have all\n\u003e available context required to do this population. As a result, the\n\u003e wording around these differences is changed to indicate that we will\n\u003e likely never address these \"bugs\".","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"6507bd1a_39f2cc7a","updated":"2021-10-11 12:13:42.000000000","message":"+1 im not entirely conviced we  should remove the indexes they are pretty cheap in the grand scheme of things but noting wort a -1","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"}],"nova/db/main/migrations/versions/16f1fbcab42b_resolve_shadow_table_diffs.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"b4941aa86473b6efc821200995f1db3fe1b56a17","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    if bind.engine.name \u003d\u003d \u0027mysql\u0027:"},{"line_number":58,"context_line":"        op.execute("},{"line_number":59,"context_line":"            \u0027ALTER TABLE shadow_instance_type_extra_specs \u0027"},{"line_number":60,"context_line":"            \u0027CONVERT TO CHARACTER SET utf8 \u0027"},{"line_number":61,"context_line":"            \u0027COLLATE utf8_bin\u0027"},{"line_number":62,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":2,"id":"faacf280_c0480252","line":60,"updated":"2021-08-30 14:06:49.000000000","message":"could this be an extensive data manipulation operation in the DB or we just mark the encoding of the underlying binary data?","commit_id":"c31fc03acf76a16c42ebf97c704480d5c2e09d9d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"dd9bee43be90b6ecf5d5d5027df31fe661d011bf","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    if bind.engine.name \u003d\u003d \u0027mysql\u0027:"},{"line_number":58,"context_line":"        op.execute("},{"line_number":59,"context_line":"            \u0027ALTER TABLE shadow_instance_type_extra_specs \u0027"},{"line_number":60,"context_line":"            \u0027CONVERT TO CHARACTER SET utf8 \u0027"},{"line_number":61,"context_line":"            \u0027COLLATE utf8_bin\u0027"},{"line_number":62,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":2,"id":"a5da3d6c_3e43da5b","line":60,"in_reply_to":"faacf280_c0480252","updated":"2021-09-21 16:47:06.000000000","message":"I genuinely have no idea, and Google is no help. All I can say is that we did this before, but of course that was for fewer entries (the shadow table can conceivably have many multiples of the rows in the main table)","commit_id":"c31fc03acf76a16c42ebf97c704480d5c2e09d9d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":true,"context_lines":[{"line_number":38,"context_line":"        batch_op.alter_column("},{"line_number":39,"context_line":"            \u0027user_id\u0027,"},{"line_number":40,"context_line":"            type_\u003dsa.String(64),"},{"line_number":41,"context_line":"            existing_type\u003dsa.String(36),"},{"line_number":42,"context_line":"        )"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # 252_add_instance_extra_table; we shouldn\u0027t have created an index for the"}],"source_content_type":"text/x-python","patch_set":4,"id":"9eaae8d9_0196c53c","line":41,"range":{"start_line":41,"start_character":27,"end_line":41,"end_character":40},"updated":"2021-10-11 12:13:42.000000000","message":"od this is the normal lenght of a uuid im not sure why we would have needed to increase it but matching this does indeed make sesne if something is using the longer id.\n\ni highly doubth we will have 64 charter user_ids however as that would not be a valid uuid anymore aht that would break things so unless we are storing extra infor in the colume im not sure why the orginal change was made.\n\nit looks like keystone allows up to 64 but i suspect if that was used it would break esle where as we certenly dont expect it to be the lenght everywhere\nhttps://github.com/openstack/nova/commit/dbdb938fc08452ea80379652ee101b6ebe006e3f#diff-8d6556dea60e0e49007fda55d06a61d076e44c83188f7f5fdb982e7c38e24d1c","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4454ef218ff4e92b8a91b0edae25ad9b6960c0f7","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        batch_op.alter_column("},{"line_number":39,"context_line":"            \u0027user_id\u0027,"},{"line_number":40,"context_line":"            type_\u003dsa.String(64),"},{"line_number":41,"context_line":"            existing_type\u003dsa.String(36),"},{"line_number":42,"context_line":"        )"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # 252_add_instance_extra_table; we shouldn\u0027t have created an index for the"}],"source_content_type":"text/x-python","patch_set":4,"id":"afa4a86b_5d5b2a51","line":41,"range":{"start_line":41,"start_character":27,"end_line":41,"end_character":40},"in_reply_to":"9eaae8d9_0196c53c","updated":"2021-10-12 08:26:13.000000000","message":"I doubt it also. It sounds like your issue is with the original change, not this one though. All I\u0027m doing here is making sure they\u0027re in sync.","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb5814db3a46e3aab5a137eef8519a12d5841562","unresolved":false,"context_lines":[{"line_number":38,"context_line":"        batch_op.alter_column("},{"line_number":39,"context_line":"            \u0027user_id\u0027,"},{"line_number":40,"context_line":"            type_\u003dsa.String(64),"},{"line_number":41,"context_line":"            existing_type\u003dsa.String(36),"},{"line_number":42,"context_line":"        )"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # 252_add_instance_extra_table; we shouldn\u0027t have created an index for the"}],"source_content_type":"text/x-python","patch_set":4,"id":"01d00985_ed216e5f","line":41,"range":{"start_line":41,"start_character":27,"end_line":41,"end_character":40},"in_reply_to":"afa4a86b_5d5b2a51","updated":"2021-10-12 10:40:24.000000000","message":"ya no issue with your change","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":true,"context_lines":[{"line_number":41,"context_line":"            existing_type\u003dsa.String(36),"},{"line_number":42,"context_line":"        )"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # 252_add_instance_extra_table; we shouldn\u0027t have created an index for the"},{"line_number":45,"context_line":"    # shadow table"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    op.drop_index(\u0027shadow_instance_extra_idx\u0027, \u0027shadow_instance_extra\u0027)"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    # 373_migration_uuid; we shouldn\u0027t have created an index for the shadow"},{"line_number":50,"context_line":"    # table"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    op.drop_index(\u0027shadow_migrations_uuid\u0027, \u0027shadow_migrations\u0027)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    # 298_mysql_extra_specs_binary_collation; we changed the collation on the"},{"line_number":55,"context_line":"    # main table but not the shadow table"}],"source_content_type":"text/x-python","patch_set":4,"id":"b88f1770_ebfb5688","line":52,"range":{"start_line":44,"start_character":3,"end_line":52,"end_character":64},"updated":"2021-10-11 12:13:42.000000000","message":"i guess however the index might speed up purge_delete_rows.\n\ni do know that having some indexs help with that espcially 8if the index is on a filed that we join on.\n\ni think have an index on the instance_uuid is likely a good thing.\n\nthis is removing the shadow table copy of https://github.com/openstack/nova/blob/stable/victoria/nova/db/sqlalchemy/migrate_repo/versions/252_add_instance_extra_table.py#L50-L52\n\nthe same may or may not be true for the migration_uuid.\n\n\nso i think this proably should not be remvoed.","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4454ef218ff4e92b8a91b0edae25ad9b6960c0f7","unresolved":false,"context_lines":[{"line_number":41,"context_line":"            existing_type\u003dsa.String(36),"},{"line_number":42,"context_line":"        )"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    # 252_add_instance_extra_table; we shouldn\u0027t have created an index for the"},{"line_number":45,"context_line":"    # shadow table"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"    op.drop_index(\u0027shadow_instance_extra_idx\u0027, \u0027shadow_instance_extra\u0027)"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    # 373_migration_uuid; we shouldn\u0027t have created an index for the shadow"},{"line_number":50,"context_line":"    # table"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    op.drop_index(\u0027shadow_migrations_uuid\u0027, \u0027shadow_migrations\u0027)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    # 298_mysql_extra_specs_binary_collation; we changed the collation on the"},{"line_number":55,"context_line":"    # main table but not the shadow table"}],"source_content_type":"text/x-python","patch_set":4,"id":"9040bf3b_ba274f16","line":52,"range":{"start_line":44,"start_character":3,"end_line":52,"end_character":64},"in_reply_to":"b88f1770_ebfb5688","updated":"2021-10-12 08:26:13.000000000","message":"If we were talking about the main table then this would be a different argument. However, we never retrieve things from the shadow tables as a normal matter of course. They\u0027re purely for \"diagnostic\" purposes. There are no other indexes present and the commit message that added the index to the main table said nothing about this shadow table being a special case, so this is certainly a mistake. Building an index for a shadow table is dumb.","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":true,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    op.drop_index(\u0027shadow_migrations_uuid\u0027, \u0027shadow_migrations\u0027)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    # 298_mysql_extra_specs_binary_collation; we changed the collation on the"},{"line_number":55,"context_line":"    # main table but not the shadow table"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    if bind.engine.name \u003d\u003d \u0027mysql\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"01e3b5a5_9625f5eb","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":44},"updated":"2021-10-11 12:13:42.000000000","message":"ack.","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4454ef218ff4e92b8a91b0edae25ad9b6960c0f7","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    op.drop_index(\u0027shadow_migrations_uuid\u0027, \u0027shadow_migrations\u0027)"},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"    # 298_mysql_extra_specs_binary_collation; we changed the collation on the"},{"line_number":55,"context_line":"    # main table but not the shadow table"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    if bind.engine.name \u003d\u003d \u0027mysql\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"7349fa0a_40a90643","line":54,"range":{"start_line":54,"start_character":6,"end_line":54,"end_character":44},"in_reply_to":"01e3b5a5_9625f5eb","updated":"2021-10-12 08:26:13.000000000","message":"Ack","commit_id":"558196ac46f857070637cc03c5e88df2b37cbb0d"}],"nova/db/main/migrations/versions/8f2f1571d55b_initial_version.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a15347382ca2fe44fff1878c91fb4e564d77d13d","unresolved":true,"context_lines":[{"line_number":163,"context_line":"            \u0027shadow_\u0027 + table_name, meta, *columns, mysql_engine\u003d\u0027InnoDB\u0027,"},{"line_number":164,"context_line":"        )"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"    # TODO(stephenfin): Fix these various bugs in a follow-up"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    # 252_add_instance_extra_table; we don\u0027t create indexes for shadow tables"},{"line_number":169,"context_line":"    # in general and these should be removed"}],"source_content_type":"text/x-python","patch_set":4,"id":"21063b0f_9cd69ffc","side":"PARENT","line":166,"range":{"start_line":166,"start_character":0,"end_line":166,"end_character":2},"updated":"2021-10-11 12:13:42.000000000","message":"this this im not sure we should have fixed since i know on large dbs we have intitionlly added indexes to solveissue with archiving deleted rows and purging them. but you are resolving a current todo so im not going to -1 over this but it might be a performace regression to remove these so we might ahve to revert this.","commit_id":"c8dadf4af84c9d829047fdacbe06ce4f2afea349"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4454ef218ff4e92b8a91b0edae25ad9b6960c0f7","unresolved":false,"context_lines":[{"line_number":163,"context_line":"            \u0027shadow_\u0027 + table_name, meta, *columns, mysql_engine\u003d\u0027InnoDB\u0027,"},{"line_number":164,"context_line":"        )"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"    # TODO(stephenfin): Fix these various bugs in a follow-up"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    # 252_add_instance_extra_table; we don\u0027t create indexes for shadow tables"},{"line_number":169,"context_line":"    # in general and these should be removed"}],"source_content_type":"text/x-python","patch_set":4,"id":"fd2ef9c8_febfb08e","side":"PARENT","line":166,"range":{"start_line":166,"start_character":0,"end_line":166,"end_character":2},"in_reply_to":"21063b0f_9cd69ffc","updated":"2021-10-12 08:26:13.000000000","message":"The index is on the main table, not this. We don\u0027t scan through the shadow tables when deleting stuff. We simply truncate it.","commit_id":"c8dadf4af84c9d829047fdacbe06ce4f2afea349"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"eb5814db3a46e3aab5a137eef8519a12d5841562","unresolved":false,"context_lines":[{"line_number":163,"context_line":"            \u0027shadow_\u0027 + table_name, meta, *columns, mysql_engine\u003d\u0027InnoDB\u0027,"},{"line_number":164,"context_line":"        )"},{"line_number":165,"context_line":""},{"line_number":166,"context_line":"    # TODO(stephenfin): Fix these various bugs in a follow-up"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":"    # 252_add_instance_extra_table; we don\u0027t create indexes for shadow tables"},{"line_number":169,"context_line":"    # in general and these should be removed"}],"source_content_type":"text/x-python","patch_set":4,"id":"7e8539cc_5b5bdef8","side":"PARENT","line":166,"range":{"start_line":166,"start_character":0,"end_line":166,"end_character":2},"in_reply_to":"fd2ef9c8_febfb08e","updated":"2021-10-12 10:40:24.000000000","message":"not quite but we dont use those columns \n\nhttps://github.com/openstack/nova/blob/fdfdba265833d237e22676f9a223ab8ca0fe1e03/nova/db/main/api.py#L4603-L4618\n\nwe would actully benifit form indexes on the delete_at created_at and updated_at columns but its not relevent to your patch.\n\ni know that operators have reported large perfromacne increase in the archive deleted row command when they added indexes to the deleted_at columns in the main tables i woudl expect the same to be true of the purge command if they use --before.\n\nagain not relevent to these indexes","commit_id":"c8dadf4af84c9d829047fdacbe06ce4f2afea349"}]}