Skip to content

Commit 1f2b4a9

Browse files
committed
fix(repair): make pick_freshest compare commit_ts from spock metadata
pick_freshest resolved its `key` only from the stripped diff row, but commit_ts (and other spock metadata) are moved into n1Meta/n2Meta during diff loading. So `pick_freshest: {key: commit_ts}` found nil on both sides and fell back to `tie` every time, never actually comparing timestamps. Resolve the key via lookupValue, which falls back to the metadata map, so `key: commit_ts` compares commit timestamps while ordinary data columns (e.g. updated_at) keep working. Tests: - unit: commit_ts-from-meta (both directions), one-side-null (the side with a timestamp wins), both-null (tie), and app-column-from-row. - integration: one-pass latest-commit-wins end-to-end (newer commit_ts side wins) and pick_freshest by an app-level column (subscription_date).
1 parent 5f23d5a commit 1f2b4a9

3 files changed

Lines changed: 236 additions & 9 deletions

File tree

internal/consistency/repair/executor.go

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -723,15 +723,11 @@ func unionKeys(maps ...map[string]any) map[string]struct{} {
723723
}
724724

725725
func freshestSide(key string, tie string, row planDiffRow) string {
726-
val1 := row.n1Row
727-
val2 := row.n2Row
728-
var k1, k2 any
729-
if val1 != nil {
730-
k1 = val1[key]
731-
}
732-
if val2 != nil {
733-
k2 = val2[key]
734-
}
726+
// Resolve the key via lookupValue so it works for both ordinary data
727+
// columns (in n1Row/n2Row) and spock metadata like commit_ts, which the
728+
// executor strips from the row and keeps in n1Meta/n2Meta.
729+
k1, _ := lookupValue("n1", key, row)
730+
k2, _ := lookupValue("n2", key, row)
735731
n1Ok := k1 != nil
736732
n2Ok := k2 != nil
737733
if !n1Ok && n2Ok {
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// ///////////////////////////////////////////////////////////////////////////
2+
//
3+
// # ACE - Active Consistency Engine
4+
//
5+
// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/)
6+
//
7+
// This software is released under the PostgreSQL License:
8+
// https://opensource.org/license/postgresql
9+
//
10+
// ///////////////////////////////////////////////////////////////////////////
11+
12+
package repair
13+
14+
import (
15+
"testing"
16+
17+
"github.com/stretchr/testify/require"
18+
)
19+
20+
// commit_ts is carried in the spock metadata map (_spock_metadata_), which the
21+
// executor keeps in n1Meta/n2Meta while stripping it from n1Row/n2Row. These
22+
// tests ensure pick_freshest's freshestSide resolves the key from metadata too,
23+
// so `key: commit_ts` actually compares commit timestamps instead of always
24+
// falling through to the tie node.
25+
func TestFreshestSide_CommitTsFromMeta(t *testing.T) {
26+
older := "2026-01-15T12:20:07.805412-08:00"
27+
newer := "2026-01-15T12:21:39.662530-08:00"
28+
29+
t.Run("n2 newer", func(t *testing.T) {
30+
row := planDiffRow{
31+
n1Row: map[string]any{"id": 1, "v": "a"},
32+
n2Row: map[string]any{"id": 1, "v": "b"},
33+
n1Meta: map[string]any{"commit_ts": older},
34+
n2Meta: map[string]any{"commit_ts": newer},
35+
}
36+
require.Equal(t, "n2", freshestSide("commit_ts", "n1", row))
37+
})
38+
39+
t.Run("n1 newer", func(t *testing.T) {
40+
row := planDiffRow{
41+
n1Row: map[string]any{"id": 1, "v": "a"},
42+
n2Row: map[string]any{"id": 1, "v": "b"},
43+
n1Meta: map[string]any{"commit_ts": newer},
44+
n2Meta: map[string]any{"commit_ts": older},
45+
}
46+
require.Equal(t, "n1", freshestSide("commit_ts", "n2", row))
47+
})
48+
}
49+
50+
// When only one side has a commit_ts (e.g. the other was written with
51+
// track_commit_timestamp off), the side that has one wins; when neither has
52+
// one, the tie node is chosen.
53+
func TestFreshestSide_CommitTsNullHandling(t *testing.T) {
54+
ts := "2026-01-15T12:21:39.662530-08:00"
55+
56+
t.Run("only n2 has commit_ts -> n2", func(t *testing.T) {
57+
row := planDiffRow{
58+
n1Meta: map[string]any{},
59+
n2Meta: map[string]any{"commit_ts": ts},
60+
}
61+
require.Equal(t, "n2", freshestSide("commit_ts", "n1", row))
62+
})
63+
64+
t.Run("only n1 has commit_ts -> n1", func(t *testing.T) {
65+
row := planDiffRow{
66+
n1Meta: map[string]any{"commit_ts": ts},
67+
n2Meta: map[string]any{},
68+
}
69+
require.Equal(t, "n1", freshestSide("commit_ts", "n2", row))
70+
})
71+
72+
t.Run("neither has commit_ts -> tie", func(t *testing.T) {
73+
row := planDiffRow{
74+
n1Meta: map[string]any{},
75+
n2Meta: map[string]any{},
76+
}
77+
require.Equal(t, "n2", freshestSide("commit_ts", "n2", row))
78+
})
79+
}
80+
81+
// An application-level timestamp column lives in the row itself and must keep
82+
// working after the metadata fallback is added.
83+
func TestFreshestSide_AppColumnFromRow(t *testing.T) {
84+
row := planDiffRow{
85+
n1Row: map[string]any{"updated_at": "2026-01-15T12:00:00-08:00"},
86+
n2Row: map[string]any{"updated_at": "2026-01-15T13:00:00-08:00"},
87+
}
88+
require.Equal(t, "n2", freshestSide("updated_at", "n1", row))
89+
}

tests/integration/advanced_repair_test.go

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"testing"
2121
"time"
2222

23+
"github.com/jackc/pgx/v5/pgxpool"
2324
"github.com/stretchr/testify/require"
2425
)
2526

@@ -212,6 +213,147 @@ tables:
212213
require.True(t, hasStaleSkipEntry(entries, 2001, "delete", "stale_delete_missing_on_n1"))
213214
}
214215

