chore: store database code as code [DET-9180] #9302

NicholasBlaskey · 2024-05-03T14:55:18Z

Ticket

Description

Store database code as code. See the added readme for details.

Test Plan

CI passes

NOTE TO REVIEWERS

To review the code, we can test that the code is the same schema with pg_dump

on main on a fresh database

pg_dump -U postgres -h 127.0.0.1 --schema-only determined > ../main.sql

on this branch on a fresh database

pg_dump -U postgres -h 127.0.0.1 --schema-only determined > ../change.sql

check the diff (or go to this link)
https://www.diffchecker.com/8X1MK9aw/

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-05-03T14:55:36Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`b075098`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66450e495706ea0008d9dd21

codecov · 2024-05-03T14:57:57Z

Codecov Report

Attention: Patch coverage is 72.81553% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 51.64%. Comparing base (93c8d81) to head (b075098).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9302      +/-   ##
==========================================
+ Coverage   45.29%   51.64%   +6.34%     
==========================================
  Files        1227      839     -388     
  Lines      154095    90398   -63697     
  Branches     2404        0    -2404     
==========================================
- Hits        69805    46685   -23120     
+ Misses      84098    43713   -40385     
+ Partials      192        0     -192

Flag	Coverage Δ
backend	`41.81% <72.81%> (+<0.01%)`	⬆️
harness	`64.09% <ø> (ø)`
web	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/config/config.go	`70.65% <100.00%> (+0.10%)`	⬆️
master/cmd/determined-master/migrate.go	`16.00% <0.00%> (ø)`
master/internal/db/setup.go	`47.05% <0.00%> (ø)`
master/internal/db/postgres.go	`54.25% <82.35%> (+3.37%)`	⬆️
master/internal/db/postgres_test_utils.go	`82.15% <59.09%> (-1.18%)`	⬇️
master/internal/db/migrations.go	`66.22% <77.04%> (+6.65%)`	⬆️

... and 393 files with indirect coverage changes

NicholasBlaskey · 2024-05-03T15:05:33Z

@jgongd tagged you wanted get thoughts if this design works for streaming updates since yall have a ton of database code

jgongd · 2024-05-03T19:39:15Z

This is a nice way to update views, triggers and functions. Streaming updates is working as expected. cc: @gt2345 @corban-beaird

ioga · 2024-05-03T23:03:06Z

master/internal/db/postgres_test_utils.go

 }

 // MustResolveTestPostgres is the same as ResolveTestPostgres but with panics on errors.
