Robert Findley (Gerrit)

unread,

Nov 5, 2025, 3:53:51 PM (5 days ago) Nov 5

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Robert Findley has uploaded the change for review

Commit message

internal/postgres: avoid locking when upserting the module path

Consistent with other usage (for example, inserting module paths),
acquire the module path ID before entering a transaction, so that
inserting module paths doesn't lock the path table (a source of major
contention).

For golang/go#75959

Change-Id: I0edc8081c675fde94d54b266db9aa6abb21e9347

Change diff

diff --git a/internal/postgres/path.go b/internal/postgres/path.go
index 9b2edf3..cbd05ec 100644
--- a/internal/postgres/path.go
+++ b/internal/postgres/path.go
@@ -7,7 +7,6 @@
 import (
 	"context"
 	"database/sql"
-	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -73,40 +72,6 @@
 	return majPath, maj, nil
 }
 
-// upsertPath adds path into the paths table if it does not exist, and returns
-// its ID either way.
-// It assumes it is running inside a transaction.
-func upsertPath(ctx context.Context, tx *database.DB, path string) (id int, err error) {
-	// Doing the select first and then the insert led to uniqueness constraint
-	// violations even with fully serializable transactions; see
-	// https://www.postgresql.org/message-id/CAOqyxwL4E_JmUScYrnwd0_sOtm3bt4c7G%2B%2BUiD2PnmdGJFiqyQ%40mail.gmail.com.
-	// If the upsert is done first and then the select, then everything works
-	// fine.
-	defer derrors.WrapStack(&err, "upsertPath(%q)", path)
-
-	if _, err := tx.Exec(ctx, `LOCK TABLE paths IN EXCLUSIVE MODE`); err != nil {
-		return 0, err
-	}
-	err = tx.QueryRow(ctx,
-		`INSERT INTO paths (path) VALUES ($1) ON CONFLICT DO NOTHING RETURNING id`,
-		path).Scan(&id)
-	if err == sql.ErrNoRows {
-		err = tx.QueryRow(ctx,
-			`SELECT id FROM paths WHERE path = $1`,
-			path).Scan(&id)
-		if err == sql.ErrNoRows {
-			return 0, errors.New("got no rows; shouldn't happen")
-		}
-	}
-	if err != nil {
-		return 0, err
-	}
-	if id == 0 {
-		return 0, errors.New("zero ID")
-	}
-	return id, nil
-}
-
 // upsertPaths adds all the paths to the paths table if they aren't already
 // there, and returns their ID either way.
 // It assumes it is running inside a transaction.
diff --git a/internal/postgres/path_test.go b/internal/postgres/path_test.go
index 1b06d90..1c32fdb 100644
--- a/internal/postgres/path_test.go
+++ b/internal/postgres/path_test.go
@@ -7,7 +7,6 @@
 import (
 	"context"
 	"database/sql"
-	"errors"
 	"fmt"
 	"testing"
 
@@ -84,38 +83,6 @@
 	}
 }
 
