Skip to content

feat(core): Add statement timeout parameter. #3544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
c-r33d wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(core): Add statement timeout parameter. #3544

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83d9202
feat(core): Add statement timeout parameter.
c-r33d May 28, 2026
81ff8de
change to statement_timeout_seconds.
c-r33d May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion opentdf-core-mode.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ logger:
# port: 5432
# user: postgres
# password: changeme
# statement_timeout: 30s
Comment thread
c-r33d marked this conversation as resolved.
Outdated
server:
auth:
enabled: false
Expand Down Expand Up @@ -57,4 +58,4 @@ server:
maxage: 3600
grpc:
reflectionEnabled: true # Default is false
port: 8383
port: 8383
1 change: 1 addition & 0 deletions opentdf-dev.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ logger:
# password: changeme
# sslmode: prefer
# connect_timeout_seconds: 15
# statement_timeout: 30s
Comment thread
c-r33d marked this conversation as resolved.
Outdated
# pool:
# max_connection_count: 4
# min_connection_count: 0
Expand Down
1 change: 1 addition & 0 deletions opentdf-example.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ db:
# password: changeme
# sslmode: prefer
# connect_timeout_seconds: 15
# statement_timeout: 30s
Comment thread
c-r33d marked this conversation as resolved.
Outdated
# pool:
# max_connection_count: 4
# min_connection_count: 0
Expand Down
24 changes: 15 additions & 9 deletions service/pkg/db/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,16 @@ type PoolConfig struct {
}

