From 21b4b81edb26c04240b626fe60076014add6d35e Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 27 May 2026 12:23:30 -0700 Subject: [PATCH] fix(filer/postgres): default to ON CONFLICT upsert to keep tx alive (#9709) * 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. --- weed/filer/mysql/mysql_sql_gen.go | 7 +++++ weed/filer/mysql/mysql_sql_gen_test.go | 28 ++++++++++++++++++++ weed/filer/mysql/mysql_store.go | 5 ++++ weed/filer/mysql2/mysql2_store.go | 5 ++++ weed/filer/postgres/postgres_sql_gen.go | 5 ++++ weed/filer/postgres/postgres_sql_gen_test.go | 25 +++++++++++++++++ weed/filer/postgres/postgres_store.go | 5 ++++ weed/filer/postgres2/postgres2_store.go | 5 ++++ 8 files changed, 85 insertions(+) create mode 100644 weed/filer/mysql/mysql_sql_gen_test.go create mode 100644 weed/filer/postgres/postgres_sql_gen_test.go diff --git a/weed/filer/mysql/mysql_sql_gen.go b/weed/filer/mysql/mysql_sql_gen.go index a2e07002b..8e77bab8a 100644 --- a/weed/filer/mysql/mysql_sql_gen.go +++ b/weed/filer/mysql/mysql_sql_gen.go @@ -13,6 +13,13 @@ type SqlGenMysql struct { UpsertQueryTemplate string } +// DefaultUpsertQuery keeps INSERTs idempotent so the inode-index KvPut +// after every entry write does not emit a duplicate-key roundtrip on +// every mutation. The VALUES() form works on MariaDB and MySQL >=5.7; +// the newer "AS new" alias from MySQL 8.0.19 errors on MariaDB, so it +// is left for users to opt into via an explicit upsertQuery. +const DefaultUpsertQuery = "INSERT INTO `%s` (`dirhash`,`name`,`directory`,`meta`) VALUES(?,?,?,?) ON DUPLICATE KEY UPDATE `meta` = VALUES(`meta`)" + var ( _ = abstract_sql.SqlGenerator(&SqlGenMysql{}) ) diff --git a/weed/filer/mysql/mysql_sql_gen_test.go b/weed/filer/mysql/mysql_sql_gen_test.go new file mode 100644 index 000000000..74303b764 --- /dev/null +++ b/weed/filer/mysql/mysql_sql_gen_test.go @@ -0,0 +1,28 @@ +package mysql + +import ( + "strings" + "testing" +) + +func TestDefaultUpsertQueryUsesOnDuplicateKey(t *testing.T) { + gen := &SqlGenMysql{UpsertQueryTemplate: DefaultUpsertQuery} + got := gen.GetSqlInsert("filemeta") + if !strings.Contains(got, "ON DUPLICATE KEY UPDATE") { + t.Fatalf("expected ON DUPLICATE KEY UPDATE in default upsert, got: %s", got) + } + if strings.Contains(got, "AS `new`") { + t.Fatalf("default should avoid MySQL 8.0.19 row-alias syntax for MariaDB compat, got: %s", got) + } + if !strings.Contains(got, "`filemeta`") { + t.Fatalf("expected backticked table name, got: %s", got) + } +} + +func TestEmptyUpsertTemplateFallsBackToPlainInsert(t *testing.T) { + gen := &SqlGenMysql{} + got := gen.GetSqlInsert("filemeta") + if strings.Contains(got, "ON DUPLICATE KEY UPDATE") { + t.Fatalf("plain INSERT path should not contain ON DUPLICATE KEY UPDATE, got: %s", got) + } +} diff --git a/weed/filer/mysql/mysql_store.go b/weed/filer/mysql/mysql_store.go index 2e83e7a4d..b5b5ef025 100644 --- a/weed/filer/mysql/mysql_store.go +++ b/weed/filer/mysql/mysql_store.go @@ -35,6 +35,9 @@ func (store *MysqlStore) GetName() string { func (store *MysqlStore) Initialize(configuration util.Configuration, prefix string) (err error) { // Absent key keeps a pooled default; an explicit 0 disables the idle pool. configuration.SetDefault(prefix+"connection_max_idle", 2) + // Default on so minimal configs avoid the duplicate-key roundtrip the + // inode-index KvPut would otherwise emit on every write. + configuration.SetDefault(prefix+"enableUpsert", true) return store.initialize( configuration.GetString(prefix+"dsn"), configuration.GetString(prefix+"upsertQuery"), @@ -64,6 +67,8 @@ func (store *MysqlStore) initialize(dsn string, upsertQuery string, enableUpsert store.SupportBucketTable = false if !enableUpsert { upsertQuery = "" + } else if upsertQuery == "" { + upsertQuery = DefaultUpsertQuery } store.SqlGenerator = &SqlGenMysql{ CreateTableSqlTemplate: "", diff --git a/weed/filer/mysql2/mysql2_store.go b/weed/filer/mysql2/mysql2_store.go index 82d0c86c2..3dde672b0 100644 --- a/weed/filer/mysql2/mysql2_store.go +++ b/weed/filer/mysql2/mysql2_store.go @@ -35,6 +35,9 @@ func (store *MysqlStore2) GetName() string { func (store *MysqlStore2) Initialize(configuration util.Configuration, prefix string) (err error) { // Absent key keeps a pooled default; an explicit 0 disables the idle pool. configuration.SetDefault(prefix+"connection_max_idle", 2) + // Default on so minimal configs avoid the duplicate-key roundtrip the + // inode-index KvPut would otherwise emit on every write. + configuration.SetDefault(prefix+"enableUpsert", true) return store.initialize( configuration.GetString(prefix+"createTable"), configuration.GetString(prefix+"upsertQuery"), @@ -57,6 +60,8 @@ func (store *MysqlStore2) initialize(createTable, upsertQuery string, enableUpse store.SupportBucketTable = true if !enableUpsert { upsertQuery = "" + } else if upsertQuery == "" { + upsertQuery = mysql.DefaultUpsertQuery } store.SqlGenerator = &mysql.SqlGenMysql{ CreateTableSqlTemplate: createTable, diff --git a/weed/filer/postgres/postgres_sql_gen.go b/weed/filer/postgres/postgres_sql_gen.go index 8832e1a45..89696d810 100644 --- a/weed/filer/postgres/postgres_sql_gen.go +++ b/weed/filer/postgres/postgres_sql_gen.go @@ -13,6 +13,11 @@ type SqlGenPostgres struct { UpsertQueryTemplate string } +// DefaultUpsertQuery keeps INSERTs idempotent so a duplicate-key failure +// (23505) cannot poison the surrounding transaction (25P02). Used when the +// user enables upsert but does not provide their own template. +const DefaultUpsertQuery = `INSERT INTO "%s" (dirhash,name,directory,meta) VALUES($1,$2,$3,$4) ON CONFLICT (dirhash, name) DO UPDATE SET directory=EXCLUDED.directory, meta=EXCLUDED.meta` + var ( _ = abstract_sql.SqlGenerator(&SqlGenPostgres{}) ) diff --git a/weed/filer/postgres/postgres_sql_gen_test.go b/weed/filer/postgres/postgres_sql_gen_test.go new file mode 100644 index 000000000..16039803c --- /dev/null +++ b/weed/filer/postgres/postgres_sql_gen_test.go @@ -0,0 +1,25 @@ +package postgres + +import ( + "strings" + "testing" +) + +func TestDefaultUpsertQueryIsConflictSafe(t *testing.T) { + gen := &SqlGenPostgres{UpsertQueryTemplate: DefaultUpsertQuery} + got := gen.GetSqlInsert("filemeta") + if !strings.Contains(got, "ON CONFLICT") { + t.Fatalf("expected ON CONFLICT in default upsert, got: %s", got) + } + if !strings.Contains(got, `"filemeta"`) { + t.Fatalf("expected quoted table name, got: %s", got) + } +} + +func TestEmptyUpsertTemplateFallsBackToPlainInsert(t *testing.T) { + gen := &SqlGenPostgres{} + got := gen.GetSqlInsert("filemeta") + if strings.Contains(got, "ON CONFLICT") { + t.Fatalf("plain INSERT path should not contain ON CONFLICT, got: %s", got) + } +} diff --git a/weed/filer/postgres/postgres_store.go b/weed/filer/postgres/postgres_store.go index fa6cb7df6..97f90b83f 100644 --- a/weed/filer/postgres/postgres_store.go +++ b/weed/filer/postgres/postgres_store.go @@ -30,6 +30,9 @@ func (store *PostgresStore) GetName() string { func (store *PostgresStore) Initialize(configuration util.Configuration, prefix string) (err error) { // Absent key keeps a pooled default; an explicit 0 disables the idle pool. configuration.SetDefault(prefix+"connection_max_idle", 2) + // Default on so minimal configs are not exposed to duplicate-key tx + // poisoning on Postgres; an explicit false still disables it. + configuration.SetDefault(prefix+"enableUpsert", true) return store.initialize( configuration.GetString(prefix+"upsertQuery"), configuration.GetBool(prefix+"enableUpsert"), @@ -56,6 +59,8 @@ func (store *PostgresStore) initialize(upsertQuery string, enableUpsert bool, us store.SupportBucketTable = false if !enableUpsert { upsertQuery = "" + } else if upsertQuery == "" { + upsertQuery = DefaultUpsertQuery } store.SqlGenerator = &SqlGenPostgres{ CreateTableSqlTemplate: "", diff --git a/weed/filer/postgres2/postgres2_store.go b/weed/filer/postgres2/postgres2_store.go index 64e86b9da..3861e0689 100644 --- a/weed/filer/postgres2/postgres2_store.go +++ b/weed/filer/postgres2/postgres2_store.go @@ -35,6 +35,9 @@ func (store *PostgresStore2) GetName() string { func (store *PostgresStore2) Initialize(configuration util.Configuration, prefix string) (err error) { // Absent key keeps a pooled default; an explicit 0 disables the idle pool. configuration.SetDefault(prefix+"connection_max_idle", 2) + // Default on so minimal configs are not exposed to duplicate-key tx + // poisoning on Postgres; an explicit false still disables it. + configuration.SetDefault(prefix+"enableUpsert", true) return store.initialize( configuration.GetString(prefix+"createTable"), configuration.GetString(prefix+"upsertQuery"), @@ -62,6 +65,8 @@ func (store *PostgresStore2) initialize(createTable, upsertQuery string, enableU store.SupportBucketTable = true if !enableUpsert { upsertQuery = "" + } else if upsertQuery == "" { + upsertQuery = postgres.DefaultUpsertQuery } store.SqlGenerator = &postgres.SqlGenPostgres{ CreateTableSqlTemplate: createTable,