216+
// TestAdvancedRepairPlan_PickFreshestByCommitTS verifies the one-pass
217+
// latest-commit-wins plan: pick_freshest with key: commit_ts keeps the row from
218+
// the side with the newer Spock commit timestamp. It also guards the regression
219+
// where pick_freshest read commit_ts from the (stripped) row instead of the
220+
// spock metadata and therefore always fell back to `tie`.
221+
func TestAdvancedRepairPlan_PickFreshestByCommitTS(t *testing.T) {
222+
ctx := context.Background()
223+
qualifiedTableName := "public.customers"
224+
225+
setupDivergence(t, ctx, qualifiedTableName)
226+
diffFile := runTableDiff(t, qualifiedTableName, []string{serviceN1, serviceN2})
227+
228+
// Precondition: setupDivergence UPDATEs index 1 & 2 on n2 after the initial
229+
// inserts, so n2's commit_ts for those rows is strictly newer than n1's.
230+
// That ordering is what "latest commit wins" must resolve to.
231+
for _, idx := range []int{1, 2} {
232+
var tsN1, tsN2 time.Time
233+
require.NoError(t, pgCluster.Node1Pool.QueryRow(ctx,
234+
fmt.Sprintf("SELECT pg_xact_commit_timestamp(xmin) FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&tsN1))
235+
require.NoError(t, pgCluster.Node2Pool.QueryRow(ctx,
236+
fmt.Sprintf("SELECT pg_xact_commit_timestamp(xmin) FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&tsN2))
237+
require.Truef(t, tsN2.After(tsN1),
238+
"precondition for index %d: n2 commit_ts %s should be newer than n1 %s", idx, tsN2, tsN1)
239+
}
240+
241+
// One-pass plan: pick_freshest by commit_ts resolves the modified rows to
242+
// the newer (n2) side; apply_from rules propagate the single-sided inserts
243+
// so the tables fully converge.
244+
plan := `
245+
version: 1
246+
tables:
247+
public.customers:
248+
rules:
249+
- name: latest_commit_wins
250+
diff_type: [row_mismatch]
251+
action:
252+
type: custom
253+
helpers:
254+
pick_freshest: { key: commit_ts, tie: n1 }
255+
- name: insert_missing_on_n2
256+
diff_type: [missing_on_n2]
257+
action: { type: apply_from, from: n1, mode: insert }
258+
- name: insert_missing_on_n1
259+
diff_type: [missing_on_n1]
260+
action: { type: apply_from, from: n2, mode: insert }
261+
`
262+
planPath := filepath.Join(t.TempDir(), "repair.yaml")
263+
require.NoError(t, os.WriteFile(planPath, []byte(plan), 0o644))
264+
265+
task := newTestTableRepairTask("", qualifiedTableName, diffFile)
266+
task.RepairPlanPath = planPath
267+
require.NoError(t, task.ValidateAndPrepare())
268+
require.NoError(t, task.Run(true))
269+
270+
assertNoTableDiff(t, qualifiedTableName)
271+
272+
// The newer (n2) side won for the modified rows, on BOTH nodes.
273+
for _, idx := range []int{1, 2} {
274+
want := fmt.Sprintf("modified.email%d@example.com", idx)
275+
var e1, e2 string
276+
require.NoError(t, pgCluster.Node1Pool.QueryRow(ctx,
277+
fmt.Sprintf("SELECT email FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&e1))
278+
require.NoError(t, pgCluster.Node2Pool.QueryRow(ctx,
279+
fmt.Sprintf("SELECT email FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&e2))
280+
require.Equalf(t, want, e1, "index %d on n1 should be the newer (n2) value", idx)
281+
require.Equalf(t, want, e2, "index %d on n2 should be the newer (n2) value", idx)
282+
}
283+
}
284+
285+
// TestAdvancedRepairPlan_PickFreshestByAppColumn verifies pick_freshest with an
286+
// application-level timestamp column (subscription_date). Unlike commit_ts this
287+
// is ordinary row data, so it is immune to freezing and works even where
288+
// commit_ts would be NULL. index 1 is newer on n2, index 2 is newer on n1, so
289+
// the winning side differs per row.
290+
func TestAdvancedRepairPlan_PickFreshestByAppColumn(t *testing.T) {
291+
ctx := context.Background()
292+
qualifiedTableName := "public.customers"
293+
env := newSpockEnv()
294+
t.Cleanup(func() { resetSharedTable(t, "customers") })
295+
296+
// Clean slate on both nodes.
297+
for _, pool := range []*pgxpool.Pool{pgCluster.Node1Pool, pgCluster.Node2Pool} {
298+
env.withRepairMode(t, ctx, pool, func(conn *pgxpool.Conn) {
299+
_, err := conn.Exec(ctx, fmt.Sprintf("TRUNCATE TABLE %s CASCADE", qualifiedTableName))
300+
require.NoError(t, err)
301+
})
302+
}
303+
304+
// Same PKs on both nodes but differing first_name and subscription_date, so
305+
// each row is a row_mismatch resolvable by freshness of subscription_date.
306+
insert := func(pool *pgxpool.Pool, idx int, firstName, subDate string) {
307+
env.withRepairMode(t, ctx, pool, func(conn *pgxpool.Conn) {
308+
_, err := conn.Exec(ctx,
309+
fmt.Sprintf("INSERT INTO %s (index, customer_id, first_name, last_name, email, subscription_date) VALUES ($1,$2,$3,$4,$5,$6)", qualifiedTableName),
310+
idx, fmt.Sprintf("CUST-%d", idx), firstName, "Last", fmt.Sprintf("e%d@example.com", idx), subDate)
311+
require.NoError(t, err)
312+
})
313+
}
314+
insert(pgCluster.Node1Pool, 1, "N1", "2020-01-01 00:00:00") // index 1: n2 newer -> n2 wins
315+
insert(pgCluster.Node2Pool, 1, "N2", "2021-01-01 00:00:00")
316+
insert(pgCluster.Node1Pool, 2, "N1", "2023-01-01 00:00:00") // index 2: n1 newer -> n1 wins
317+
insert(pgCluster.Node2Pool, 2, "N2", "2022-01-01 00:00:00")
318+
319+
diffFile := runTableDiff(t, qualifiedTableName, []string{serviceN1, serviceN2})
320+
321+
plan := `
322+
version: 1
323+
tables:
324+
public.customers:
325+
rules:
326+
- name: latest_by_subscription_date
327+
diff_type: [row_mismatch]
328+
action:
329+
type: custom
330+
helpers:
331+
pick_freshest: { key: subscription_date, tie: n1 }
332+
`
333+
planPath := filepath.Join(t.TempDir(), "repair.yaml")
334+
require.NoError(t, os.WriteFile(planPath, []byte(plan), 0o644))
335+
336+
task := newTestTableRepairTask("", qualifiedTableName, diffFile)
337+
task.RepairPlanPath = planPath
338+
require.NoError(t, task.ValidateAndPrepare())
339+
require.NoError(t, task.Run(true))
340+
341+
assertNoTableDiff(t, qualifiedTableName)
342+
343+
// index 1 -> n2 (newer subscription_date) won; index 2 -> n1 won. The whole
344+
// winning row is applied, so first_name identifies the side that won.
345+
want := map[int]string{1: "N2", 2: "N1"}
346+
for idx, wantName := range want {
347+
var f1, f2 string
348+
require.NoError(t, pgCluster.Node1Pool.QueryRow(ctx,
349+
fmt.Sprintf("SELECT first_name FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&f1))
350+
require.NoError(t, pgCluster.Node2Pool.QueryRow(ctx,
351+
fmt.Sprintf("SELECT first_name FROM %s WHERE index = %d", qualifiedTableName, idx)).Scan(&f2))
352+
require.Equalf(t, wantName, f1, "index %d on n1 should be the fresher side", idx)
353+
require.Equalf(t, wantName, f2, "index %d on n2 should be the fresher side", idx)
354+
}
355+
}
356+
215357
func listStaleSkipLogs(t *testing.T) []string {
216358
t.Helper()
217359
paths, err := filepath.Glob(filepath.Join("reports", "*", "stale_repair_skips_*.json"))

0 commit comments

Comments
 (0)