Drop support for citus.multi_shard_commit_protocol #5380
Conversation
onderkalaci
force-pushed
the
drop_1pc_single_statement
branch
from
611d7cb
to
4a72228
Compare
onderkalaci
force-pushed
the
drop_1pc_multi_shard
branch
from
6b604e0
to
e483ea7
Compare
| COPY reference_table (a, b, z) FROM STDIN WITH CSV; | ||
| ERROR: cannot assign TransactionIds during recovery | ||
| COPY citus_local_table (a, b, z) FROM STDIN WITH CSV; | ||
| ERROR: cannot assign TransactionIds during recovery |
There was a problem hiding this comment.
it would be nice if we can give a better error message somehow, or is this the same error that PG gives?
There was a problem hiding this comment.
This is the error PG gives.
There are some challenges to give proper errors and some input from you would be valuable:
-
For replication factor == 1 tables, we use 2PC when it is a multi-shard command. But, we don't know (shouldn't know) whether the command is multi-shard until the executor. However, until we get the adaptive executor, PG already throws this error. So, the only place to give proper error is the planner. And, I hesitate to enforce this in the planner given that we might change it in the future?
-
For COPY we always use 2PC, so can give a better error. But we do that even for single shard COPY commands. Do you think would that break some users?
2.2) For copy on Citus local tables, we trigger 2PC as for all other COPY commands. Maybe we can add a special case for Citus local tables?
onderkalaci
force-pushed
the
drop_1pc_single_statement
branch
4 times, most recently
from
fa48ada
to
64980c1
Compare
onderkalaci
force-pushed
the
drop_1pc_multi_shard
branch
from
e483ea7
to
79138a1
Compare
| 5 | 6 | 7 | ||
| (2 rows) | ||
|
|
||
| ERROR: cannot assign TransactionIds during recovery |
There was a problem hiding this comment.
do we want to fix this error?
There was a problem hiding this comment.
Please see #5380 (comment)
| @@ -1499,6 +1475,22 @@ ReadOnlyTask(TaskType taskType) | |||
| } | |||
|
|
|||
|
|
|||
| bool | |||
| TaskListCannotBeExecutedInTransaction(List *taskList) | |||
In the past, we allowed users to manually switch to 1PC (e.g., one phase commit). However, with this commit, we don't. All multi-shard modifications are done via 2PC.
onderkalaci
force-pushed
the
drop_1pc_multi_shard
branch
from
e8807e7
to
5de0090
Compare
In the past, we allowed users to manually switch to 1PC
(e.g., one phase commit). However, with this commit, we
don't. All multi-shard modifications are done via 2PC.
DESCRIPTION: Drop support for citus.multi_shard_commit_protocol, always use 2PC
TODO: