diff --git a/internal/meta/README.md b/internal/meta/README.md index 15d50a57..6a3bdb0c 100644 --- a/internal/meta/README.md +++ b/internal/meta/README.md @@ -25,6 +25,7 @@ The metadata store: orgs, public and signing keys, repos, webhooks, git credenti - An unknown `create_state` maps to legacy, which is never treated as complete [meta.go:knownCreateState] - Repo inserts map an `(org, url)` unique violation to `ErrAlreadyExists`, including soft-deleted rows [pg.go:pgInsertRepo] [sqlite.go:insertRepo] - `ListRepos` asserts limit in [1, 100]; `decodeCursor` maps any malformed cursor to `ErrInvalidCursor` [pg.go:ListRepos] [cursor.go:decodeCursor] +- Postgres `ListRepos` lowers the `q` and `prefix` pattern once per query in an InitPlan, not once per row, and gives the answers of `url ILIKE` [pg.go:pgListReposQuery] - `SoftDeleteRepo` returns `ErrAlreadyDeleted` for a deleted row and `ErrNotFound` for an absent one [pg.go:SoftDeleteRepo] - Credentials and upstreams attach only to live repos; a second one per repo is `ErrAlreadyExists` [pg_eventing.go:createGitCredential] - `TryBeginSync`/`TryBeginPush` refuse unexpired claims; finish and restore match only the caller's holder; a miss returns `ErrClaimLost` when a different holder owns the lane, else `ErrNotFound` (no run in flight, or upstream detached) [pg_eventing.go:FinishSync] [pg_eventing.go:classifyGuardMiss] [sqlite_eventing.go:FinishSync] diff --git a/internal/meta/pg.go b/internal/meta/pg.go index e5bb24de..d01ddf11 100644 --- a/internal/meta/pg.go +++ b/internal/meta/pg.go @@ -637,24 +637,10 @@ func (s *PG) ListRepos(ctx context.Context, org string, params ListParams) (List if err != nil { return ListResult{}, err } - - sql := `SELECT ` + repoCols + ` FROM repos WHERE org_id = $1 AND deleted_at IS NULL` - args := []any{orgID} - if pattern := listURLPattern(params); pattern != "" { - args = append(args, pattern) - sql += fmt.Sprintf(` AND url ILIKE $%d`, len(args)) - } - if params.Cursor != "" { - createdAt, repoID, err := decodeCursor(params.Cursor) - if err != nil { - return ListResult{}, err - } - args = append(args, createdAt, repoID) - sql += fmt.Sprintf(` AND (created_at, repo_id) < ($%d, $%d)`, len(args)-1, len(args)) + sql, args, err := pgListReposQuery(orgID, params) + if err != nil { + return ListResult{}, err } - // Fetch one extra row to compute HasMore without a second query. - args = append(args, params.Limit+1) - sql += fmt.Sprintf(` ORDER BY created_at DESC, repo_id DESC LIMIT $%d`, len(args)) rows, err := s.pool.Query(ctx, sql, args...) if err != nil { @@ -683,3 +669,34 @@ func (s *PG) ListRepos(ctx context.Context, org string, params ListParams) (List } return res, nil } + +// pgListReposQuery returns the ListRepos SQL for one page and its +// arguments. +// +// The url filter gives the answers of `url ILIKE p`, which matches +// lower(url) against lower(p) as LIKE does (like.c, Generic_Text_IC_like). +// ILIKE itself lowers p again for every row it tests in a multibyte +// database, so a long q or prefix cost rows × its length. The scalar +// subquery is an InitPlan: it lowers p once per query, in custom and +// generic plans alike. +func pgListReposQuery(orgID int64, params ListParams) (string, []any, error) { + assertf(orgID > 0, "pgListReposQuery: non-positive org id %d", orgID) + sql := `SELECT ` + repoCols + ` FROM repos WHERE org_id = $1 AND deleted_at IS NULL` + args := []any{orgID} + if pattern := listURLPattern(params); pattern != "" { + args = append(args, pattern) + sql += fmt.Sprintf(` AND lower(url) LIKE (SELECT lower($%d::text))`, len(args)) + } + if params.Cursor != "" { + createdAt, repoID, err := decodeCursor(params.Cursor) + if err != nil { + return "", nil, err + } + args = append(args, createdAt, repoID) + sql += fmt.Sprintf(` AND (created_at, repo_id) < ($%d, $%d)`, len(args)-1, len(args)) + } + // Fetch one extra row to compute HasMore without a second query. + args = append(args, params.Limit+1) + sql += fmt.Sprintf(` ORDER BY created_at DESC, repo_id DESC LIMIT $%d`, len(args)) + return sql, args, nil +} diff --git a/internal/meta/pg_list_repos_test.go b/internal/meta/pg_list_repos_test.go new file mode 100644 index 00000000..3d3c63c3 --- /dev/null +++ b/internal/meta/pg_list_repos_test.go @@ -0,0 +1,147 @@ +package meta + +import ( + "fmt" + "regexp" + "slices" + "strings" + "testing" + "time" + "unicode" + + "github.com/jackc/pgx/v5" +) + +// TestPGListReposLowersFilterOnce pins that the ListRepos url filter +// lowers q and prefix once per query, not once per row. `url ILIKE p` +// lowered p for every row it tested, so a 200 KiB q over 3,100 repos held +// a PostgreSQL backend for seconds. The generic plan is the one that +// pgx's statement cache can run: its per-row filter must not read p, an +// InitPlan must lower it once, and the filter must be LIKE (`~~`) on +// lower(url). ILIKE (`~~*`) lowers its pattern again for each row, even +// when an InitPlan computes that pattern. +func TestPGListReposLowersFilterOnce(t *testing.T) { + s, ok := pgBackend(t).(*PG) + if !ok { + t.Fatal("pgBackend is not *PG") + } + ctx := t.Context() + params := ListParams{Q: "sdk", Prefix: "pierre/", Cursor: encodeCursor(time.Unix(1, 0), "r"), Limit: 10} + sql, args, err := pgListReposQuery(1, params) + if err != nil { + t.Fatal(err) + } + i := slices.Index(args, any(listURLPattern(params))) + if i < 0 { + t.Fatalf("no argument is the pattern %q: %v", listURLPattern(params), args) + } + pattern := regexp.MustCompile(fmt.Sprintf(`\$%d\b`, i+1)) + + conn, err := s.pool.Acquire(ctx) + if err != nil { + t.Fatal(err) + } + defer conn.Release() + // GENERIC_PLAN (PostgreSQL 16 and later) plans the $n placeholders + // without values. The simple protocol sends them as written. + results, err := conn.Conn().PgConn().Exec(ctx, "EXPLAIN (GENERIC_PLAN) "+sql).ReadAll() + if err != nil { + t.Fatalf("explain: %v", err) + } + var plan strings.Builder + for _, r := range results { + for _, row := range r.Rows { + plan.Write(row[0]) + plan.WriteByte('\n') + } + } + if !strings.Contains(plan.String(), "InitPlan") || pattern.MatchString(plan.String()) { + t.Fatalf("want the pattern ($%d) lowered once in an InitPlan and never read per row:\n%s", i+1, plan.String()) + } + // PostgreSQL 16 prints the InitPlan's result as `$0`, and 17 and later + // as `(InitPlan 1).col1`, so match only the operator and its left side. + if !strings.Contains(plan.String(), "lower(url) ~~ ") || strings.Contains(plan.String(), "~~*") { + t.Fatalf("want the filter `lower(url) ~~ `, and no ILIKE (~~*), which lowers its pattern per row:\n%s", plan.String()) + } +} + +// TestPGListReposMatchesILIKE pins the ListRepos url filter to the +// answers of `url ILIKE p`, the filter it replaced, for urls and filters +// whose case folding is not ASCII-only. +func TestPGListReposMatchesILIKE(t *testing.T) { + s, ok := pgBackend(t).(*PG) + if !ok { + t.Fatal("pgBackend is not *PG") + } + ctx := t.Context() + org := seedOrg(t, s) + urls := []string{ + "acme/Straße", "acme/STRASSE", "acme/ẞig", "acme/İstanbul", "acme/istanbul", + "acme/IRMAK", "acme/ırmak", "acme/ΣΊΣΥΦΟΣ", "acme/σίσυφος", "acme/Džemal", + "acme/dž", "acme/ffi", "acme/\u212aelvin", "acme/kelvin", "acme/\u212bngstrom", + "acme/Ångström", "acme/e\u0301clair", "acme/Éclair", "acme/50%Off", "acme/A_B", + `acme/Back\Slash`, "ACME/upper", "Ünïcode/Prefix", + } + for _, u := range urls { + if _, err := s.CreateRepo(ctx, org, u, "main"); err != nil { + t.Fatalf("create %s: %v", u, err) + } + } + orgID, err := s.lookupOrgID(ctx, org) + if err != nil { + t.Fatal(err) + } + + // Each filter is a rune of a url, a start of a url, or both, in every + // case that Go knows for it. + seen := map[ListParams]bool{} + var filters []ListParams + add := func(prefix, q string) { + for _, p := range []string{prefix, strings.ToUpper(prefix), strings.ToLower(prefix)} { + for _, q := range []string{q, strings.ToUpper(q), strings.ToLower(q)} { + f := ListParams{Prefix: strings.TrimSpace(p), Q: strings.TrimSpace(q), Limit: 100} + if !seen[f] && (f.Prefix != "" || f.Q != "") { + seen[f] = true + filters = append(filters, f) + } + } + } + } + for _, u := range urls { + runes := []rune(u) + for k, r := range runes { + for f := unicode.SimpleFold(r); ; f = unicode.SimpleFold(f) { + add("", string(f)) + add("acme/", string(f)) + if f == r { + break + } + } + add(string(runes[:k+1]), "") + } + } + + for _, f := range filters { + res, err := s.ListRepos(ctx, org, f) + if err != nil { + t.Fatalf("ListRepos(prefix=%q, q=%q): %v", f.Prefix, f.Q, err) + } + got := make([]string, 0, len(res.Repos)) + for _, r := range res.Repos { + got = append(got, r.URL) + } + rows, err := s.pool.Query(ctx, `SELECT url FROM repos WHERE org_id = $1 AND deleted_at IS NULL + AND url ILIKE $2 ORDER BY created_at DESC, repo_id DESC`, orgID, listURLPattern(f)) + if err != nil { + t.Fatal(err) + } + want, err := pgx.CollectRows(rows, pgx.RowTo[string]) + if err != nil { + t.Fatal(err) + } + if res.HasMore || !slices.Equal(got, want) { + t.Errorf("prefix=%q q=%q: got %q (HasMore=%v), ILIKE gives %q", f.Prefix, f.Q, got, res.HasMore, want) + } + } + t.Logf("%d filters over %d urls", len(filters), len(urls)) +} diff --git a/internal/meta/sqlite.go b/internal/meta/sqlite.go index e85780fc..2862c8d3 100644 --- a/internal/meta/sqlite.go +++ b/internal/meta/sqlite.go @@ -724,11 +724,11 @@ func (s *SQLite) RemoveSigningKey(ctx context.Context, org string, id int64) err // ListRepos pages live repos for the org, keyset-ordered by // (created_at DESC, repo_id DESC). // -// PG's `url ILIKE ?` becomes a start and a substring test of lower(url), -// not LIKE: SQLite refuses a LIKE pattern over 50,000 bytes +// PG's `lower(url) LIKE lower(p)` becomes a start and a substring test of +// lower(url), not LIKE: SQLite refuses a LIKE pattern over 50,000 bytes // (SQLITE_MAX_LIKE_PATTERN_LENGTH), and q and prefix can be longer. SQLite's // lower() only folds ASCII, so non-ASCII urls match case-sensitively here -// while PG ILIKE folds per locale — an accepted divergence for local mode. +// while PG lower() folds per locale — an accepted divergence for local mode. func (s *SQLite) ListRepos(ctx context.Context, org string, params ListParams) (ListResult, error) { // Limit is validated/clamped by the HTTP handler per the Store contract // (default 20, max 100); a value outside [1,100] is a caller bug.