-func TestUpsertPathConcurrently(t *testing.T) {
-	// Verify that we get no constraint violations or other errors when
-	// the same path is upserted multiple times concurrently.
-	t.Parallel()
-	testDB, release := acquire(t)
-	defer release()
-	ctx := context.Background()
-
-	const n = 10
-	errc := make(chan error, n)
-	for i := 0; i < n; i++ {
-		go func() {
-			errc <- testDB.db.Transact(ctx, sql.LevelRepeatableRead, func(tx *database.DB) error {
-				id, err := upsertPath(ctx, tx, "a/path")
-				if err != nil {
-					return err
-				}
-				if id == 0 {
-					return errors.New("zero id")
-				}
-				return nil
-			})
-		}()
-
-	}
-	for i := 0; i < n; i++ {
-		if err := <-errc; err != nil {
-			t.Fatal(err)
-		}
-	}
-}
-
 func TestUpsertPaths(t *testing.T) {
 	t.Parallel()
 	testDB, release := acquire(t)
diff --git a/internal/postgres/version.go b/internal/postgres/version.go
index 602c322..caadb87 100644
--- a/internal/postgres/version.go
+++ b/internal/postgres/version.go
@@ -467,6 +467,14 @@
 func (db *DB) UpdateLatestModuleVersions(ctx context.Context, vNew *internal.LatestModuleVersions) (_ *internal.LatestModuleVersions, err error) {
 	defer derrors.WrapStack(&err, "UpdateLatestModuleVersions(%q)", vNew.ModulePath)
 
+	pathToID, err := upsertPaths(ctx, db.db, []string{vNew.ModulePath})
+	if err != nil {
+		return nil, err
+	}
+	pathID, ok := pathToID[vNew.ModulePath]
+	if !ok {
+		return nil, fmt.Errorf("BUG: failed to acquire ID for path %q", vNew.ModulePath)
+	}
 	var vResult *internal.LatestModuleVersions
 	// We need RepeatableRead here because the INSERT...ON CONFLICT does a read.
 	err = db.db.Transact(ctx, sql.LevelRepeatableRead, func(tx *database.DB) error {
@@ -474,6 +482,9 @@
 		if err != nil {
 			return err
 		}
+		if vCur != nil && id != pathID {
+			return fmt.Errorf("inconsistent path ID: %d != %d", id, pathID)
+		}
 		// Is vNew the most recent information, or does the DB already have
 		// something more up to date?
 		update := vCur == nil || rawIsMoreRecent(vNew.RawVersion, vCur.RawVersion) ||
@@ -504,7 +515,7 @@
 			}
 		}
 		vResult = vNew
-		return upsertLatestModuleVersions(ctx, tx, vNew.ModulePath, id, vNew, 200)
+		return upsertLatestModuleVersions(ctx, tx, vNew.ModulePath, pathID, vNew, 200)
 	})
 	if err != nil {
 		return nil, err
@@ -542,12 +553,6 @@
 	defer derrors.WrapStack(&err, "upsertLatestModuleVersions(%s, %d)", modulePath, status)
 
 	// If the row doesn't exist, get a path ID for the module path.
-	if id == 0 {
-		id, err = upsertPath(ctx, tx, modulePath)
-		if err != nil {
-			return err
-		}
-	}
 	var (
 		raw, cooked, good string
 		goModBytes        = []byte{} // not nil, a zero-length slice

Change information

Files:

M internal/postgres/path.go
M internal/postgres/path_test.go
M internal/postgres/version.go

Change size: M

Delta: 3 files changed, 12 insertions(+), 75 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
kokoro-CI-Passes

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Robert Findley (Gerrit)

unread,

Nov 5, 2025, 3:59:35 PM (5 days ago) Nov 5

to goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Robert Findley voted Run-TryBot+1

Run-TryBot

+1

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review

Legacy-TryBots-Pass

No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
kokoro-CI-Passes

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

kokoro (Gerrit)

unread,

Nov 5, 2025, 4:09:22 PM (5 days ago) Nov 5

to Robert Findley, goph...@pubsubhelper.golang.org, Gopher Robot, Go LUCI, golang-co...@googlegroups.com

Attention needed from Robert Findley

kokoro voted kokoro-CI-1

Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/37dc3bc0-8f42-41ce-9cd4-036d6b7aede7

kokoro-CI

-1

Open in Gerrit

Related details

Attention is currently required from:

Robert Findley

Submit Requirements:

Code-Review
Legacy-TryBots-Pass

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass
kokoro-CI-Passes

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Ethan Lee (Gerrit)

unread,

Nov 5, 2025, 5:30:32 PM (5 days ago) Nov 5

to Robert Findley, goph...@pubsubhelper.golang.org, Go LUCI, kokoro, Gopher Robot, golang-co...@googlegroups.com

Attention needed from Robert Findley

Ethan Lee added 2 comments

File internal/postgres/path.go

Line 77, Patchset 1 (Latest):// It assumes it is running inside a transaction.

Ethan Lee . unresolved

Are we breaking this assumption by calling upsertPaths outside of a transaction now in UpdateLatestModuleVersions?

File internal/postgres/version.go

Line 555, Patchset 1 (Latest): // If the row doesn't exist, get a path ID for the module path.

Ethan Lee . unresolved

nit: remove this comment line

Open in Gerrit

Related details

Attention is currently required from:

Robert Findley

Submit Requirements:

Code-Review
Legacy-TryBots-Pass
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

kokoro-CI-Passes

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Jonathan Amsterdam (Gerrit)

unread,

Nov 6, 2025, 8:27:02 AM (5 days ago) Nov 6

to Robert Findley, goph...@pubsubhelper.golang.org, Ethan Lee, Go LUCI, kokoro, Gopher Robot, golang-co...@googlegroups.com

Attention needed from Robert Findley

Jonathan Amsterdam added 2 comments

File internal/postgres/path.go

Line 77, Patchset 1 (Latest):// It assumes it is running inside a transaction.

Ethan Lee . unresolved

Are we breaking this assumption by calling upsertPaths outside of a transaction now in UpdateLatestModuleVersions?

Jonathan Amsterdam

Yes, but maybe it doesn't matter. We may try to insert a few extra paths, but because of the on conflict do nothing clause, they shouldn't cause any trouble.

File internal/postgres/version.go

Line 470, Patchset 1 (Latest): pathToID, err := upsertPaths(ctx, db.db, []string{vNew.ModulePath})

Jonathan Amsterdam . unresolved

Why not just replace upsertPath on line 546? I suggest making the minimal change.

unsatisfied_requirement

satisfied_requirement

open

diffy

Robert Findley (Gerrit)

unread,

Nov 7, 2025, 1:29:49 PM (3 days ago) Nov 7

to goph...@pubsubhelper.golang.org, Jonathan Amsterdam, Ethan Lee, Go LUCI, kokoro, Gopher Robot, golang-co...@googlegroups.com

Robert Findley voted and added 1 comment

Votes added by Robert Findley

Hold

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Robert Findley . resolved

Holding in favor of CL 718720.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
Legacy-TryBots-Pass

No-Holds

No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
kokoro-CI-Passes

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[pkgsite] internal/postgres: avoid locking when upserting the module path

Robert Findley (Gerrit)

Robert Findley has uploaded the change for review

Commit message

Change diff

Change information

Related details

Robert Findley (Gerrit)

Robert Findley voted Run-TryBot+1

Related details

kokoro (Gerrit)

kokoro voted kokoro-CI-1

Related details

Ethan Lee (Gerrit)

Ethan Lee added 2 comments

Related details

Jonathan Amsterdam (Gerrit)

Jonathan Amsterdam added 2 comments

Robert Findley (Gerrit)

Robert Findley voted and added 1 comment

Votes added by Robert Findley

1 comment

Related details