-func MustResolveTestPostgres(t *testing.T) *PgDB {
-	pgDB, err := ResolveTestPostgres()
+func MustResolveTestPostgres(t *testing.T) (*PgDB, func()) {


is this an emerging style in go land to return an object + closing/cancellation function? I thought something like

db := MustResolveTestPostgres(...) defer db.Close()

would be more idiomatic

Yeah I have seen it in some places. I know Ryan used it in func ResolveNewPostgresDatabase() (*PgDB, func(), error) {. I think I have seen it in other established Go APIs but I'm not recalling where. The one advantage I see is it more intentional that you need to close something. Its hard for someone to know if they need to or don't need to call the Close().

Doing that, the linter complains that we aren't checking the error. We could add another helper or something. I wanna do another pass on this code as a follow on later to reduce duplication and split tests into isolated db tests soon (so each package would be running in a seperate database instance).

internal/db/postgres_rbac_intg_test.go:354:18: Error return value of `pgDB.Close` is not checked (errcheck)

NicholasBlaskey · 2024-05-03T19:34:21Z

master/internal/db/postgres_test_utils.go

 // ResolveTestPostgres resolves a connection to a postgres database. To debug tests that use this
 // (or otherwise run the tests outside of the Makefile), make sure to set
 // DET_INTEGRATION_POSTGRES_URL.
-func ResolveTestPostgres() (*PgDB, error) {
+func ResolveTestPostgres() (*PgDB, func(), error) {


we didn't close these tests meaning even before this change we have a ton of connections that get left over the course of a test run

I think adding the extra few queries to migrate the test pushed this over the edge. The db handling is kinda of a mess right now. We probably should move away from TestMain everywhere and add per package db isolation.

NicholasBlaskey · 2024-05-06T16:17:39Z

master/internal/db/postgres_test_utils.go

 }

 // MustResolveTestPostgres is the same as ResolveTestPostgres but with panics on errors.
-func MustResolveTestPostgres(t *testing.T) *PgDB {
-	pgDB, err := ResolveTestPostgres()
+func MustResolveTestPostgres(t *testing.T) (*PgDB, func()) {


Yeah I have seen it in some places. I know Ryan used it in func ResolveNewPostgresDatabase() (*PgDB, func(), error) {. I think I have seen it in other established Go APIs but I'm not recalling where. The one advantage I see is it more intentional that you need to close something. Its hard for someone to know if they need to or don't need to call the Close().

Doing that, the linter complains that we aren't checking the error. We could add another helper or something. I wanna do another pass on this code as a follow on later to reduce duplication and split tests into isolated db tests soon (so each package would be running in a seperate database instance).

internal/db/postgres_rbac_intg_test.go:354:18: Error return value of `pgDB.Close` is not checked (errcheck)

mackrorysd · 2024-05-13T17:58:36Z

master/cmd/determined-master/migrate.go

@@ -46,7 +46,7 @@ func runMigrate(cmd *cobra.Command, args []string) error {
 		}
 	}()

-	if _, err = database.Migrate(config.DB.Migrations, args); err != nil {
+	if _, err = database.Migrate(config.DB.Migrations, config.DB.DatabaseCode, args); err != nil {


Don't mean to bikeshed, but I'd love a more specific term than "database code". I hear database code and I picture all the files containing queries, migrations, etc. Took me a minute to remember what this ticket referred to. Can't think of a great suggestion myself. In my head these are the "stateless" entities but that's probably not the right term.

Maybe "ViewsAndTriggers", just to make the intended contents very clear. Calling it "stateless" or "transient" also suggests that you should be putting anything in there that can't get deleted and recreated routinely.

renamed to views_and_triggers, its not 100% accurate but it probably is more clear the intention of it

mackrorysd

I have a couple of suggestions / comments / questions, but I wouldn't block merging once you've considered them.

mackrorysd · 2024-05-13T18:20:38Z

master/internal/db/migrations.go

+	}
+
+	// Update our hash and return we need to create db code.
+	if _, err := db.sql.Exec("UPDATE db_code_hash SET hash = $1", ourHash); err != nil {


Reapplying DB code if the hash has changed makes sense, especially to ensure during development it's current. My first thought was that something human-readable here in another column would be great for troubleshooting. It should always match the current running master process anyway, so maybe that's not valuable.. Just thinking out loud.

Seems like overall kill to store something like all the view definitions in the column. Any other ideas?

mackrorysd · 2024-05-13T18:21:44Z

master/internal/db/migrations.go

+
+	// Update our hash and return we need to create db code.
+	if _, err := db.sql.Exec("UPDATE db_code_hash SET hash = $1", ourHash); err != nil {
+		return nil, false, fmt.Errorf("updating our database hash: %w", err)


Shouldn't we be returning true here?

no just returning the zero value.

If an error is not nil the other return values should not be looked at. it is really hard to work with code that returns non nil errors and 'meaningful' values.

mackrorysd · 2024-05-13T18:34:15Z

master/internal/db/migrations.go

+	return nil
+}
+
+var testOnlyDBLock func(sql *sqlx.DB) (unlock func())


This took me a minute to figure out - a comment indicating it gets initialized and acquired in init() in a test module would be a clarity improvement IMO.

I like the way it was factored out, though - much cleaner

added comment

mackrorysd · 2024-05-13T18:56:55Z

master/internal/usergroup/group_intg_test.go

@@ -396,8 +399,8 @@ var (
 	}
 	testGroups = []model.Group{testGroup, testGroupStatic}
 	testUser   = model.User{
-		ID:       1217651234,
-		Username: fmt.Sprintf("IntegrationTest%d", 1217651234),
+		ID:       1217651235,


Just curious - why is this incrementing?

this was the same value as tests in rbac tests. technically these tests run concurrently as the other test so there is a race condition where one test is in progress running and other test tries to create the user but fails due to duplicate user.

i think this was intended to be globally unique but is a copypasta error.

mackrorysd · 2024-05-13T18:58:43Z

master/static/db_code/README.md

+- The Postgres schema ```determined_code``` will be dropped if it exists.
+- Migrations run as they did before ```determined_code``` was added.
+- The Postgres schema ```determined_code``` will be recreated.
+- All SQL files in the ``db_code`` will be run in lexicographically order.


mackrorysd · 2024-05-13T19:00:07Z

master/static/db_code/README.md

+
+Migrations can't see or use views because migrations happen before ```determined_code``` is created.
+
+In the unlikely event this is an issue, you can track views in regular migrations.


Seems like the most likely scenario for an interaction would be a view or trigger referencing a table in the same commit that the table gets created in. For that reason, should we drop code, run migrations, and then recreate the code?

I think this is the order it will run in?

On everytime the Determined master starts up

The Postgres schema determined_code will be dropped if it exists.

Migrations run as they did before determined_code was added.

The Postgres schema determined_code will be recreated.

All SQL files in the db_code will be run in lexicographical order.

eecsliu

Just one non blocking clarification. I really like the change!

eecsliu · 2024-05-15T18:13:55Z

master/internal/db/migrations.go

+}
+
+func (db *PgDB) dropDBCode() error {
+	// SET search_path since the ALTER DATABASE ... SET SEARCH_PATH won't apply to this connection


im confused by this comment - can you elaborate?

oh I think this comment is out of date, see https://github.com/determined-ai/determined/pull/9302/files/3f6fe3ef6d5bed31c711ad171757e22ac090f6ee#diff-5b35744fbe2a9f5695da280e46848172deb3cdaedada8395015b997c1cb87578R209

cla-bot bot added the cla-signed label May 3, 2024

NicholasBlaskey changed the title ~~chore: store database code as code~~ chore: store database code as code [DET-7021] May 3, 2024

NicholasBlaskey requested review from ioga, mackrorysd, eecsliu and jgongd May 3, 2024 15:05

NicholasBlaskey marked this pull request as ready for review May 3, 2024 20:00

NicholasBlaskey requested review from a team as code owners May 3, 2024 20:00

NicholasBlaskey requested a review from ShreyaLnuHpe May 3, 2024 20:00

ioga reviewed May 3, 2024

View reviewed changes

NicholasBlaskey commented May 6, 2024

View reviewed changes

NicholasBlaskey requested a review from ioga May 6, 2024 17:32

NicholasBlaskey changed the title ~~chore: store database code as code [DET-7021]~~ chore: store database code as code [DET-9180] May 6, 2024

mackrorysd reviewed May 13, 2024

View reviewed changes

mackrorysd approved these changes May 13, 2024

View reviewed changes

eecsliu approved these changes May 15, 2024

View reviewed changes

NicholasBlaskey added 7 commits May 15, 2024 15:00

chore: store database code as code

b703f90

fix deadlock

2eb61d0

uhhhh

95b8a18

test fix hopefully

28572d2

fix dbs

b4bda45

lighting

53646f9

sean's feedback

1848947

NicholasBlaskey force-pushed the db_code_2 branch from 3f6fe3e to 1848947 Compare May 15, 2024 19:00

rebase fix

7764c28

change

b075098

NicholasBlaskey enabled auto-merge (squash) May 15, 2024 19:37

NicholasBlaskey disabled auto-merge May 15, 2024 19:57

NicholasBlaskey merged commit 653a0de into main May 15, 2024
80 of 97 checks passed

NicholasBlaskey deleted the db_code_2 branch May 15, 2024 19:59

kkunapuli pushed a commit that referenced this pull request May 16, 2024

chore: store database code as code [DET-9180] (#9302)

aa028d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: store database code as code [DET-9180] #9302

chore: store database code as code [DET-9180] #9302

NicholasBlaskey commented May 3, 2024 •

edited

Loading

netlify bot commented May 3, 2024 •

edited

Loading

codecov bot commented May 3, 2024 •

edited

Loading

NicholasBlaskey commented May 3, 2024

jgongd commented May 3, 2024

ioga May 3, 2024

NicholasBlaskey May 6, 2024 •

edited

Loading

NicholasBlaskey May 3, 2024

NicholasBlaskey May 6, 2024 •

edited

Loading

mackrorysd May 13, 2024

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

mackrorysd left a comment

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

mackrorysd May 13, 2024

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

mackrorysd May 13, 2024

mackrorysd May 13, 2024

NicholasBlaskey May 15, 2024

eecsliu left a comment

eecsliu May 15, 2024

NicholasBlaskey May 15, 2024


		Migrations can't see or use views because migrations happen before ```determined_code``` is created.

		In the unlikely event this is an issue, you can track views in regular migrations.

chore: store database code as code [DET-9180] #9302

chore: store database code as code [DET-9180] #9302

Conversation

NicholasBlaskey commented May 3, 2024 • edited Loading

Ticket

Description

Test Plan

NOTE TO REVIEWERS

Checklist

netlify bot commented May 3, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented May 3, 2024 • edited Loading

Codecov Report

NicholasBlaskey commented May 3, 2024

jgongd commented May 3, 2024

Choose a reason for hiding this comment

NicholasBlaskey May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasBlaskey May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackrorysd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eecsliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasBlaskey commented May 3, 2024 •

edited

Loading

netlify bot commented May 3, 2024 •

edited

Loading

codecov bot commented May 3, 2024 •

edited

Loading

NicholasBlaskey May 6, 2024 •

edited

Loading

NicholasBlaskey May 6, 2024 •

edited

Loading