type Config struct {
Host string `mapstructure:"host" json:"host" default:"localhost"`
Port int `mapstructure:"port" json:"port" default:"5432"`
Database string `mapstructure:"database" json:"database" default:"opentdf"`
User string `mapstructure:"user" json:"user" default:"postgres"`
Password string `mapstructure:"password" json:"password" default:"changeme"`
SSLMode string `mapstructure:"sslmode" json:"sslmode" default:"prefer"`
Schema string `mapstructure:"schema" json:"schema" default:"opentdf"`
ConnectTimeout int `mapstructure:"connect_timeout_seconds" json:"connect_timeout_seconds" default:"15"`
Pool PoolConfig `mapstructure:"pool" json:"pool"`
Host string `mapstructure:"host" json:"host" default:"localhost"`
Port int `mapstructure:"port" json:"port" default:"5432"`
Database string `mapstructure:"database" json:"database" default:"opentdf"`
User string `mapstructure:"user" json:"user" default:"postgres"`
Password string `mapstructure:"password" json:"password" default:"changeme"`
SSLMode string `mapstructure:"sslmode" json:"sslmode" default:"prefer"`
Schema string `mapstructure:"schema" json:"schema" default:"opentdf"`
ConnectTimeout int `mapstructure:"connect_timeout_seconds" json:"connect_timeout_seconds" default:"15"`
StatementTimeout string `mapstructure:"statement_timeout" json:"statement_timeout"`
Comment thread
c-r33d marked this conversation as resolved.
Outdated
Pool PoolConfig `mapstructure:"pool" json:"pool"`

RunMigrations bool `mapstructure:"runMigrations" json:"runMigrations" default:"true"`
MigrationsFS *embed.FS `mapstructure:"-" json:"-"`
Expand All @@ -114,6 +115,7 @@ func (c Config) LogValue() slog.Value {
slog.String("sslmode", c.SSLMode),
slog.String("schema", c.Schema),
slog.Int("connect_timeout_seconds", c.ConnectTimeout),
slog.String("statement_timeout", c.StatementTimeout),
Comment thread
c-r33d marked this conversation as resolved.
Outdated
slog.Group("pool",
slog.Int("max_connection_count", int(c.Pool.MaxConns)),
slog.Int("min_connection_count", int(c.Pool.MinConns)),
Expand Down Expand Up @@ -250,6 +252,10 @@ func (c Config) buildConfig() (*pgxpool.Config, error) {
parsed.MaxConnIdleTime = time.Duration(c.Pool.MaxConnIdleTime) * time.Second
parsed.HealthCheckPeriod = time.Duration(c.Pool.HealthCheckPeriod) * time.Second

if c.StatementTimeout != "" {
parsed.ConnConfig.RuntimeParams["statement_timeout"] = c.StatementTimeout
}
Comment thread
c-r33d marked this conversation as resolved.
Outdated
Comment on lines +255 to +257

@coderabbitai coderabbitai Bot May 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# How are migrations run? Do they reuse the pool/SQLDB built from buildConfig?
rg -nP --type=go -C4 '\b(RunMigrations|runMigrations|Migrate|migrat)' service/pkg/db
# Find the migration runner entrypoint and what conn/pool it uses
ast-grep --pattern 'func ($_ $_) RunMigrations($$$) { $$$ }'

Repository: opentdf/platform

Length of output: 10075

statement_timeout is pool-wide and will also affect migrations/admin work

The statement_timeout startup runtime param applies to every statement executed via this client’s shared pgxpool.Pool:

  • migrationInit casts c.Pgx to *pgxpool.Pool and creates the migrations DB handle with stdlib.OpenDBFromPool(pool), so Goose runs migrations on connections from the same pool that carries RuntimeParams["statement_timeout"].
  • This means a pool-wide timeout could fail slow startup migrations/backfills, not just the intended List RPC queries.
  • "%ds" formatting is correct for PostgreSQL statement_timeout (unit-suffixed seconds like "30s").
if c.StatementTimeoutSeconds > 0 {
	parsed.ConnConfig.RuntimeParams["statement_timeout"] = fmt.Sprintf("%ds", c.StatementTimeoutSeconds)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/pkg/db/db.go` around lines 255 - 257, The code currently sets
parsed.ConnConfig.RuntimeParams["statement_timeout"] using
c.StatementTimeoutSeconds which applies a pool-wide timeout and will also affect
migrations/admin work (see migrationInit which casts c.Pgx to *pgxpool.Pool and
uses stdlib.OpenDBFromPool). Remove the RuntimeParams assignment and instead
apply the timeout only where needed: use context.WithTimeout around RPC
handlers/queries that require a per-request timeout (or run "SET LOCAL
statement_timeout = 'Ns'" at the start of the specific transaction) in the
List-query code paths; keep references to c.StatementTimeoutSeconds,
parsed.ConnConfig.RuntimeParams, migrationInit, c.Pgx and stdlib.OpenDBFromPool
to locate the related code.

@jakedoublev jakedoublev Jun 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point. Should we override this client setting while running migrations? @c-r33d

@coderabbitai coderabbitai Bot Jun 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@c-r33d c-r33d Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I actually was rethinking how I was going to do this because of the coderabbit comment. The problem is that we run migrations during startup and that uses the pool that where we have already specified. I will check if we can override this with a command-level SET.


// Configure the search_path schema immediately on connection opening
parsed.AfterConnect = func(ctx context.Context, conn *pgx.Conn) error {
_, err := conn.Exec(ctx, "SET search_path TO "+pgx.Identifier{c.Schema}.Sanitize())
Expand Down
43 changes: 41 additions & 2 deletions service/pkg/db/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ import (

func Test_BuildConfig_ConnString(t *testing.T) {
tests := []struct {
config *Config
want string
config *Config
want string
wantRuntimeParams map[string]string
}{
{
config: &Config{
Expand Down Expand Up @@ -63,6 +64,36 @@ func Test_BuildConfig_ConnString(t *testing.T) {
},
want: "postgres://myuser:mypassword@myhost:1234/mydb?sslmode=require",
},
// Statement timeout should not be added as a runtime parameter unless configured.
{
config: &Config{
Host: "localhost",
Port: 5432,
Database: "opentdf",
User: "postgres",
Password: "changeme",
SSLMode: "prefer",
},
want: "postgres://postgres:changeme@localhost:5432/opentdf?sslmode=prefer",
wantRuntimeParams: map[string]string{
"statement_timeout": "",
},
},
{
config: &Config{
Host: "localhost",
Port: 5432,
Database: "opentdf",
User: "postgres",
Password: "changeme",
SSLMode: "prefer",
StatementTimeout: "30s",
Comment thread
c-r33d marked this conversation as resolved.
Outdated
},
want: "postgres://postgres:changeme@localhost:5432/opentdf?sslmode=prefer",
wantRuntimeParams: map[string]string{
"statement_timeout": "30s",
},
},
}

for _, test := range tests {
Expand All @@ -71,5 +102,13 @@ func Test_BuildConfig_ConnString(t *testing.T) {
assert.Equal(t, test.want, cfg.ConnString())
// AfterConnect hook was defined when building
assert.NotNil(t, cfg.AfterConnect)

for key, value := range test.wantRuntimeParams {
if value == "" {
assert.NotContains(t, cfg.ConnConfig.RuntimeParams, key)
continue
}
assert.Equal(t, value, cfg.ConnConfig.RuntimeParams[key])
}
}
}
Loading