citusdata / citusdata/citus

Refactor Connection Management

Open
#2,183 1 comment 1 reaction 0 assignees View on GitHub
technical debt
Dominant language
C
Stars
12.8k
Forks
794
Avg merge
2d 14h
Merged PRs (30d)
31

Description

This is to track improvements we can make to connection management related code, and the issue is to be edited as I get better understanding of what we can improve.

**High-level structural issues**
- [ ] Consolidate the API we use for connection management in different executors. We probably don't need the `connectionId` stuff in `multi_client_executor.c` anymore.
- [ ] The reason behind having the hash tables in `connection_management.c` and `placement_connection.c` are a bit difficult to understand. They also make the code very bloated. It wasn't until I tried to remove `ConnectionHash` that I understood why we exactly need it. I don't have a specific suggestion on how we should make the situation better. I'll experiment with some.
- [ ] `placement_connection.c` is difficult to follow.

**Other structural issues**
- [ ] `FinishConnectionEstablishment()` is similar to `MultiClientConnectPoll()` + `ClientConnectionReady()`. Consolidate them and remove duplicate code.
- [ ] `ShutdownConnection()` seems to be too low level to be called outside `connection_management.c`, check if it is really necessary to be exported.

**Implementation detail issues**
- [ ] `MultiClientSendQuery()` probably should set `connection->remoteTransaction->transactionFailed`, but it doesn't. If this is the case, we should probably call `HandleRemoteTransactionConnectionError()` there. If that is the case, some duplicate code between `multi_router_executor.c` and `multi_client_executor.c`. Extract the common logic into a single function.
- [ ] `GetPlacementConnection()`: In `multi_copy.c` and `multi_explain.c`, we check the status of result of `GetPlacementConnection()` before using it. In `master_stage_protocol.c` and `master_delete_protocol.c` we don't. Should we check it or not? Understand this.
- [ ] Similarly, for `GetNodeConnection()`. Sometimes we check the status of its result, sometimes we don't.
...

(more items for later)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.