From 370b593217911ea87d908be01ca1ee7846eafd5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89mile=20R=C3=A9?= Date: Tue, 19 May 2026 11:37:26 +0400 Subject: [PATCH] Replace xmax upsert trick with RETURNING full row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upsert methods now RETURNING all struct columns and scan the result back into the pointer receiver, keeping the caller in sync with the actual DB state (id, created_at, etc. from the existing row on conflict). Insert detection compares the saved original ID with the returned ID instead of relying on the PostgreSQL-internal xmax column. Signed-off-by: Émile Ré --- contrib/claude/coredata.md | 44 +++++++++++++++++++ pkg/cookiebanner/tracker_mapping_worker.go | 13 ++---- pkg/coredata/common_third_party.go | 33 ++++++++++++-- pkg/coredata/common_third_party_domain.go | 17 +++++-- pkg/coredata/common_tracker_pattern.go | 39 ++++++++-------- pkg/coredata/tracker_resource.go | 31 +++++++++++-- .../common_tracker_patterns.go | 2 +- 7 files changed, 139 insertions(+), 40 deletions(-) diff --git a/contrib/claude/coredata.md b/contrib/claude/coredata.md index 9ad1d0755..eaae866bc 100644 --- a/contrib/claude/coredata.md +++ b/contrib/claude/coredata.md @@ -145,6 +145,50 @@ if err != nil { } ``` +## Upsert with insert detection and receiver sync + +When an upsert needs to report whether a row was inserted or already existed, `RETURNING` all struct columns and scan the full row back into the receiver. Save the original ID before the query; on a fresh insert the returned ID matches, on a conflict/update the existing row's ID is returned. The receiver is a **pointer** so the caller always sees the actual DB state after the upsert. + +Do **not** use `RETURNING (xmax = 0) AS inserted` — `xmax` is a PostgreSQL internal system column and is fragile. + +```go +// Good — RETURNING full row, sync receiver, compare original ID +func (t *Thing) Upsert(ctx context.Context, conn pg.Tx) (inserted bool, err error) { + q := ` +INSERT INTO things (id, name, created_at, updated_at) +VALUES (@id, @name, @created_at, @updated_at) +ON CONFLICT (name) DO UPDATE +SET + name = EXCLUDED.name, + updated_at = EXCLUDED.updated_at +RETURNING + id, + name, + created_at, + updated_at +` + originalID := t.ID + args := pgx.StrictNamedArgs{...} + + rows, err := conn.Query(ctx, q, args) + if err != nil { + return false, fmt.Errorf("cannot upsert thing: %w", err) + } + defer rows.Close() + + row, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[Thing]) + if err != nil { + return false, fmt.Errorf("cannot collect upsert result: %w", err) + } + + *t = row + return originalID == t.ID, nil +} + +// Bad — xmax trick: relies on PostgreSQL internal column +RETURNING (xmax = 0) AS inserted +``` + ## Sentinel errors ```go diff --git a/pkg/cookiebanner/tracker_mapping_worker.go b/pkg/cookiebanner/tracker_mapping_worker.go index 28972c8bd..6bac20c2a 100644 --- a/pkg/cookiebanner/tracker_mapping_worker.go +++ b/pkg/cookiebanner/tracker_mapping_worker.go @@ -252,12 +252,10 @@ func (h *trackerMappingHandler) matchByDomain( UpdatedAt: now, } - actualID, _, err := commonPattern.Upsert(ctx, tx) - if err != nil { + if _, err := commonPattern.Upsert(ctx, tx); err != nil { return nil, nil, fmt.Errorf("cannot upsert common tracker pattern from domain match: %w", err) } - commonPattern.ID = actualID thirdPartyID, err := h.resolveThirdParty(ctx, tx, tp, &commonPattern) if err != nil { return nil, nil, fmt.Errorf("cannot resolve third party from domain match: %w", err) @@ -346,12 +344,10 @@ func (h *trackerMappingHandler) identifyWithAgent( UpdatedAt: now, } - actualID, _, err := commonPattern.Upsert(ctx, tx) - if err != nil { + if _, err := commonPattern.Upsert(ctx, tx); err != nil { return nil, nil, fmt.Errorf("cannot upsert common tracker pattern from agent: %w", err) } - commonPattern.ID = actualID thirdPartyID, err := h.resolveThirdParty(ctx, tx, tp, &commonPattern) if err != nil { return nil, nil, fmt.Errorf("cannot resolve third party from agent match: %w", err) @@ -447,12 +443,11 @@ func (h *trackerMappingHandler) createUnmatchedPattern( UpdatedAt: now, } - actualID, _, err := commonPattern.Upsert(ctx, tx) - if err != nil { + if _, err := commonPattern.Upsert(ctx, tx); err != nil { return nil, fmt.Errorf("cannot upsert unmatched common tracker pattern: %w", err) } - return &actualID, nil + return &commonPattern.ID, nil } func (h *trackerMappingHandler) resolveThirdParty( diff --git a/pkg/coredata/common_third_party.go b/pkg/coredata/common_third_party.go index 34cb75d03..c3685f8f9 100644 --- a/pkg/coredata/common_third_party.go +++ b/pkg/coredata/common_third_party.go @@ -310,7 +310,7 @@ INSERT INTO common_third_parties ( // Upsert inserts a row, or on slug conflict updates every column except // id and created_at. Returns true if a new row was inserted, false if an // existing row was updated. -func (t CommonThirdParty) Upsert( +func (t *CommonThirdParty) Upsert( ctx context.Context, conn pg.Tx, ) (inserted bool, err error) { @@ -379,9 +379,32 @@ SET security_page_url = EXCLUDED.security_page_url, trust_page_url = EXCLUDED.trust_page_url, updated_at = EXCLUDED.updated_at -RETURNING (xmax = 0) AS inserted +RETURNING + id, + name, + slug, + category, + headquarter_address, + legal_name, + website_url, + privacy_policy_url, + service_level_agreement_url, + service_software_agreement_url, + data_processing_agreement_url, + business_associate_agreement_url, + subprocessors_list_url, + certifications, + status_page_url, + terms_of_service_url, + security_page_url, + trust_page_url, + logo_file_id, + created_at, + updated_at ` + originalID := t.ID + args := pgx.StrictNamedArgs{ "id": t.ID, "name": t.Name, @@ -412,12 +435,14 @@ RETURNING (xmax = 0) AS inserted } defer rows.Close() - inserted, err = pgx.CollectExactlyOneRow(rows, pgx.RowTo[bool]) + row, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[CommonThirdParty]) if err != nil { return false, fmt.Errorf("cannot collect upsert result: %w", err) } - return inserted, nil + *t = row + + return originalID == t.ID, nil } func (t CommonThirdParty) Delete( diff --git a/pkg/coredata/common_third_party_domain.go b/pkg/coredata/common_third_party_domain.go index f028b0f26..6e4952784 100644 --- a/pkg/coredata/common_third_party_domain.go +++ b/pkg/coredata/common_third_party_domain.go @@ -118,7 +118,7 @@ INSERT INTO common_third_party_domains ( return nil } -func (d CommonThirdPartyDomain) Upsert( +func (d *CommonThirdPartyDomain) Upsert( ctx context.Context, conn pg.Tx, ) (inserted bool, err error) { @@ -139,9 +139,16 @@ INSERT INTO common_third_party_domains ( ON CONFLICT (common_third_party_id, domain) DO UPDATE SET updated_at = EXCLUDED.updated_at -RETURNING (xmax = 0) AS inserted +RETURNING + id, + common_third_party_id, + domain, + created_at, + updated_at ` + originalID := d.ID + args := pgx.StrictNamedArgs{ "id": d.ID, "common_third_party_id": d.CommonThirdPartyID, @@ -156,12 +163,14 @@ RETURNING (xmax = 0) AS inserted } defer rows.Close() - inserted, err = pgx.CollectExactlyOneRow(rows, pgx.RowTo[bool]) + row, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[CommonThirdPartyDomain]) if err != nil { return false, fmt.Errorf("cannot collect upsert result: %w", err) } - return inserted, nil + *d = row + + return originalID == d.ID, nil } func (d CommonThirdPartyDomain) Delete( diff --git a/pkg/coredata/common_tracker_pattern.go b/pkg/coredata/common_tracker_pattern.go index 27dcaa16c..757ae6068 100644 --- a/pkg/coredata/common_tracker_pattern.go +++ b/pkg/coredata/common_tracker_pattern.go @@ -189,10 +189,10 @@ INSERT INTO common_tracker_patterns ( return nil } -func (p CommonTrackerPattern) Upsert( +func (p *CommonTrackerPattern) Upsert( ctx context.Context, conn pg.Tx, -) (actualID gid.GID, inserted bool, err error) { +) (inserted bool, err error) { q := ` INSERT INTO common_tracker_patterns ( id, @@ -224,9 +224,21 @@ SET description = EXCLUDED.description, confidence = EXCLUDED.confidence, updated_at = EXCLUDED.updated_at -RETURNING id, (xmax = 0) AS inserted +RETURNING + id, + common_third_party_id, + tracker_type, + pattern, + match_type, + description, + max_age_seconds, + confidence, + created_at, + updated_at ` + originalID := p.ID + args := pgx.StrictNamedArgs{ "id": p.ID, "common_third_party_id": p.CommonThirdPartyID, @@ -242,27 +254,18 @@ RETURNING id, (xmax = 0) AS inserted rows, err := conn.Query(ctx, q, args) if err != nil { - return gid.GID{}, false, fmt.Errorf("cannot upsert common tracker pattern: %w", err) + return false, fmt.Errorf("cannot upsert common tracker pattern: %w", err) } defer rows.Close() - type upsertResult struct { - ID gid.GID - Inserted bool - } - - res, err := pgx.CollectExactlyOneRow( - rows, - func(row pgx.CollectableRow) (upsertResult, error) { - var r upsertResult - return r, row.Scan(&r.ID, &r.Inserted) - }, - ) + row, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[CommonTrackerPattern]) if err != nil { - return gid.GID{}, false, fmt.Errorf("cannot collect upsert result: %w", err) + return false, fmt.Errorf("cannot collect upsert result: %w", err) } - return res.ID, res.Inserted, nil + *p = row + + return originalID == p.ID, nil } func (p CommonTrackerPattern) Delete( diff --git a/pkg/coredata/tracker_resource.go b/pkg/coredata/tracker_resource.go index 3ed653d6a..3c52570b8 100644 --- a/pkg/coredata/tracker_resource.go +++ b/pkg/coredata/tracker_resource.go @@ -308,9 +308,24 @@ INSERT INTO tracker_resources ( ON CONFLICT (cookie_banner_id, resource_type, origin, path) DO UPDATE SET last_detected_at = GREATEST(tracker_resources.last_detected_at, EXCLUDED.last_detected_at), updated_at = EXCLUDED.updated_at -RETURNING (xmax = 0) AS inserted +RETURNING + id, + organization_id, + cookie_banner_id, + cookie_category_id, + resource_type, + origin, + path, + display_name, + description, + excluded, + last_detected_at, + created_at, + updated_at ` + originalID := tr.ID + args := pgx.StrictNamedArgs{ "id": tr.ID, "tenant_id": scope.GetTenantID(), @@ -328,12 +343,20 @@ RETURNING (xmax = 0) AS inserted "updated_at": tr.UpdatedAt, } - var inserted bool - if err := tx.QueryRow(ctx, q, args).Scan(&inserted); err != nil { + rows, err := tx.Query(ctx, q, args) + if err != nil { return false, fmt.Errorf("cannot upsert tracker resource: %w", err) } + defer rows.Close() - return inserted, nil + row, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[TrackerResource]) + if err != nil { + return false, fmt.Errorf("cannot collect upsert result: %w", err) + } + + *tr = row + + return originalID == tr.ID, nil } func (tr *TrackerResource) Update( diff --git a/pkg/proboctl/seed/common-tracker-patterns/common_tracker_patterns.go b/pkg/proboctl/seed/common-tracker-patterns/common_tracker_patterns.go index a91b8e2dd..23246bb92 100644 --- a/pkg/proboctl/seed/common-tracker-patterns/common_tracker_patterns.go +++ b/pkg/proboctl/seed/common-tracker-patterns/common_tracker_patterns.go @@ -145,7 +145,7 @@ func NewCmdCommonTrackerPatterns(f *cmdutil.Factory) *cobra.Command { UpdatedAt: now, } - _, wasInserted, err := pattern.Upsert(ctx, tx) + wasInserted, err := pattern.Upsert(ctx, tx) if err != nil { return fmt.Errorf("cannot upsert common tracker pattern %q: %w", p.Pattern, err) }