* postgres2: default the filemeta CREATE TABLE when createTable is unset
An empty createTable rendered through fmt.Sprintf produced
%!(EXTRA string=filemeta), which Postgres rejected with a syntax error at
init. Fall back to a working template so a minimal config bootstraps.
* mysql2: default the filemeta CREATE TABLE when createTable is unset
Same empty-template failure the postgres2 path had: an unset createTable
rendered to %!(EXTRA string=filemeta) and MySQL rejected it at init.
Fall back to a working template.
Concurrent bucket deletion across multiple filer replicas races on the
per-bucket DROP TABLE. The first replica drops the table; the rest hit
an undefined-table error (postgres 42P01, mysql 1051) which propagates
out of DeleteFolderChildren and panics the filer. On restart the same
pending DROP re-runs and the filer crash-loops.
Make the drop idempotent. Same defect class in all SQL backends, so fix
postgres, postgres2, mysql, mysql2, and sqlite together.
* filer: skip COLLATE "C" list fallback on CockroachDB
CockroachDB string comparison is already byte-ordered, so wrapping the
list queries in COLLATE "C" can never change result order. It reports
datcollate=en_US.utf8 regardless of how the database was created, so the
collation check always misfires and forces the fallback. On 22.1 and
older COLLATE "C" is rejected as an invalid locale, turning every filer
list query into a hard failure. Detect the backend via version() and
keep the default ordering.
* filer: test CockroachDB collation detection
Cover the CockroachDB skip path and the locale-aware Postgres force path
with go-sqlmock.
* mysql: keep S3 list order byte-lexicographic regardless of name column collation
ORDER BY name and the name > ? pagination predicate follow the column
collation, so a case-insensitive filemeta.name (e.g. utf8mb3_general_ci)
returns S3 keys out of byte order and breaks clients that merge two sorted
listings.
Detect the live name collation at startup; only when it isn't binary, wrap
the list comparison, prefix, and ORDER BY in BINARY name so order and
pagination stay consistent. Correctly configured utf8mb4_bin tables keep
their indexed range scan unchanged, and the operator gets a warning to
convert the column.
* postgres: keep S3 list order byte-lexicographic regardless of name column collation
ORDER BY name and the name > $n pagination predicate follow the column or
database collation, so a locale-aware filemeta.name (e.g. the en_US.UTF-8
database default) returns S3 keys out of byte order and breaks clients that
merge two sorted listings.
Detect the live name collation at startup; only when it isn't byte-ordered,
wrap the list comparison, prefix, and ORDER BY in COLLATE "C" so order and
pagination stay consistent. A byte-ordered (C/POSIX/C.UTF-8) column keeps its
indexed range scan unchanged, and the operator gets a warning to declare the
column COLLATE "C".
* fix(filer/postgres): default to ON CONFLICT upsert to keep tx alive
A KvPut from the inode-index secondary write could fail with 23505
(duplicate key) inside a rename's transaction, after which the next
statement returned 25P02 and rename surfaced to FUSE as EIO. Default
the postgres upsert query when enableUpsert=true so INSERTs are
idempotent; the enableUpsert=false escape hatch is preserved for
non-PG-compatible backends.
* fix(filer/mysql): default to ON DUPLICATE KEY UPDATE upsert
Same shape as the postgres default: when enableUpsert=true but no
upsertQuery is configured, install a sensible default so the
inode-index KvPut does not waste a duplicate-key roundtrip on every
entry write. Uses the VALUES() form so the default works on MariaDB
and MySQL >=5.7; the MySQL 8.0.19 row-alias form is left to explicit
config.
* fix(filer): default enableUpsert=true for sql stores
The default-template fallback only kicks in when enableUpsert=true,
so minimal configs that omit the flag entirely were still exposed.
Default it on for postgres/postgres2/mysql/mysql2; an explicit false
in filer.toml still wins because SetDefault only fills absent keys.
* fix(filer): don't disable the SQL idle connection pool when unconfigured
The mysql/mysql2/postgres stores called SetMaxIdleConns(maxIdle)
unconditionally, so an unset connection_max_idle (0) actively kept zero
idle connections - every query opened and closed a fresh connection
instead of reusing the pool.
Only apply the value when it's set; otherwise leave database/sql's
default idle pool of 2 in place.
* comments: shorten idle-pool note
* fix(filer): default the SQL idle pool via config, keep explicit 0 honored
Apply the idle-pool default at the config layer with SetDefault instead of
guarding the SetMaxIdleConns call. An absent connection_max_idle now reads
back as 2 (pool stays on), while an explicit 0 flows through to
SetMaxIdleConns(0) so operators can still disable idle pooling on purpose.
* refactor(util): extract pgx OpenDB + DSN builder into shared pgxutil
The postgres filer store had OpenPGXDB plus duplicated key=value DSN
assembly across postgres/ and postgres2/. Move the connection helper to
weed/util/pgxutil and add BuildDSN so the credential postgres store can
land on the same code path.
filer/postgres/pgx_conn.go keeps OpenPGXDB as a thin alias so postgres2
keeps building unchanged.
* refactor(credential/postgres): use shared pgxutil for connection setup
Replace the bespoke fmt.Sprintf DSN + sql.Open("pgx", ...) path with
pgxutil.BuildDSN + pgxutil.OpenDB so the credential store mirrors the
postgres filer store. This also drops the leaky RegisterConnConfig-style
init in favor of stdlib.OpenDB(*config), which doesn't accumulate
entries in the global pgx config map.
Adds parity knobs the filer store already exposes: sslcrl, and
configurable connection_max_idle / connection_max_open /
connection_max_lifetime_seconds (with the previous hardcoded 25/5/5min
as defaults). Also moves the jsonbParam helper here so other store
files can reuse it. (Helper is also referenced by postgres_identity.go,
which is migrated to it in the next commit.)
* refactor(credential/postgres): use jsonbParam helper across all writers
Consolidate JSONB write handling on the new pgxutil-adjacent helper
jsonbParam(b []byte) interface{}, which returns nil (driver writes SQL
NULL) when the marshaled JSON is empty and string(b) otherwise.
postgres_identity.go: replace the inline 'var fooParam any' /
'fooParam = string(b)' pattern with the helper. Same in CreateUser
and UpdateUser.
postgres_inline_policy.go, postgres_policy.go, postgres_service_account.go,
postgres_group.go: every JSONB writer was still passing []byte. Under
pgx simple_protocol (pgbouncer_compatible=true), []byte is encoded as
bytea and Postgres rejects that against a JSONB column with "invalid
input syntax for type json". Route them through jsonbParam too.
* fix(credential/postgres): rework SaveConfiguration to handle rename + UNIQUE access keys
The IAM rename path (s3api UpdateUser) renames an identity in place
and keeps its access keys. With the previous flow — upsert each user,
then per-user delete-and-insert credentials, then prune absent users —
the renamed user's access keys were still owned by the old row when
the INSERT for the new name ran, tripping credentials.access_key's
global UNIQUE constraint and failing every rename of a user with
credentials.
Reorder the SaveConfiguration body so the prune step runs BEFORE the
credential replace. CASCADE on the old user releases its access keys
in the same transaction, and the new name can then claim them.
While here:
- Replace the per-user loop DELETE FROM users WHERE username = $1 with
a single DELETE ... WHERE username = ANY($1), one round trip instead
of N inside the transaction.
- Surface inline-policy CASCADE losses: count user_inline_policies for
the prune set and emit a Warningf when the count is non-zero so
rename-driven drops are visible in operator logs (the structural
fix for renames lives at the IAM layer in a follow-up commit).
- Two-pass credential replace: clear credentials for every user we are
about to rewrite first, then insert, so an access key can be moved
between two users in the same SaveConfiguration call.
- credErr := credRows.Err() before credRows.Close() in
LoadConfiguration — Err() is documented as safe after Close, but
the leading-capture pattern matches the rest of the file.
* fix(s3api/iam): preserve inline policies when renaming a user
EmbeddedIamApi.UpdateUser renames an identity in place and the caller
persists via SaveConfiguration, which prunes the old username and
CASCADE-drops its rows from user_inline_policies. GetUserPolicy and
ListUserPolicies then return nothing under the new name even though
the API reported success — silent data loss.
Before flipping sourceIdent.Name, list the user's stored inline
policies and re-attach each one under the new name. The subsequent
SaveConfiguration prune still CASCADE-removes the old-name rows; only
the duplicates we just wrote under the new name survive. Adds a
regression test that puts a policy on the old name, renames, and
asserts the policy is readable under the new name.
* perf(credential/postgres): batch the credential clear in SaveConfiguration
The two-pass credential replace was clearing each incoming user's
credentials with its own DELETE statement — N round-trips inside the
transaction. Match the pattern already used for the user prune and
issue a single DELETE FROM credentials WHERE username = ANY($1)
instead.
* refactor(s3api/iam): plumb context through UpdateUser
UpdateUser was synthesizing a fresh context.Background() inside the
inline-policy migration block, which discards the request deadline,
cancellation, and tracing carried by the caller. Add ctx as the first
parameter and pass r.Context() in via the ExecuteAction dispatcher,
mirroring the signature already used by CreatePolicy /
AttachUserPolicy / DetachUserPolicy.
* fix(util/pgxutil): quote DSN values per libpq rules
BuildDSN was concatenating values directly, so any password / cert path
/ database name with a space, single quote, or backslash produced a
malformed connection string and pgx.ParseConfig either errored or
mis-parsed the remainder. Critical now that the helper is shared with
the credential store: mTLS deployments routinely sourcing passwords or
secret-mounted cert paths from a vault are exactly the case where
spaces and quotes show up.
Add quoteDSNValue: empty values and values containing whitespace, `'`,
or `\` are wrapped in single quotes with `'` and `\` escaped per
PostgreSQL libpq rules; plain alphanumeric values pass through
unchanged. Apply it to every variable field in BuildDSN.
Adds a test that round-trips a password containing spaces, quotes and
backslashes through pgx.ParseConfig and confirms the parsed Config
matches the input.
* fix(credential,s3api/iam): atomic UserRenamer to avoid FK violation on rename
The previous IAM rename path called PutUserInlinePolicy(newName, ...)
before SaveConfiguration created the new users row. user_inline_policies
has a non-deferrable FOREIGN KEY (username) REFERENCES users(username),
which Postgres validates at statement time, so every rename of a user
that owned at least one inline policy failed with an FK violation. The
existing memory-store regression test missed it because the memory
backend has no FK enforcement.
Add an optional credential.UserRenamer interface plus a
CredentialManager.RenameUser thin shim that returns (supported, err).
Implement it on PostgresStore as an atomic in-transaction migration:
INSERT the new users row by SELECT-copying from the old, UPDATE
credentials.username and user_inline_policies.username to the new
name (FK satisfied because both rows now exist), then DELETE the old
row. ErrUserNotFound / ErrUserAlreadyExists are surfaced cleanly.
Implement it on MemoryStore by re-binding store.users / store.accessKeys
/ store.inlinePolicies under the new name. Also fixes a small leak in
DeleteUser, which was forgetting to drop the user's inline-policy
bucket.
EmbeddedIamApi.UpdateUser now calls RenameUser first; if the store
implements the interface, that's the whole migration. If it doesn't
(stores without FK enforcement), fall back to the previous
list / get / put copy.
Adds a focused test for MemoryStore.RenameUser that asserts the
identity, the access-key index, and the inline policies all land
under the new name.
* fix(filer/postgres): use pgx v5 API for PgBouncer simple protocol
In pgx/v5 the `prefer_simple_protocol` DSN parameter was removed, so
appending it to the connection string caused PgBouncer/PostgreSQL to
reject it as an unknown startup parameter:
FATAL: unsupported startup parameter: prefer_simple_protocol (SQLSTATE 08P01)
Parse the DSN with pgx.ParseConfig and, when pgbouncer_compatible is
set, configure DefaultQueryExecMode = QueryExecModeSimpleProtocol and
disable the statement/description caches. Register the config via
stdlib.RegisterConnConfig before sql.Open.
Fixes#9005
* refactor(filer/postgres): extract shared OpenPGXDB helper with cleanup
Extract the pgx v5 ParseConfig/RegisterConnConfig/sql.Open/Ping logic
into a shared postgres.OpenPGXDB helper used by both postgres and
postgres2 filer stores, eliminating ~60 lines of duplication.
The helper also unregisters the conn config via stdlib.UnregisterConnConfig
on every failure path (sql.Open error, Ping error) so we do not leak
entries in stdlib's global connection config map when initialization
fails.
* refactor(filer/postgres): use stdlib.OpenDB to avoid conn config leak
Switch OpenPGXDB from RegisterConnConfig + sql.Open("pgx", connStr) to
stdlib.OpenDB(*connConfig). The former leaks an entry in stdlib's global
conn config map on every successful initialization; stdlib.OpenDB takes
the config directly and keeps no global registration.
Addresses CodeRabbit review feedback on #9010.
Chris Lu
chrislu
John W Higgins
Chris Lu
LazyDBA247-Anyvision