Only execute access rights change queries on a single node #257
Conversation
For the read-only step, we're filtering by replicated users, whose grant information is stored in ZooKeeper. This means that executing the query on multiple nodes is counterproductive. Instead of achieving faster information propagation, we're getting transaction errors from ZooKeeper. Any one of these can in turn fail the step.
|
|
||
| async def grant_write_on_table(self, table: Table, user_name: bytes) -> None: | ||
| escaped_user_name = escape_sql_identifier(user_name) | ||
| grant_statement = ( | ||
| f"GRANT INSERT, ALTER UPDATE, ALTER DELETE ON {table.escaped_sql_identifier} TO {escaped_user_name}" | ||
| ) | ||
| await asyncio.gather(*(client.execute(grant_statement.encode()) for client in self.clients)) | ||
| await self.clients[0].execute(grant_statement.encode()) |
There was a problem hiding this comment.
This change looks very controversial to me
- Yes, in principle it is not necessary to change privileges multiple times, but we do not control how fast it will reach other servers, do we? Theoretically it might be delayed for unknown time which spoils all guarantees.
- I really want to continue this discussion. So far I think it is better to just delete this step completely...
There was a problem hiding this comment.
I mean the cluster is in a single region, and zookeeper is strictly serializable, so the upper bound on the latency for the change to go through in ZK is <5ms.
Actually I don't think the deduplication log is relevant for the restore. The log is saved in ZooKeeper, and the restore process doesn't start from a ZK snapshot but from an empty state. So the block-level information is lost.
There was a problem hiding this comment.
Will test if in practice the argument I just provided stands, but the main branch is broken at the moment and I'd rather we fix it. If the step is irrelevant, then of course we'll drop it.
There was a problem hiding this comment.
Zookeeper only supports write operation serialization, but reads could propagate slowly.
They first go to the local zookeeper, and then to the clickhouse to be processed, as I understand.
But looks like it does not efficiently make things worse, you are right.
- the solution is not easy, so, lets start with this one
For the read-only step, we're filtering by replicated users, whose grant information is stored in ZooKeeper.
This means that executing the query on multiple nodes is counterproductive. Instead of achieving faster information propagation, we're getting transaction errors from ZooKeeper. Any one of these can in turn fail the step.
Here's one such example: