Normalize feedback primary key to (id) for upgraded installs

[iplay88keys]

Summary

Adds migration 000004_feedback_single_pk that converts the feedback table's primary key from the legacy composite (id, user_id) to single (id), so fresh installs and upgraded installs end at the same schema.

Background

Feedback's PK shape diverged in OSS PR #1306 (commit cbc8d57e, first released in v0.7.15):

Version	Feedback struct	GORM AutoMigrate result
≤ v0.7.14	gorm.Model + UserID gorm:"primaryKey;not null"	composite PK (id, user_id)
≥ v0.7.15 (after #1306)	gorm.Model + UserID gorm:"not null;index"	single-column PK (id)

GORM AutoMigrate is additive-only, so it cannot drop a primary key. Any deployment that was ever migrated under v0.7.14 or earlier kept the composite PK forever, even after the struct was fixed.

The 000001_initial baseline matches the fixed struct's output (id BIGSERIAL PRIMARY KEY), which is correct for fresh installs but no-ops on upgraded installs (the CREATE TABLE IF NOT EXISTS is skipped). Result: upgraded installs still carry the composite PK after running the new migration tooling.

Risks

Duplicate id rows would block the migration. The old composite (id, user_id) only enforces uniqueness of the pair, so it permits two rows with id=1 if their user_id differs. The new single-column (id) requires id itself to be unique. This isn't purely hypothetical for long-lived deployments: throughout the GORM era (and still today, until sqlc closed the gap in v0.9.x) the feedback HTTP handler unmarshalled the entire database.Feedback struct directly from the request body, including the embedded gorm.Model.ID field — so a client sending {"id": 99999, "user_id": "...", ...} could insert with an explicit id. The pre-fix save() helper used ON CONFLICT ... DO UPDATE on the composite PK, and StoreFeedback after PR #1306 used db.Create(); both paths passed a non-zero ID through to Postgres unchanged. The composite PK then allowed the same explicit id to appear under multiple user_id values. The sqlc-era query is hardcoded to omit id from the column list, which is why this gap doesn't grow further on v0.9.x — but pre-v0.9.x history is what's already in the database.

If any such rows exist, ADD CONSTRAINT feedback_pkey PRIMARY KEY (id) will fail with a unique-violation and the migration aborts atomically. This surfaces at startup before traffic, which is the correct failure mode, but operators should run the following against any long-lived database before deploying and dedupe if it returns rows:

SELECT id, COUNT(*) FROM feedback GROUP BY id HAVING COUNT(*) > 1;

Brief table lock during migration. DROP CONSTRAINT and ADD CONSTRAINT PRIMARY KEY each take an AccessExclusiveLock on feedback, blocking all reads and writes for the duration. Adding the new PK requires a full table scan to validate uniqueness and not-null. For typical feedback data sizes this completes in milliseconds; an unusually large table could take seconds. The whole migration runs in a single transaction so the state is always consistent.

No foreign keys reference feedback.id, so there are no cascades or referential-integrity ripples to manage.

[Copilot]

Pull request overview

Adds a corrective core migration to make upgraded databases converge on the same feedback schema that fresh installs already get: PRIMARY KEY (id) instead of the legacy composite key. This fits the migration subsystem’s goal of making schema state deterministic across install/upgrade paths.

Changes:

• Add 000004_feedback_single_pk.up.sql to replace the legacy feedback composite primary key with a single-column primary key on id.
• Add a documented no-op down migration for this convergence step.
• Document the migration’s assumptions and its relationship to existing feedback queries/indexes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
go/core/pkg/migrations/core/000004_feedback_single_pk.up.sql	Adds the schema change that normalizes feedback PKs to (id).
go/core/pkg/migrations/core/000004_feedback_single_pk.down.sql	Adds a no-op rollback file with rationale for leaving the normalized PK in place.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[iplay88keys]

This would result in a fresh install env applying this migration on top having the wrong primary key. I think it's fine to leave as a result.

[iplay88keys]

@EItanya I don't think this is necessary based on the limited usefulness of the feedback feature and the unlikelihood that this will have duplicate ids. The only reason I'm considering it is to have a clearer message for manual fixes in the unlikely event that there is an issue with this upgrade. Another option is we we could just raise it in the release notes if we want, but as before, I think this is unlikely to be an issue.