feat: add optional read-write separation for database#1516
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request implements read-write separation by introducing a routerDriver that directs read-only queries to a configured replica while routing writes and transactions to the master. The changes include updated configuration schemas, documentation, and database initialization logic. Review feedback identifies a potential connection leak in the driver's Close method, suggests more accurate SQL token matching, recommends a more flexible dialect fallback, and points out a performance optimization in the SQL parser.
| func (d *routerDriver) Close() error { | ||
| // Only close the drivers we created; let the underlying *sql.DB leak | ||
| // only if both drivers reference the same *sql.DB (shouldn't happen in practice). | ||
| if d.replica != nil { | ||
| d.replica.Close() | ||
| } | ||
| return nil // master driver is closed via the sql.DB pool lifecycle | ||
| } |
There was a problem hiding this comment.
The Close method currently only closes the replica driver. Since the master driver is also created and managed within the NewEntClient lifecycle (wrapping a *sql.DB), it must be explicitly closed here to prevent connection pool leaks when the Ent client is closed. The comment suggesting it is closed elsewhere is incorrect in this context.
| func (d *routerDriver) Close() error { | |
| // Only close the drivers we created; let the underlying *sql.DB leak | |
| // only if both drivers reference the same *sql.DB (shouldn't happen in practice). | |
| if d.replica != nil { | |
| d.replica.Close() | |
| } | |
| return nil // master driver is closed via the sql.DB pool lifecycle | |
| } | |
| func (d *routerDriver) Close() error { | |
| var err error | |
| if d.replica != nil { | |
| err = d.replica.Close() | |
| } | |
| if masterErr := d.master.Close(); masterErr != nil { | |
| err = masterErr | |
| } | |
| return err | |
| } |
| func entDialect(dialectName string) string { | ||
| switch dialectName { | ||
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | ||
| return dialect.Postgres | ||
| case "sqlite3", "sqlite": | ||
| return dialect.SQLite | ||
| case "mysql", "tidb": | ||
| return dialect.MySQL | ||
| default: | ||
| return dialect.Postgres | ||
| } | ||
| } |
There was a problem hiding this comment.
The entDialect function defaults to dialect.Postgres for any unknown dialect. This could lead to incorrect SQL generation if a custom or unsupported dialect is used. It should return the input dialectName as a fallback, allowing Ent to attempt to handle it or provide a more descriptive error.
| func entDialect(dialectName string) string { | |
| switch dialectName { | |
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | |
| return dialect.Postgres | |
| case "sqlite3", "sqlite": | |
| return dialect.SQLite | |
| case "mysql", "tidb": | |
| return dialect.MySQL | |
| default: | |
| return dialect.Postgres | |
| } | |
| } | |
| func entDialect(dialectName string) string { | |
| switch dialectName { | |
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | |
| return dialect.Postgres | |
| case "sqlite3", "sqlite": | |
| return dialect.SQLite | |
| case "mysql", "tidb": | |
| return dialect.MySQL | |
| default: | |
| return dialectName | |
| } | |
| } |
| for _, prefix := range readOnlyPrefixes { | ||
| if strings.HasPrefix(upper, prefix) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Using strings.HasPrefix to identify read-only keywords can lead to false positives if a non-keyword token starts with a reserved word (e.g., a table or function named SELECTING). Since nextSQLToken already extracts the full token, an exact equality check is safer and more accurate.
| for _, prefix := range readOnlyPrefixes { | |
| if strings.HasPrefix(upper, prefix) { | |
| return true | |
| } | |
| } | |
| for _, prefix := range readOnlyPrefixes { | |
| if upper == prefix { | |
| return true | |
| } | |
| } |
| // block comments (/* */), line comments (--), and whitespace. | ||
| func nextSQLToken(sqlStr string) string { | ||
| var result strings.Builder | ||
| runes := []rune(sqlStr) |
There was a problem hiding this comment.
Converting the entire SQL string to a []rune slice causes an O(N) memory allocation and an extra pass over the string. Since only the first token is needed, it is more efficient to iterate over the string directly using range or utf8.DecodeRuneInString.
|
Thanks for your pr, https://entgo.io/docs/faq/#how-to-configure-two-or-more-db-to-separate-read-and-write |
|
understood. the above is based on my own engineering experience, I forgot to refer to best practices. i will modify the code later |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements database read-write separation by introducing support for a read replica. The changes include new configuration options in conf/conf.go and config.example.yml, updated Helm charts, and comprehensive documentation for routing logic. A new routerDriver is introduced to handle the routing of SELECT queries to the replica while ensuring writes and transactions remain on the master. Review feedback focuses on improving the robustness of the database initialization logic, specifically addressing a regression where unsupported dialects silently default to Postgres, and ensuring that both master and replica connections are closed during shutdown to prevent resource leaks.
| func openDB(dialectName, dsn string, maxOpen, maxIdle int, maxLifetime, maxIdleTime time.Duration) (string, *sql.DB, error) { | ||
| sqlDB, err := sql.Open(driverName(dialectName), dsn) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| if maxOpen > 0 { | ||
| sqlDB.SetMaxOpenConns(maxOpen) | ||
| } | ||
| if maxIdle > 0 { | ||
| sqlDB.SetMaxIdleConns(maxIdle) | ||
| } | ||
| if maxLifetime > 0 { | ||
| sqlDB.SetConnMaxLifetime(maxLifetime) | ||
| } | ||
| if maxIdleTime > 0 { | ||
| sqlDB.SetConnMaxIdleTime(maxIdleTime) | ||
| } | ||
|
|
||
| return entDialect(dialectName), sqlDB, nil | ||
| } |
There was a problem hiding this comment.
The openDB function should handle potential errors from entDialect and ensure that the sql.DB handle is closed if a dialect error occurs after the connection is opened. Currently, entDialect has a silent fallback to Postgres which is problematic.
func openDB(dialectName, dsn string, maxOpen, maxIdle int, maxLifetime, maxIdleTime time.Duration) (string, *sql.DB, error) {
dialect, err := entDialect(dialectName)
if err != nil {
return "", nil, err
}
sqlDB, err := sql.Open(driverName(dialectName), dsn)
if err != nil {
return "", nil, err
}
if maxOpen > 0 {
sqlDB.SetMaxOpenConns(maxOpen)
}
if maxIdle > 0 {
sqlDB.SetMaxIdleConns(maxIdle)
}
if maxLifetime > 0 {
sqlDB.SetConnMaxLifetime(maxLifetime)
}
if maxIdleTime > 0 {
sqlDB.SetConnMaxIdleTime(maxIdleTime)
}
return dialect, sqlDB, nil
}| func entDialect(dialectName string) string { | ||
| switch dialectName { | ||
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | ||
| return dialect.Postgres | ||
| case "sqlite3", "sqlite": | ||
| return dialect.SQLite | ||
| case "mysql", "tidb": | ||
| return dialect.MySQL | ||
| default: | ||
| return dialect.Postgres | ||
| } | ||
| } |
There was a problem hiding this comment.
The entDialect function silently defaults to dialect.Postgres for any unknown dialect. This is a regression from the previous implementation which panicked on invalid dialects. It should return an error for unsupported dialects to ensure the application fails fast with a clear message.
| func entDialect(dialectName string) string { | |
| switch dialectName { | |
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | |
| return dialect.Postgres | |
| case "sqlite3", "sqlite": | |
| return dialect.SQLite | |
| case "mysql", "tidb": | |
| return dialect.MySQL | |
| default: | |
| return dialect.Postgres | |
| } | |
| } | |
| func entDialect(dialectName string) (string, error) { | |
| switch dialectName { | |
| case "postgres", "pgx", "postgresdb", "pg", "postgresql": | |
| return dialect.Postgres, nil | |
| case "sqlite3", "sqlite": | |
| return dialect.SQLite, nil | |
| case "mysql", "tidb": | |
| return dialect.MySQL, nil | |
| default: | |
| return "", fmt.Errorf("invalid dialect: %s", dialectName) | |
| } | |
| } |
| func (d *routerDriver) Close() error { | ||
| if err := d.master.Close(); err != nil { | ||
| return err | ||
| } | ||
| if d.replica != nil { | ||
| return d.replica.Close() | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
In the Close method, if d.master.Close() returns an error, d.replica.Close() is never called. Both database handles should be attempted to be closed to avoid resource leaks.
| func (d *routerDriver) Close() error { | |
| if err := d.master.Close(); err != nil { | |
| return err | |
| } | |
| if d.replica != nil { | |
| return d.replica.Close() | |
| } | |
| return nil | |
| } | |
| func (d *routerDriver) Close() error { | |
| err1 := d.master.Close() | |
| var err2 error | |
| if d.replica != nil { | |
| err2 = d.replica.Close() | |
| } | |
| if err1 != nil { | |
| return err1 | |
| } | |
| return err2 | |
| } |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
/gemini review
…---Original---
From: ***@***.***>
Date: Mon, Apr 27, 2026 23:56 PM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [looplj/axonhub] feat: add optional read-write separation for database (PR #1516)
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request implements database read-write separation by introducing support for a read replica. The changes include new configuration options in conf/conf.go and config.example.yml, updated Helm charts, and comprehensive documentation for routing logic. A new routerDriver is introduced to handle the routing of SELECT queries to the replica while ensuring writes and transactions remain on the master. Review feedback focuses on improving the robustness of the database initialization logic, specifically addressing a regression where unsupported dialects silently default to Postgres, and ensuring that both master and replica connections are closed during shutdown to prevent resource leaks.
In internal/server/db/ent.go:
> +func openDB(dialectName, dsn string, maxOpen, maxIdle int, maxLifetime, maxIdleTime time.Duration) (string, *sql.DB, error) { + sqlDB, err := sql.Open(driverName(dialectName), dsn) + if err != nil { + return "", nil, err + } + + if maxOpen > 0 { + sqlDB.SetMaxOpenConns(maxOpen) + } + if maxIdle > 0 { + sqlDB.SetMaxIdleConns(maxIdle) + } + if maxLifetime > 0 { + sqlDB.SetConnMaxLifetime(maxLifetime) + } + if maxIdleTime > 0 { + sqlDB.SetConnMaxIdleTime(maxIdleTime) + } + + return entDialect(dialectName), sqlDB, nil +}
The openDB function should handle potential errors from entDialect and ensure that the sql.DB handle is closed if a dialect error occurs after the connection is opened. Currently, entDialect has a silent fallback to Postgres which is problematic.
func openDB(dialectName, dsn string, maxOpen, maxIdle int, maxLifetime, maxIdleTime time.Duration) (string, *sql.DB, error) { dialect, err := entDialect(dialectName) if err != nil { return "", nil, err } sqlDB, err := sql.Open(driverName(dialectName), dsn) if err != nil { return "", nil, err } if maxOpen > 0 { sqlDB.SetMaxOpenConns(maxOpen) } if maxIdle > 0 { sqlDB.SetMaxIdleConns(maxIdle) } if maxLifetime > 0 { sqlDB.SetConnMaxLifetime(maxLifetime) } if maxIdleTime > 0 { sqlDB.SetConnMaxIdleTime(maxIdleTime) } return dialect, sqlDB, nil }
In internal/server/db/ent.go:
> +func entDialect(dialectName string) string { + switch dialectName { + case "postgres", "pgx", "postgresdb", "pg", "postgresql": + return dialect.Postgres + case "sqlite3", "sqlite": + return dialect.SQLite + case "mysql", "tidb": + return dialect.MySQL + default: + return dialect.Postgres + } +}
The entDialect function silently defaults to dialect.Postgres for any unknown dialect. This is a regression from the previous implementation which panicked on invalid dialects. It should return an error for unsupported dialects to ensure the application fails fast with a clear message.
⬇️ Suggested change -func entDialect(dialectName string) string { - switch dialectName { - case "postgres", "pgx", "postgresdb", "pg", "postgresql": - return dialect.Postgres - case "sqlite3", "sqlite": - return dialect.SQLite - case "mysql", "tidb": - return dialect.MySQL - default: - return dialect.Postgres - } -} +func entDialect(dialectName string) (string, error) { + switch dialectName { + case "postgres", "pgx", "postgresdb", "pg", "postgresql": + return dialect.Postgres, nil + case "sqlite3", "sqlite": + return dialect.SQLite, nil + case "mysql", "tidb": + return dialect.MySQL, nil + default: + return "", fmt.Errorf("invalid dialect: %s", dialectName) + } +}
In internal/server/db/router.go:
> +func (d *routerDriver) Close() error { + if err := d.master.Close(); err != nil { + return err + } + if d.replica != nil { + return d.replica.Close() + } + return nil +}
In the Close method, if d.master.Close() returns an error, d.replica.Close() is never called. Both database handles should be attempted to be closed to avoid resource leaks.
⬇️ Suggested change -func (d *routerDriver) Close() error { - if err := d.master.Close(); err != nil { - return err - } - if d.replica != nil { - return d.replica.Close() - } - return nil -} +func (d *routerDriver) Close() error { + err1 := d.master.Close() + var err2 error + if d.replica != nil { + err2 = d.replica.Close() + } + if err1 != nil { + return err1 + } + return err2 +}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
There was a problem hiding this comment.
Code Review
This pull request implements read-write separation for the database by introducing a read replica configuration. It adds a routerDriver to the Ent client that automatically routes SELECT queries to the configured replica while directing all writes and transactions to the master database. The changes include updated configuration defaults, example YAML files, Helm charts, and documentation in both English and Chinese. One piece of feedback was provided to ensure that the read replica's connection pool settings inherit the master's defaults when not explicitly configured, preventing the replica from defaulting to unlimited connections.
| readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, | ||
| cfg.ReadReplica.MaxOpenConns, cfg.ReadReplica.MaxIdleConns, | ||
| cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime) |
There was a problem hiding this comment.
The read replica connection pool settings (read_max_open_conns and read_max_idle_conns) default to 0 in the configuration. In Go's sql.DB, a value of 0 for MaxOpenConns means unlimited connections. This is inconsistent with the master database, which defaults to 20 open and 10 idle connections. If a user configures a read replica but doesn't explicitly set its pool limits, the replica could potentially overwhelm the database server with unlimited connections. It is safer to inherit the master's pool settings if the replica-specific ones are not provided.
| readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, | |
| cfg.ReadReplica.MaxOpenConns, cfg.ReadReplica.MaxIdleConns, | |
| cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime) | |
| maxOpen := cfg.ReadReplica.MaxOpenConns | |
| if maxOpen == 0 { | |
| maxOpen = cfg.MaxOpenConns | |
| } | |
| maxIdle := cfg.ReadReplica.MaxIdleConns | |
| if maxIdle == 0 { | |
| maxIdle = cfg.MaxIdleConns | |
| } | |
| readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, | |
| maxOpen, maxIdle, | |
| cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime) |
|
If you don't fill in the read-only copy address, is there no problem with this? If the read-only copy address is filled in, people should see it and actively modify it (possibly
…---Original---
From: ***@***.***>
Date: Tue, Apr 28, 2026 00:06 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [looplj/axonhub] feat: add optional read-write separation for database (PR #1516)
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request implements read-write separation for the database by introducing a read replica configuration. It adds a routerDriver to the Ent client that automatically routes SELECT queries to the configured replica while directing all writes and transactions to the master database. The changes include updated configuration defaults, example YAML files, Helm charts, and documentation in both English and Chinese. One piece of feedback was provided to ensure that the read replica's connection pool settings inherit the master's defaults when not explicitly configured, preventing the replica from defaulting to unlimited connections.
In internal/server/db/ent.go:
> + readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, + cfg.ReadReplica.MaxOpenConns, cfg.ReadReplica.MaxIdleConns, + cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime)
The read replica connection pool settings (read_max_open_conns and read_max_idle_conns) default to 0 in the configuration. In Go's sql.DB, a value of 0 for MaxOpenConns means unlimited connections. This is inconsistent with the master database, which defaults to 20 open and 10 idle connections. If a user configures a read replica but doesn't explicitly set its pool limits, the replica could potentially overwhelm the database server with unlimited connections. It is safer to inherit the master's pool settings if the replica-specific ones are not provided.
⬇️ Suggested change - readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, - cfg.ReadReplica.MaxOpenConns, cfg.ReadReplica.MaxIdleConns, - cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime) +maxOpen := cfg.ReadReplica.MaxOpenConns +if maxOpen == 0 { + maxOpen = cfg.MaxOpenConns +} +maxIdle := cfg.ReadReplica.MaxIdleConns +if maxIdle == 0 { + maxIdle = cfg.MaxIdleConns +} +readDialect, replicaDB, err := openDB(cfg.Dialect, cfg.ReadReplica.DSN, + maxOpen, maxIdle, + cfg.ConnMaxLifetime, cfg.ConnMaxIdleTime)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Warning Gemini encountered an error creating the issue-comment-reply. You can try again by commenting |
|
Please confirm if it is tested in local, I will merge it if it is ready. |
|
it has already been produced in the existing CNPG
434836402
***@***.***
|
Summary
Add optional read-write separation for the database. When
read_replica.read_dsnis configured, read-only queries (SELECT/WITH, etc.) are automatically routed to the replica, while writes andtransactions always go to the master. When unconfigured, everything falls back to single-master mode with zero overhead.
Changes
ReadReplicaConfig(read_dsn,read_max_open_conns,read_max_idle_conns)NewEntClient, extractopenDB()/driverName()/entDialect(); enable router driver only whenread_dsnis non-emptyrouterDriverthat inspects the first SQL token for routing +masterTxto keep transactions on masterread_replicadefaults (empty/zero)Notes
client.Tx()) always go to master to avoid replication lagread_dsnto enable — zero business code changes