internal/postgres: change UpsertModuleVersionStates to update
UpsertModuleVersionStates is changed to UpdateModuleVersionStates. There
should never be a situation where UpsertModuleVersionStates is called
and a row does not already exist for that module.
If that happens, an error is now returned.
For golang/go#46985
Fixes golang/go#39628
Change-Id: I357396cee6eb743513ae249609f76f4cd4c19e9b
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/341860
Trust: Julie Qiu <julie@golang.org>
Run-TryBot: Julie Qiu <julie@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
diff --git a/internal/postgres/insert_module_test.go b/internal/postgres/insert_module_test.go
index 00e178d..3c44d87 100644
--- a/internal/postgres/insert_module_test.go
+++ b/internal/postgres/insert_module_test.go
@@ -458,7 +458,7 @@
okVersion = "v1.0.0"
)
- mvs := &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: "example.com/Mod",
Version: altVersion,
AppVersion: "appVersion",
@@ -467,8 +467,17 @@
GoModPath: "example.com/mod",
FetchErr: derrors.AlternativeModule,
}
+ if err := testDB.InsertIndexVersions(ctx, []*internal.IndexVersion{
+ {
+ Path: mvs.ModulePath,
+ Version: mvs.Version,
+ Timestamp: mvs.Timestamp,
+ },
+ }); err != nil {
+ t.Fatal(err)
+ }
addLatest(ctx, t, testDB, mvs.ModulePath, altVersion, "")
- if err := testDB.UpsertModuleVersionState(ctx, mvs); err != nil {
+ if err := testDB.UpdateModuleVersionState(ctx, mvs); err != nil {
t.Fatal(err)
}
m := sample.Module(mvs.ModulePath, okVersion, "p")
@@ -592,17 +601,27 @@
{modulePathA, "v1.4.0", 200, false}, // new latest version is OK
{modulePathA, "v1.5.0", altModuleStatus, true}, // "I can do this all day." --Captain America
} {
- addLatest(ctx, t, testDB, test.modulePath, test.version, "")
- if err := testDB.UpsertModuleVersionState(ctx, &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: test.modulePath,
Version: test.version,
AppVersion: "appVersion",
Timestamp: time.Now(),
Status: test.status,
HasGoMod: true,
+ }
+ if err := testDB.InsertIndexVersions(ctx, []*internal.IndexVersion{
+ {
+ Path: mvs.ModulePath,
+ Version: mvs.Version,
+ Timestamp: mvs.Timestamp,
+ },
}); err != nil {
t.Fatal(err)
}
+ addLatest(ctx, t, testDB, test.modulePath, test.version, "")
+ if err := testDB.UpdateModuleVersionState(ctx, mvs); err != nil {
+ t.Fatal(err)
+ }
got, err := isAlternativeModulePath(ctx, testDB.db, modulePathA)
if err != nil {
@@ -630,7 +649,17 @@
addLatest(ctx, t, testDB, modulePath, version, modfile)
}
- if err := testDB.UpsertModuleVersionState(ctx, &ModuleVersionStateForUpsert{
+ if err := testDB.InsertIndexVersions(ctx,
+ []*internal.IndexVersion{
+ {
+ Path: modulePath,
+ Version: version,
+ Timestamp: time.Now(),
+ },
+ }); err != nil {
+ t.Fatal(err)
+ }
+ if err := testDB.UpdateModuleVersionState(ctx, &ModuleVersionStateForUpdate{
ModulePath: modulePath,
Version: version,
AppVersion: "app",
diff --git a/internal/postgres/requeue_test.go b/internal/postgres/requeue_test.go
index b2c7726..6198b65 100644
--- a/internal/postgres/requeue_test.go
+++ b/internal/postgres/requeue_test.go
@@ -159,7 +159,7 @@
checkNextToRequeue(want, len(mods))
for _, m := range mods {
- mvs := &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: m.modulePath,
Version: m.version,
AppVersion: "2020-04-29t14",
@@ -168,7 +168,7 @@
GoModPath: m.modulePath,
FetchErr: derrors.FromStatus(m.status, "test string"),
}
- if err := upsertModuleVersionState(ctx, testDB.db, &m.numPackages, mvs); err != nil {
+ if err := updateModuleVersionState(ctx, testDB.db, &m.numPackages, mvs); err != nil {
t.Fatal(err)
}
}
diff --git a/internal/postgres/search_test.go b/internal/postgres/search_test.go
index 77261b7..96b621c 100644
--- a/internal/postgres/search_test.go
+++ b/internal/postgres/search_test.go
@@ -1185,14 +1185,23 @@
// We add that information to module_version_states.
alternativeModulePath := strings.ToLower(canonicalModule.ModulePath)
alternativeStatus := derrors.ToStatus(derrors.AlternativeModule)
- mvs := &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: alternativeModulePath,
Version: "v1.2.0",
Timestamp: time.Now(),
Status: alternativeStatus,
GoModPath: canonicalModule.ModulePath,
}
- if err := testDB.UpsertModuleVersionState(ctx, mvs); err != nil {
+ if err := testDB.InsertIndexVersions(ctx, []*internal.IndexVersion{
+ {
+ Path: mvs.ModulePath,
+ Version: mvs.Version,
+ Timestamp: mvs.Timestamp,
+ },
+ }); err != nil {
+ t.Fatal(err)
+ }
+ if err := testDB.UpdateModuleVersionState(ctx, mvs); err != nil {
t.Fatal(err)
}
diff --git a/internal/postgres/versionstate.go b/internal/postgres/versionstate.go
index 85f8d54..2ccaa02 100644
--- a/internal/postgres/versionstate.go
+++ b/internal/postgres/versionstate.go
@@ -105,7 +105,7 @@
})
}
-type ModuleVersionStateForUpsert struct {
+type ModuleVersionStateForUpdate struct {
ModulePath string
Version string
AppVersion string
@@ -117,9 +117,9 @@
PackageVersionStates []*internal.PackageVersionState
}
-// UpsertModuleVersionState inserts or updates the module_version_state table with
+// UpdateModuleVersionState inserts or updates the module_version_state table with
// the results of a fetch operation for a given module version.
-func (db *DB) UpsertModuleVersionState(ctx context.Context, mvs *ModuleVersionStateForUpsert) (err error) {
+func (db *DB) UpdateModuleVersionState(ctx context.Context, mvs *ModuleVersionStateForUpdate) (err error) {
defer derrors.WrapStack(&err, "UpsertModuleVersionState(ctx, %s@%s)", mvs.ModulePath, mvs.Version)
ctx, span := trace.StartSpan(ctx, "UpsertModuleVersionState")
defer span.End()
@@ -131,9 +131,8 @@
n := len(mvs.PackageVersionStates)
numPackages = &n
}
-
return db.db.Transact(ctx, sql.LevelDefault, func(tx *database.DB) error {
- if err := upsertModuleVersionState(ctx, tx, numPackages, mvs); err != nil {
+ if err := updateModuleVersionState(ctx, tx, numPackages, mvs); err != nil {
return err
}
// Sync modules.status if the module exists in the modules table.
@@ -147,7 +146,7 @@
})
}
-func upsertModuleVersionState(ctx context.Context, db *database.DB, numPackages *int, mvs *ModuleVersionStateForUpsert) (err error) {
+func updateModuleVersionState(ctx context.Context, db *database.DB, numPackages *int, mvs *ModuleVersionStateForUpdate) (err error) {
defer derrors.WrapStack(&err, "upsertModuleVersionState(%q, %q, ...)", mvs.ModulePath, mvs.Version)
ctx, span := trace.StartSpan(ctx, "upsertModuleVersionState")
defer span.End()
@@ -158,42 +157,35 @@
}
affected, err := db.Exec(ctx, `
- INSERT INTO module_version_states AS mvs (
- module_path,
- version,
- sort_version,
- app_version,
- index_timestamp,
- status,
- has_go_mod,
- go_mod_path,
- error,
- num_packages,
- incompatible)
- VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
- ON CONFLICT (module_path, version)
- DO UPDATE
- SET
- app_version=excluded.app_version,
- status=excluded.status,
- has_go_mod=excluded.has_go_mod,
- go_mod_path=excluded.go_mod_path,
- error=excluded.error,
- num_packages=excluded.num_packages,
- try_count=mvs.try_count+1,
- last_processed_at=CURRENT_TIMESTAMP,
- -- back off exponentially until 1 hour, then at constant 1-hour intervals
- next_processed_after=CASE
- WHEN mvs.last_processed_at IS NULL THEN
- CURRENT_TIMESTAMP + INTERVAL '1 minute'
- WHEN 2*(mvs.next_processed_after - mvs.last_processed_at) < INTERVAL '1 hour' THEN
- CURRENT_TIMESTAMP + 2*(mvs.next_processed_after - mvs.last_processed_at)
- ELSE
- CURRENT_TIMESTAMP + INTERVAL '1 hour'
- END;`,
- mvs.ModulePath, mvs.Version, version.ForSorting(mvs.Version),
- mvs.AppVersion, mvs.Timestamp, mvs.Status, mvs.HasGoMod, mvs.GoModPath, sqlErrorMsg, numPackages,
- version.IsIncompatible(mvs.Version))
+ UPDATE module_version_states
+ SET app_version=$1,
+ status=$2,
+ has_go_mod=$3,
+ go_mod_path=$4,
+ error=$5,
+ num_packages=$6,
+ try_count=try_count+1,
+ last_processed_at=CURRENT_TIMESTAMP,
+ -- back off exponentially until 1 hour, then at constant 1-hour intervals
+ next_processed_after=CASE
+ WHEN last_processed_at IS NULL THEN
+ CURRENT_TIMESTAMP + INTERVAL '1 minute'
+ WHEN 2*(next_processed_after - last_processed_at) < INTERVAL '1 hour' THEN
+ CURRENT_TIMESTAMP + 2*(next_processed_after - last_processed_at)
+ ELSE
+ CURRENT_TIMESTAMP + INTERVAL '1 hour'
+ END
+ WHERE
+ module_path=$7
+ AND version=$8`,
+ mvs.AppVersion,
+ mvs.Status,
+ mvs.HasGoMod,
+ mvs.GoModPath,
+ sqlErrorMsg,
+ numPackages,
+ mvs.ModulePath,
+ mvs.Version)
if err != nil {
return err
}
diff --git a/internal/postgres/versionstate_test.go b/internal/postgres/versionstate_test.go
index cfd0f5f..953097c 100644
--- a/internal/postgres/versionstate_test.go
+++ b/internal/postgres/versionstate_test.go
@@ -137,7 +137,7 @@
Status: 500,
}
)
- mvs := &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: fooVersion.Path,
Version: fooVersion.Version,
Timestamp: fooVersion.Timestamp,
@@ -146,7 +146,7 @@
FetchErr: fetchErr,
PackageVersionStates: []*internal.PackageVersionState{pkgVersionState},
}
- must(t, testDB.UpsertModuleVersionState(ctx, mvs))
+ must(t, testDB.UpdateModuleVersionState(ctx, mvs))
errString := fetchErr.Error()
numPackages := 1
wantFooState := &internal.ModuleVersionState{
@@ -274,7 +274,7 @@
MustInsertModule(ctx, t, testDB, m)
}
- mvsu := &ModuleVersionStateForUpsert{
+ mvsu := &ModuleVersionStateForUpdate{
ModulePath: m.ModulePath,
Version: m.Version,
AppVersion: appVersion,
@@ -282,7 +282,16 @@
Status: test.status,
HasGoMod: true,
}
- err := testDB.UpsertModuleVersionState(ctx, mvsu)
+ if err := testDB.InsertIndexVersions(ctx, []*internal.IndexVersion{
+ {
+ Path: mvsu.ModulePath,
+ Version: mvsu.Version,
+ Timestamp: mvsu.Timestamp,
+ },
+ }); err != nil {
+ t.Fatal(err)
+ }
+ err := testDB.UpdateModuleVersionState(ctx, mvsu)
if test.wantUpsertMVSError != (err != nil) {
t.Fatalf("db.UpsertModuleVersionState(): %v, want error: %t", err, test.wantUpsertMVSError)
}
@@ -328,13 +337,17 @@
testDB, release := acquire(t)
defer release()
- mvs := &ModuleVersionStateForUpsert{
+ mvs := &ModuleVersionStateForUpdate{
ModulePath: "m.com",
Version: "v1.2.3",
Timestamp: time.Now(),
Status: 200,
}
- must(t, testDB.UpsertModuleVersionState(ctx, mvs))
+ must(t, testDB.InsertIndexVersions(
+ ctx,
+ []*internal.IndexVersion{{Path: mvs.ModulePath, Version: mvs.Version, Timestamp: time.Now()}},
+ ))
+ must(t, testDB.UpdateModuleVersionState(ctx, mvs))
wantStatus := 999
wantError := "Error"
must(t, testDB.UpdateModuleVersionStatus(ctx, mvs.ModulePath, mvs.Version, wantStatus, wantError))
diff --git a/internal/worker/fetch.go b/internal/worker/fetch.go
index a2fb408..4f0c26c 100644
--- a/internal/worker/fetch.go
+++ b/internal/worker/fetch.go
@@ -198,8 +198,6 @@
ft.Error = err
ft.Status = http.StatusInternalServerError
}
- // Do not return an error here, because we want to insert into
- // module_version_states below.
}
// Regardless of what the status code is, insert the result into
// version_map, so that a response can be returned for frontend_fetch.
@@ -220,6 +218,15 @@
// modules that have been published to index.golang.org.
return ft.Status, ft.ResolvedVersion, ft.Error
}
+ // Return an error here if a row does not exist in module_version_states.
+ // This can happen if the source is frontend fetch, since we don't insert
+ // rows to avoid cluttering module_version_states.
+ if _, err := f.DB.GetModuleVersionState(ctx, modulePath, ft.ResolvedVersion); err != nil {
+ if errors.Is(err, derrors.NotFound) {
+ return ft.Status, "", ft.Error
+ }
+ return http.StatusInternalServerError, "", err
+ }
// Make sure the latest version of the module is the one in search_documents
// and imports_unique.
@@ -237,10 +244,8 @@
// module@version. This must happen last, because if it succeeds with a
// code < 500 but a later action fails, we will never retry the later
// action.
- // TODO(golang/go#39628): Split UpsertModuleVersionState into
- // InsertModuleVersionState and UpdateModuleVersionState.
startUpdate := time.Now()
- mvs := &postgres.ModuleVersionStateForUpsert{
+ mvs := &postgres.ModuleVersionStateForUpdate{
ModulePath: ft.ModulePath,
Version: ft.ResolvedVersion,
AppVersion: appVersionLabel,
@@ -250,13 +255,13 @@
FetchErr: ft.Error,
PackageVersionStates: ft.PackageVersionStates,
}
- err = f.DB.UpsertModuleVersionState(ctx, mvs)
- ft.timings["db.UpsertModuleVersionState"] = time.Since(startUpdate)
+ err = f.DB.UpdateModuleVersionState(ctx, mvs)
+ ft.timings["db.UpdateModuleVersionState"] = time.Since(startUpdate)
if err != nil {
log.Error(ctx, err)
if ft.Error != nil {
ft.Status = http.StatusInternalServerError
- ft.Error = fmt.Errorf("db.UpsertModuleVersionState: %v, original error: %v", err, ft.Error)
+ ft.Error = fmt.Errorf("db.UpdateModuleVersionState: %v, original error: %v", err, ft.Error)
}
logTaskResult(ctx, ft, "Failed to update module version state")
return http.StatusInternalServerError, ft.ResolvedVersion, ft.Error
diff --git a/internal/worker/fetcherror_test.go b/internal/worker/fetcherror_test.go
index e130112..9e3ca87 100644
--- a/internal/worker/fetcherror_test.go
+++ b/internal/worker/fetcherror_test.go
@@ -16,7 +16,6 @@
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
- "golang.org/x/mod/semver"
"golang.org/x/pkgsite/internal"
"golang.org/x/pkgsite/internal/derrors"
"golang.org/x/pkgsite/internal/fetch"
@@ -378,11 +377,6 @@
t.Fatalf("GetModuleInfo: got %v, want Is(NotFound)", err)
}
}
- if semver.IsValid(version) {
- if _, err := testDB.GetModuleVersionState(ctx, modulePath, version); err != nil {
- t.Fatal(err)
- }
- }
vm, err := testDB.GetVersionMap(ctx, modulePath, version)
if err != nil {
t.Fatal(err)
