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

Schema merging may fail to detect recreated schemas with the same name #3797

Closed
duarten opened this Issue Oct 1, 2018 · 3 comments

Comments

Projects
None yet
1 participant
@duarten
Member

duarten commented Oct 1, 2018

Installation details
Scylla version (or git commit hash): >= 1.0
Cluster size:
OS (RHEL/CentOS/Ubuntu/AWS AMI):

Currently we diff schemas based on table/view name, and if the names match, then we detect altered schemas by comparing the schema mutations. This fails to detect transitions which involve dropping and recreating a schema with the same name, if a node receives these notifications simultaneously (for example, if the node was temporarily down or partitioned).

Consider the following test:

    def test_schema_recreated_while_node_down(self):
        self.cluster.set_configuration_options(values={ 'ring_delay_ms': 1000, 'experimental': True})
        self.cluster.populate(2)
        self.cluster.start(wait_other_notice=True)

        [node1, node2] = self.cluster.nodelist()

        session = self.patient_cql_connection(node1)

        debug('Creating schema')
        self.create_ks(session, 'ks', 2)
        session.execute("CREATE TABLE cf (p int PRIMARY KEY, v text);")

        debug('Populating')
        session.execute(SimpleStatement("INSERT INTO cf (p, v) VALUES (1, '1')", consistency_level = ConsistencyLevel.ALL))

        debug("Stopping node2")
        node2.stop(gently=True)

        debug("Re-creating schema")
        session.execute("DROP TABLE cf;")
        session.execute("CREATE TABLE cf (p int PRIMARY KEY, v1 bigint, v2 text);")

        debug("Restarting node2")
        node2.start(wait_for_binary_proto=True)

        rows = session.execute(SimpleStatement("SELECT * FROM cf", consistency_level = ConsistencyLevel.ALL))
        assert rows_to_list(rows) == [], "Expected an empty result set, got %s" % (rows)

It fails with:

AssertionError: Unexpected error in node1 node log: ["ERROR 2018-10-01 23:16:42,550 [shard 1] storage_proxy - Exception when communicating with 127.0.0.2: std::runtime_error (Can't find a column family with UUID 4181ef60-c5bf-11e8-94a8-000000000001)"]

And the logs of the node that was stopped contain:

INFO  2018-10-01 23:16:29,299 [shard 0] schema_tables - Altering ks.cf id=4181ef60-c5bf-11e8-94a8-000000000001 version=408450c5-e31e-34d5-9f31-156449c38a77

The solution is to diff schemas using their ID, and not their name.

Note that because the ID is persisted and created when executing a create_table_statement, then even if a schema is re-created with the exact same structure as before, we will still considered it altered because the mutations will differ.

This also stops schema pulling from working, since it relies on schema merging.

@duarten

This comment has been minimized.

Member

duarten commented Oct 1, 2018

I think that keyspaces are susceptible to this, but in that case it's fine: dropping and recreating a keyspace can indeed be considered a keyspace alter together with dropping all tables of the dropped keyspace.

@duarten

This comment has been minimized.

Member

duarten commented Oct 1, 2018

User types are also fine, since they're just metadata and have no ID (like keyspaces, they're values with no identity).

@duarten duarten self-assigned this Oct 1, 2018

@duarten

This comment has been minimized.

Member

duarten commented Oct 1, 2018

Finally, note that the proposed solution doesn't apply to tables dropped and created with the ID property. For that, we would need to detect deltas instead of applying changes and then reading the new state to find differences. However, I think this solution is enough, because tables are usually created with ID = {} for very specific, peculiar reasons. The original motivation meant for the new table to be treated exactly as the old, so the current behavior is in fact the desired one.

Also note that eventually we'll revamp our schema management (#1207), which can solve this problem in an obviously-correct way.

tgrabiec added a commit that referenced this issue Oct 2, 2018

db/schema_tables: Drop tables before creating new ones
Doing it by the inverse order doesn't support dropping and creating a
schema with the same name.

Refs #3797

Signed-off-by: Duarte Nunes <duarte@scylladb.com>
Message-Id: <20181001230932.47153-1-duarte@scylladb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment