separate write db and read db by sredxny · Pull Request #121 · TykTechnologies/storage

sredxny · 2025-09-29T14:56:55Z

Description

For postgres handle 2 connections: write connection that will write in main db, and read connection that will read replicas, if readConnection string is not set then we will use the same connection to the main server.

There are some visor warning that will not be addressed here:

security Issue
Check notice: security Issue
Error messages returned upon connection failure wrap the original database driver errors. This could potentially leak sensitive information about the database infrastructure (e.g., hostnames, usernames, IP addresses) into logs or to the end-user.

Am not returning the error from gorm, but a generated error

style Issue
Check failure: style Issue
The nil-check for the database connection `if db == nil` will cause a compile error as `db` is not defined in this scope. This was likely introduced during refactoring and should check `writeDB` and `readDB` instead.

Outdated, we do not have that and we check readDb and write DB

security Issue
Check warning: security Issue
The `DropTable` function constructs a raw SQL query for `SELECT COUNT(*)` using `fmt.Sprintf` with an unsanitized table name. While table names are typically not user-controlled, this practice is unsafe and could lead to SQL injection if an attacker finds a way to influence the `name` parameter.

Same as indexes creation, we cannot receive normalized params as it represents a braking change in the interface. Anyway users do not have access to the query executed here.

documentation Issue
Check warning: documentation Issue
The test comment is outdated and contradicts the code's behavior. The comment for Case 3 states that the ID should remain empty, but the `ensureID` function was updated to always create a new ID if one is not provided. The test correctly asserts the new behavior, but the comment is misleading.

outdated as well

Related Issue

Motivation and Context

Test Coverage For This Change

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)
Documentation updates or improvements.

Checklist

github-actions · 2025-09-29T14:57:07Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

sredny buitrago seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

probelabs · 2025-09-29T15:03:44Z

🔍 Code Analysis Results

This is an excellent pull request that introduces a crucial feature for database scalability. Here is a comprehensive analysis:

1. Change Impact Analysis

What this PR accomplishes

This pull request refactors the PostgreSQL database driver to support read/write splitting. It introduces the ability to configure separate database connections for write operations (like INSERT, UPDATE, DELETE) and read operations (SELECT). This allows the application to direct write traffic to a primary database and offload read traffic to one or more replica databases.

This is a significant architectural improvement that enables:

Scalability: Read-heavy workloads can be scaled by adding more read replicas without impacting the performance of the primary write database.
Performance: Reduces the load on the primary database, leading to faster query execution for both reads and writes.
High Availability: Can be part of a high-availability strategy, allowing the application to continue serving read requests even if the primary database is temporarily unavailable.

The implementation is backward-compatible. If a separate read connection string is not provided, the driver defaults to using the primary (write) connection for all operations, ensuring existing deployments continue to work without any configuration changes.

Key technical changes introduced

New Configuration Option: A ReadConnectionString field has been added to the ClientOpts struct (persistent/internal/types/client_options.go), allowing users to specify a connection string for a read replica.
Dual Connection Management: The connection management logic in persistent/internal/driver/postgres/lifecycle.go has been fundamentally updated. The lifeCycle struct now holds two separate gorm.DB connections: writeDB and readDB. The Connect method establishes both connections if two different connection strings are provided, and the Close and Ping methods are updated to manage the lifecycle of both.
Intelligent Query Routing: All database operations within the PostgreSQL driver have been systematically routed to the appropriate connection:
- Write Operations: All functions that modify data or schema (e.g., Insert, Update, Delete, Migrate, CreateIndex) now exclusively use the writeDB connection. These changes are primarily in basic_operations.go, indexes.go, and schema.go.
- Read Operations: All functions that only retrieve data (e.g., Query, Count, GetIndexes, DBTableStats, HasTable) now use the readDB connection. These changes are found in query.go, indexes.go, and schema.go.
Code Refactoring: Common helper functions (ensureID, cloneDBObject, mergeQueryFields) and their tests were moved from the basic_operations files into new, dedicated utils.go and utils_test.go files. This improves code organization and reusability.

Affected system components

The changes are well-encapsulated within the PostgreSQL driver located at persistent/internal/driver/postgres/. While the modifications are localized to this driver, any part of the application that uses the persistent storage layer with a PostgreSQL backend can now leverage this new capability to build more scalable and resilient database architectures.

2. Architecture Visualization

The following diagram illustrates the new database connection architecture. It shows how the PostgresDriver now acts as a router, directing queries to either the primary database for writes or a read replica for reads.

graph TD
    title PostgreSQL Read/Write Connection Separation

    subgraph Application
        PostgresDriver
    end

    subgraph "Database Infrastructure"
        PrimaryDB["Primary DB (Writes)"]
        ReplicaDB["Read Replica (Reads)"]
    end

    PostgresDriver -- "writeDB Connection (INSERT, UPDATE, DELETE, etc.)" --> PrimaryDB
    PostgresDriver -- "readDB Connection (SELECT, COUNT, etc.)" --> ReplicaDB

    PrimaryDB -- "Data Replication" --> ReplicaDB

    classDef note fill:#f9f,stroke:#333,stroke-width:2px;
    
    subgraph Note
        direction LR
        A["If ReadConnectionString is not provided,<br/>the readDB connection points to the Primary DB,<br/>using the same connection as writeDB."]
    end
    class Note note

Powered by Visor from Probelabs

Last updated: 2025-09-30T13:45:53.090Z | Triggered by: synchronize | Commit: bbab3b8

probelabs · 2025-09-29T15:03:45Z

🔍 Code Analysis Results

Security Issues (4)

Severity	Location	Issue
🔴 Critical	`persistent/internal/driver/postgres/lifecycle.go:195-217`	The `Close()` function will cause a panic due to attempting to close the same database connection twice when no separate read replica is configured. When `ReadConnectionString` is empty, `l.readSQLDB` and `l.writeSQLDB` point to the same instance. The function's logic leads to a second close call on the same underlying connection, causing a `panic: close of closed channel`. This can be exploited to cause a Denial of Service by crashing the application. 💡 Suggestion To prevent a panic from a double-close operation, determine if a single connection is being used before any close operations are performed. Store this state in a boolean and use it to conditionally close the read connection only if it's a separate instance. 🔧 Suggested Fix func (l *lifeCycle) Close() error { var writeErr, readErr isSingleConnection := l.readSQLDB == l.writeSQLDB // Close write connection if l.writeSQLDB != nil { writeErr = l.writeSQLDB.Close() } // Close read connection only if it's a different instance if !isSingleConnection && l.readSQLDB != nil { readErr = l.readSQLDB.Close() } // Nullify all pointers to prevent reuse l.writeDB = nil l.writeSQLDB = nil l.readDB = nil l.readSQLDB = nil if writeErr != nil { return writeErr } return readErr `}`
🟢 Info	`persistent/internal/driver/postgres/lifecycle.go:104`	Error messages returned upon connection failure wrap the original database driver errors. This could potentially leak sensitive information about the database infrastructure (e.g., hostnames, usernames, IP addresses) into logs or to the end-user. 💡 Suggestion For production environments, consider logging the detailed error internally and returning a more generic, sanitized error message to the caller to avoid exposing internal system details. This prevents attackers from gaining intelligence about your infrastructure from error messages.
🟡 Warning	`persistent/internal/driver/postgres/schema.go:321`	The `DropTable` function constructs a raw SQL query for `SELECT COUNT()` using `fmt.Sprintf` with an unsanitized table name. While table names are typically not user-controlled, this practice is unsafe and could lead to SQL injection if an attacker finds a way to influence the `name` parameter. 💡 Suggestion* Sanitize the table name identifier before embedding it in the raw SQL string. The `sanitizeIdentifier` function introduced in `indexes.go` would be suitable for this purpose. The sanitized name should then be used in the `fmt.Sprintf` call.
🟡 Warning	`persistent/internal/driver/postgres/lifecycle_test.go:1`	The test suite lacks coverage for the new read/write connection functionality. Critical paths, such as connecting with a `ReadConnectionString` and correctly closing both single and dual connections, are untested. This omission allowed a critical Denial of Service vulnerability in the `Close()` function to go undetected. 💡 Suggestion Add new test cases to `TestLifeCycleConnect` to validate behavior when `ReadConnectionString` is provided. Create a dedicated `TestLifeCycleClose` function to ensure that `Close()` works correctly in both single-connection mode (does not panic) and dual-connection mode (closes both connections successfully).

Performance Issues (3)

Severity	Location	Issue
🔴 Critical	`persistent/internal/driver/postgres/lifecycle.go:195-217`	The `Close()` function will cause a panic due to attempting to close the same database connection twice when no separate read replica is configured. In single-connection mode, `l.readSQLDB` and `l.writeSQLDB` point to the same instance. The logic incorrectly attempts to close the connection a second time, leading to a `panic: close of closed channel`. This is a critical resource management failure that can lead to a denial of service. 💡 Suggestion To prevent a panic from a double-close operation, determine if a single connection is being used before any close operations are performed. Store this state in a boolean and use it to conditionally close the read connection only if it's a separate instance. 🔧 Suggested Fix func (l *lifeCycle) Close() error { var writeErr, readErr isSingleConnection := l.readSQLDB == l.writeSQLDB // Close write connection if l.writeSQLDB != nil { writeErr = l.writeSQLDB.Close() } // Close read connection only if it's a different instance if !isSingleConnection && l.readSQLDB != nil { readErr = l.readSQLDB.Close() } // Nullify all pointers to prevent reuse l.writeDB = nil l.writeSQLDB = nil l.readDB = nil l.readSQLDB = nil if writeErr != nil { return writeErr } return readErr `}`
🟢 Info	`persistent/internal/driver/postgres/indexes.go:13`	The compilation of the regular expression for `sanitizeIdentifier` has been moved to a global variable. This is a positive micro-optimization that avoids the performance overhead of recompiling the regex on every function call. 💡 Suggestion This change is a good performance practice and should be maintained.
🟡 Warning	`persistent/internal/driver/postgres/schema.go:321-326`	The `DropTable` function executes a `SELECT COUNT()` query on the read replica before dropping the table. On very large tables, this can be a slow and resource-intensive operation, potentially requiring a full table scan and impacting the performance of other read queries. 💡 Suggestion* Re-evaluate the necessity of returning an exact row count when dropping a table. If this is for informational purposes, consider if the feature is worth the potential performance cost. An alternative is to retrieve an estimated row count from PostgreSQL's statistics (`pg_class.reltuples`), which is much faster, though less accurate.

Quality Issues (5)

Severity	Location	Issue
🔴 Critical	`persistent/internal/driver/postgres/lifecycle.go:195-217`	The `Close()` function will cause a panic when a separate read replica is not configured. In single-connection mode, `l.readSQLDB` and `l.writeSQLDB` point to the same instance. The current logic attempts to close the connection twice by first closing `l.writeSQLDB` and then incorrectly re-evaluating the condition for closing `l.readSQLDB` after `l.writeSQLDB` has been set to nil. This leads to a `panic: close of closed channel`, which is a critical resource management failure that will crash the application. 💡 Suggestion To prevent a panic from a double-close operation, determine if a single connection is being used before any close operations or pointer assignments are performed. Store this state in a boolean and use it to conditionally close the read connection only if it's a separate instance. 🔧 Suggested Fix func (l *lifeCycle) Close() error { var writeErr, readErr isSingleConnection := l.readSQLDB == l.writeSQLDB // Close write connection if l.writeSQLDB != nil { writeErr = l.writeSQLDB.Close() } // Close read connection only if it's a different instance if !isSingleConnection && l.readSQLDB != nil { readErr = l.readSQLDB.Close() } // Nullify all pointers to prevent reuse l.writeDB = nil l.writeSQLDB = nil l.readDB = nil l.readSQLDB = nil if writeErr != nil { return writeErr } return readErr `}`
🟠 Error	`persistent/internal/driver/postgres/lifecycle_test.go:1`	The test suite lacks coverage for the new read/write connection functionality. Critical paths, such as connecting with a `ReadConnectionString` and correctly closing both single and dual connections, are untested. This omission allowed a critical Denial of Service vulnerability in the `Close()` function to go undetected. 💡 Suggestion Add new test cases to `TestLifeCycleConnect` to validate behavior when `ReadConnectionString` is provided. Create a dedicated `TestLifeCycleClose` function to ensure that `Close()` works correctly in both single-connection mode (does not panic) and dual-connection mode (closes both connections successfully).
🟢 Info	`persistent/internal/types/client_options.go:22-40`	The comment for the new `ReadConnectionString` field is significantly more verbose than comments for other fields in the same struct, creating an inconsistent documentation style. This makes the struct definition harder to scan and read. 💡 Suggestion Condense the comment to be more concise and consistent with the style of other comments in the struct. The detailed explanation of benefits and usage examples is better suited for package-level documentation or a README. 🔧 Suggested Fix `// ReadConnectionString specifies the connection string for read-only operations, // enabling the use of read replicas. If empty, ConnectionString is used for // all operations. ReadConnectionString string`
🟢 Info	`persistent/internal/driver/postgres/schema.go:68-499`	The pattern for using database connection handles is inconsistent across functions in this file. Some functions (`HasTable`, `DBTableStats`, `GetTables`) assign `d.readDB` to a local `db` variable at the start, while others (`DropTable`, `Drop`, `Migrate`) access the struct fields `d.writeDB` and `d.readDB` directly or assign them to different local variables. 💡 Suggestion For improved readability and consistency, adopt a single pattern. Assigning the appropriate connection to a local variable (e.g., `db`) at the start of each function is a good practice, especially if the connection is used multiple times.
🟡 Warning	`persistent/internal/driver/postgres/lifecycle.go:147`	In the `Connect` function, if establishing the read connection fails, a new error variable `err` is declared with `err := l.writeSQLDB.Close()`, shadowing the original error from `establishConnection`. This means the original, more specific error (e.g., "invalid credentials for read replica") is lost, and a generic "failed to establish read connection" is returned. This makes debugging connection issues much harder. 💡 Suggestion Avoid shadowing the `err` variable. Log the cleanup error if it occurs, but return the original, more informative error from `establishConnection` to aid in debugging.

Style Issues (7)

Severity	Location	Issue
🟠 Error	`persistent/internal/driver/postgres/schema.go:304-306`	The nil-check for the database connection `if db == nil` will cause a compile error as `db` is not defined in this scope. This was likely introduced during refactoring and should check `writeDB` and `readDB` instead. 💡 Suggestion Update the condition to check both the `writeDB` and `readDB` connections, which are used within the `DropTable` function, to prevent a compile error and ensure the function behaves correctly. 🔧 Suggested Fix `if writeDB == nil \|\| readDB == nil {`
🟢 Info	`persistent/internal/types/client_options.go:22-40`	The comment for the new `ReadConnectionString` field is excessively verbose compared to the concise, single-line comments for other fields in the same struct. This creates a stylistic inconsistency and makes the struct definition harder to scan. 💡 Suggestion Condense the comment to align with the existing style. Detailed explanations of benefits and usage examples are better suited for package-level documentation or a README. 🔧 Suggested Fix `// ReadConnectionString specifies the connection string for read-only operations, // enabling the use of read replicas. If empty, ConnectionString is used for // all operations. ReadConnectionString string`
🟢 Info	`persistent/internal/driver/postgres/lifecycle.go:195-217`	The logic in the `Close()` function to handle single vs. dual connections is functionally correct but difficult to read at a glance due to a complex conditional. This reduces maintainability. 💡 Suggestion Simplify the logic by first checking if the read and write connections point to the same underlying instance. Use this boolean to conditionally close the read connection, making the code's intent clearer. 🔧 Suggested Fix `isSingleConnection := l.readSQLDB == l.writeSQLDB // Close write connection if l.writeSQLDB != nil { err = l.writeSQLDB.Close() } // Only close read connection if it's a different instance if !isSingleConnection && l.readSQLDB != nil { readErr := l.readSQLDB.Close() if err == nil { err = readErr } }</code></pre></details>`
🟢 Info	`persistent/internal/driver/postgres/lifecycle.go:199`	The comment `// Close write connection` is duplicated on a consecutive line. 💡 Suggestion Remove the redundant comment on line 199 to clean up the code.
🟢 Info	`persistent/internal/driver/postgres/indexes.go:369-374`	The helper function `sanitizeIdentifier` is defined at the end of the file, far from its usage point in `CreateIndex`. While functionally correct, placing helper functions closer to where they are called improves code organization and readability. 💡 Suggestion Consider moving the `sanitizeIdentifier` function definition to be either just before or just after the `CreateIndex` function to improve code flow and discoverability for future maintainers.
🟡 Warning	`persistent/internal/driver/postgres/schema.go:68-499`	The pattern for using database connection handles is inconsistent across the file. Some functions assign `d.readDB` or `d.writeDB` to a local `db` variable at the start, while others access the struct field directly. This reduces code uniformity and readability. 💡 Suggestion For improved readability and consistency, adopt a single pattern. Assigning the appropriate connection to a local `db` variable at the start of each function is recommended, especially if the connection is used multiple times.
🟡 Warning	`persistent/internal/driver/postgres/utils_test.go:71`	The test comment is outdated and contradicts the code's behavior. The comment for Case 3 states that the ID should remain empty, but the `ensureID` function was updated to always create a new ID if one is not provided. The test correctly asserts the new behavior, but the comment is misleading. 💡 Suggestion Update the comment to accurately reflect that a new ID is generated when one is not provided in the original object or the query. 🔧 Suggested Fix `// Case 3: neither originalID nor query["id"] → a new ID is generated`

Powered by Visor from Probelabs

Last updated: 2025-09-30T13:45:54.819Z | Triggered by: synchronize | Commit: bbab3b8

sonarqubecloud · 2025-09-30T00:36:52Z

Quality Gate failed

Failed conditions
1 Security Hotspot
79.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

separate write db and read db

41219d3

sredny buitrago added 9 commits September 29, 2025 11:33

update tests to use write/read connection

4f1ace0

update tags for tests

5a7dd10

refactor write/read conns

7f07362

address visor comments

f0fd3c7

removed duplicated func

b8c494c

update test

2829fce

update test for ensure id

7cd58f6

safe errors returned

d1dc95b

added test for write and read dsn

3b6944a

sredny buitrago added 3 commits September 30, 2025 09:33

addressing code style

4d66c46

remove unsued import

52ee223

fix schema test

bbab3b8

Base automatically changed from implement-postgres to main November 12, 2025 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate write db and read db#121

separate write db and read db#121
sredxny wants to merge 13 commits into
mainfrom
allow-write-and-read-connection

sredxny commented Sep 29, 2025 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 29, 2025

Uh oh!

probelabs Bot commented Sep 29, 2025 •

edited

Loading

Uh oh!

probelabs Bot commented Sep 29, 2025 •

edited

Loading

Uh oh!

sonarqubecloud Bot commented Sep 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

sredxny commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

Test Coverage For This Change

Screenshots (if appropriate)

Types of changes

Checklist

Uh oh!

github-actions Bot commented Sep 29, 2025

Uh oh!

probelabs Bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Code Analysis Results

1. Change Impact Analysis

What this PR accomplishes

Key technical changes introduced

Affected system components

2. Architecture Visualization

Uh oh!

probelabs Bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Code Analysis Results

Security Issues (4)

Performance Issues (3)

Quality Issues (5)

Style Issues (7)

Uh oh!

sonarqubecloud Bot commented Sep 30, 2025

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sredxny commented Sep 29, 2025 •

edited

Loading

probelabs Bot commented Sep 29, 2025 •

edited

Loading

probelabs Bot commented Sep 29, 2025 •

edited

Loading