From 6abe0dfb8d43e2d1dc560976b46586b62854017e Mon Sep 17 00:00:00 2001 From: ethanpailes Date: Mon, 28 Sep 2020 20:23:41 -0400 Subject: [PATCH] fix race in lazy computation of column offset tab (#118) This patch fixes a race in the way that the column offset tables were getting computed. Previously, multiple goroutines could attempt to Scan a record of the same type at once and then compute the offset table at the same time. They could then write different slice headers at the same time, potentially corrupting data. The fix is to guard each offset table with a RWMutex. The scan routine acquires a read lock whenever it accesses the table and the update routine acquires a write lock before updating it. We could make the critical section for the update routine shorter (by just wrapping the actual slice update), but I thought it would be better to prevent readers from enqueueing more writers and causing thrash. I did not really benchmark this. This race is unlikely to occur unless a service is under heavy load, but I was able to write a test that confirms that it is in fact a real issue. Threading code is tricky, so when we audit this for bugs, we want to make sure that: - Scan has released all locks by the time it returns (I needed fairly fine grained control of when the lock was held so I had to kick it C style rather than using defer). - No read lock can ever be held by a given goroutine when it calls fillColPosTab, as this would deadlock. Fixes #115 --- cmd/pggen/test/db.sql | 7 ++ cmd/pggen/test/models/escape_hatches.go | 32 +++++++- cmd/pggen/test/models/pggen.toml | 3 + cmd/pggen/test/race_test.go | 76 +++++++++++++++++++ .../extending_models/models/models.gen.go | 18 ++++- .../models/pggen_prelude.gen.go | 36 +++++---- examples/id_in_set/models/models.gen.go | 18 ++++- .../id_in_set/models/pggen_prelude.gen.go | 36 +++++---- examples/include_specs/models/models.gen.go | 46 +++++++++-- .../include_specs/models/pggen_prelude.gen.go | 36 +++++---- examples/middleware/models/models.gen.go | 18 ++++- .../middleware/models/pggen_prelude.gen.go | 36 +++++---- examples/middleware/output.txt | 6 -- .../query_argument_names/models/models.gen.go | 18 ++++- .../models/pggen_prelude.gen.go | 36 +++++---- examples/single_results/models/models.gen.go | 18 ++++- .../models/pggen_prelude.gen.go | 36 +++++---- examples/timestamps/models/models.gen.go | 18 ++++- .../timestamps/models/pggen_prelude.gen.go | 36 +++++---- gen/gen_pgclient.go | 5 ++ gen/gen_prelude.go | 36 +++++---- gen/gen_table.go | 14 +++- tools/test.bash | 5 +- 23 files changed, 409 insertions(+), 181 deletions(-) create mode 100644 cmd/pggen/test/race_test.go diff --git a/cmd/pggen/test/db.sql b/cmd/pggen/test/db.sql index da2d812..38092fa 100644 --- a/cmd/pggen/test/db.sql +++ b/cmd/pggen/test/db.sql @@ -360,6 +360,13 @@ CREATE TABLE funky_enums ( enum_val funky_name_enum NOT NULL ); +-- for exclusive use by the TestOffsetTableFilling test, +-- will have columns added and removed during testing +CREATE TABLE offset_table_fillings ( + id SERIAL PRIMARY KEY, + i1 integer NOT NULL +); + -- -- Load Data -- diff --git a/cmd/pggen/test/models/escape_hatches.go b/cmd/pggen/test/models/escape_hatches.go index bc08b0b..650d871 100644 --- a/cmd/pggen/test/models/escape_hatches.go +++ b/cmd/pggen/test/models/escape_hatches.go @@ -1,10 +1,38 @@ package models +import ( + "reflect" + "unsafe" +) + // expose some private stuff for testing purposes func (p *PGClient) ClearCaches() { - newClient := PGClient{impl: p.impl, topLevelDB: p.topLevelDB} - *p = newClient + // It would be nice if we could take advantage of zero values by just clobbering the + // PGClient with: + // + // ``` + // newClient := PGClient{impl: p.impl, topLevelDB: p.topLevelDB} + // *p = newClient + // ``` + // + // But then we will be copying/clobbering mutexes which govet is not a fan of. + + v := reflect.ValueOf(p).Elem() + emptySlice := []int{} + emptySliceV := reflect.ValueOf(emptySlice) + for i := 0; i < v.NumField(); i++ { + field := v.Field(i) + if field.Type().Kind() != reflect.Slice { + continue + } + + // We resort to unsafe shenannigans to enable us to use reflection to set + // unexported fields. We are not really breaking privacy here because the + // PGClient type is defined in this module. + field = reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem() + field.Set(emptySliceV) + } } func (tx *TxPGClient) ClearCaches() { diff --git a/cmd/pggen/test/models/pggen.toml b/cmd/pggen/test/models/pggen.toml index 2730524..8b2723e 100644 --- a/cmd/pggen/test/models/pggen.toml +++ b/cmd/pggen/test/models/pggen.toml @@ -431,3 +431,6 @@ [[table]] name = "funky_enums" + +[[table]] + name = "offset_table_fillings" diff --git a/cmd/pggen/test/race_test.go b/cmd/pggen/test/race_test.go new file mode 100644 index 0000000..8d64670 --- /dev/null +++ b/cmd/pggen/test/race_test.go @@ -0,0 +1,76 @@ +package test + +import ( + "fmt" + "sync" + "testing" + + "github.com/opendoor-labs/pggen/cmd/pggen/test/models" +) + +// file: race_test.go +// +// this file contains tests that attempt to exercise potentially racy code +// + +// NOTE: I have been able to confirm that this test will reliably turn up +// data races by commenting out all the lock calls in the generated Scan routine +// for OffsetTableFilling and then running `go test -race --run TestOffsetTableFilling` +// in a loop. Re-generating the models code makes the race warnings go away. +func TestOffsetTableFilling(t *testing.T) { + nscanners := 100 + nmods := 10 + + // insert some data so the results are not empty, not really needed but somehow + // makes me fell better. + id, err := pgClient.InsertOffsetTableFilling(ctx, &models.OffsetTableFilling{ + I1: 1, + }) + chkErr(t, err) + + // start mucking about with the table + errchan := make(chan error) + modRoutine := func() { + for i := 0; i < nmods; i++ { + _, err := pgClient.Handle().ExecContext( + ctx, fmt.Sprintf("ALTER TABLE offset_table_fillings ADD COLUMN i%d integer", i+2)) + errchan <- err + } + } + + wg := sync.WaitGroup{} + wg.Add(nscanners) + scanRoutine := func(tid int) { + for i := 0; i < 100; i++ { + // don't check the error, as it might be something like + // "sorry, too many clients already" or "cached plan must not change result type". + // we don't actually care about these issues, we just want to see if we will + // get a race with `go test -race` + pgClient.GetOffsetTableFilling(ctx, id) // nolint: errcheck + } + wg.Done() + } + + for i := 0; i < (nscanners / 2); i++ { + go scanRoutine(i) + } + go modRoutine() + for i := 0; i < (nscanners / 2); i++ { + go scanRoutine((nscanners / 2) + i) + } + + wg.Wait() + for i := 0; i < nmods; i++ { + err := <-errchan + chkErr(t, err) + } + + _, err = pgClient.Handle().ExecContext(ctx, ` + DROP TABLE offset_table_fillings; + CREATE TABLE offset_table_fillings ( + id SERIAL PRIMARY KEY, + i1 integer NOT NULL + ); + `) + chkErr(t, err) +} diff --git a/examples/extending_models/models/models.gen.go b/examples/extending_models/models/models.gen.go index c32c5bc..588d67b 100644 --- a/examples/extending_models/models/models.gen.go +++ b/examples/extending_models/models/models.gen.go @@ -12,6 +12,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -25,9 +26,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForDog sync.RWMutex colIdxTabForDog []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -772,16 +777,20 @@ type Dog struct { } func (r *Dog) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForDog.RLock() if client.colIdxTabForDog == nil { + client.rwlockForDog.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForDog, - `dogs`, + &client.rwlockForDog, + rs, &client.colIdxTabForDog, ) if err != nil { return err } + client.rwlockForDog.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForDog @@ -794,6 +803,7 @@ func (r *Dog) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForDog[genIdx](r, &nullableTgts) } } + client.rwlockForDog.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -803,11 +813,14 @@ func (r *Dog) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForDog.RLock() if len(client.colIdxTabForDog) != len(colNames) { + client.rwlockForDog.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForDog, - `drop_cols`, + &client.rwlockForDog, + rs, &client.colIdxTabForDog, ) if err != nil { @@ -816,6 +829,7 @@ func (r *Dog) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForDog.RUnlock() return err } } diff --git a/examples/extending_models/models/pggen_prelude.gen.go b/examples/extending_models/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/extending_models/models/pggen_prelude.gen.go +++ b/examples/extending_models/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/id_in_set/models/models.gen.go b/examples/id_in_set/models/models.gen.go index 8b37286..fe3d53e 100644 --- a/examples/id_in_set/models/models.gen.go +++ b/examples/id_in_set/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -24,9 +25,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForFoo sync.RWMutex colIdxTabForFoo []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -846,16 +851,20 @@ type Foo struct { } func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForFoo.RLock() if client.colIdxTabForFoo == nil { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `foos`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { return err } + client.rwlockForFoo.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForFoo @@ -868,6 +877,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForFoo[genIdx](r, &nullableTgts) } } + client.rwlockForFoo.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -877,11 +887,14 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForFoo.RLock() if len(client.colIdxTabForFoo) != len(colNames) { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `drop_cols`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { @@ -890,6 +903,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForFoo.RUnlock() return err } } diff --git a/examples/id_in_set/models/pggen_prelude.gen.go b/examples/id_in_set/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/id_in_set/models/pggen_prelude.gen.go +++ b/examples/id_in_set/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/include_specs/models/models.gen.go b/examples/include_specs/models/models.gen.go index 10f1975..448c288 100644 --- a/examples/include_specs/models/models.gen.go +++ b/examples/include_specs/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -24,11 +25,17 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForGrandparent sync.RWMutex colIdxTabForGrandparent []int + rwlockForParent sync.RWMutex colIdxTabForParent []int + rwlockForChild sync.RWMutex colIdxTabForChild []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -2627,16 +2634,20 @@ type Parent struct { } func (r *Parent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForParent.RLock() if client.colIdxTabForParent == nil { + client.rwlockForParent.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForParent, - `parents`, + &client.rwlockForParent, + rs, &client.colIdxTabForParent, ) if err != nil { return err } + client.rwlockForParent.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForParent @@ -2649,6 +2660,7 @@ func (r *Parent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error scanTgts[runIdx] = scannerTabForParent[genIdx](r, &nullableTgts) } } + client.rwlockForParent.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -2658,11 +2670,14 @@ func (r *Parent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForParent.RLock() if len(client.colIdxTabForParent) != len(colNames) { + client.rwlockForParent.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForParent, - `drop_cols`, + &client.rwlockForParent, + rs, &client.colIdxTabForParent, ) if err != nil { @@ -2671,6 +2686,7 @@ func (r *Parent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error return r.Scan(ctx, client, rs) } else { + client.rwlockForParent.RUnlock() return err } } @@ -2719,16 +2735,20 @@ type Grandparent struct { } func (r *Grandparent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForGrandparent.RLock() if client.colIdxTabForGrandparent == nil { + client.rwlockForGrandparent.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForGrandparent, - `grandparents`, + &client.rwlockForGrandparent, + rs, &client.colIdxTabForGrandparent, ) if err != nil { return err } + client.rwlockForGrandparent.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForGrandparent @@ -2741,6 +2761,7 @@ func (r *Grandparent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) scanTgts[runIdx] = scannerTabForGrandparent[genIdx](r, &nullableTgts) } } + client.rwlockForGrandparent.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -2750,11 +2771,14 @@ func (r *Grandparent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForGrandparent.RLock() if len(client.colIdxTabForGrandparent) != len(colNames) { + client.rwlockForGrandparent.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForGrandparent, - `drop_cols`, + &client.rwlockForGrandparent, + rs, &client.colIdxTabForGrandparent, ) if err != nil { @@ -2763,6 +2787,7 @@ func (r *Grandparent) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) return r.Scan(ctx, client, rs) } else { + client.rwlockForGrandparent.RUnlock() return err } } @@ -2813,16 +2838,20 @@ type Child struct { } func (r *Child) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForChild.RLock() if client.colIdxTabForChild == nil { + client.rwlockForChild.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForChild, - `children`, + &client.rwlockForChild, + rs, &client.colIdxTabForChild, ) if err != nil { return err } + client.rwlockForChild.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForChild @@ -2835,6 +2864,7 @@ func (r *Child) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error scanTgts[runIdx] = scannerTabForChild[genIdx](r, &nullableTgts) } } + client.rwlockForChild.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -2844,11 +2874,14 @@ func (r *Child) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForChild.RLock() if len(client.colIdxTabForChild) != len(colNames) { + client.rwlockForChild.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForChild, - `drop_cols`, + &client.rwlockForChild, + rs, &client.colIdxTabForChild, ) if err != nil { @@ -2857,6 +2890,7 @@ func (r *Child) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error return r.Scan(ctx, client, rs) } else { + client.rwlockForChild.RUnlock() return err } } diff --git a/examples/include_specs/models/pggen_prelude.gen.go b/examples/include_specs/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/include_specs/models/pggen_prelude.gen.go +++ b/examples/include_specs/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/middleware/models/models.gen.go b/examples/middleware/models/models.gen.go index 593b571..0cf6bfa 100644 --- a/examples/middleware/models/models.gen.go +++ b/examples/middleware/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -24,9 +25,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForFoo sync.RWMutex colIdxTabForFoo []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -747,16 +752,20 @@ type Foo struct { } func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForFoo.RLock() if client.colIdxTabForFoo == nil { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `foos`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { return err } + client.rwlockForFoo.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForFoo @@ -769,6 +778,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForFoo[genIdx](r, &nullableTgts) } } + client.rwlockForFoo.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -778,11 +788,14 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForFoo.RLock() if len(client.colIdxTabForFoo) != len(colNames) { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `drop_cols`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { @@ -791,6 +804,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForFoo.RUnlock() return err } } diff --git a/examples/middleware/models/pggen_prelude.gen.go b/examples/middleware/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/middleware/models/pggen_prelude.gen.go +++ b/examples/middleware/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/middleware/output.txt b/examples/middleware/output.txt index cdd1fdf..13ae600 100644 --- a/examples/middleware/output.txt +++ b/examples/middleware/output.txt @@ -7,11 +7,5 @@ QueryContext query: INSERT INTO "foos" ("value") VALUES ($1) QueryRowContext query: UPDATE "foos" SET ("id","value") = ($1, $2) WHERE "id" = $3 RETURNING "id" ExecContext query: DELETE FROM "foos" WHERE "id" = ANY($1) QueryContext query: SELECT * FROM "foos" WHERE "id" = ANY($1) -QueryContext query: - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - bax lish diff --git a/examples/query_argument_names/models/models.gen.go b/examples/query_argument_names/models/models.gen.go index c5df9b6..5722d2d 100644 --- a/examples/query_argument_names/models/models.gen.go +++ b/examples/query_argument_names/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -24,9 +25,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForUser sync.RWMutex colIdxTabForUser []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -901,16 +906,20 @@ type User struct { } func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForUser.RLock() if client.colIdxTabForUser == nil { + client.rwlockForUser.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForUser, - `users`, + &client.rwlockForUser, + rs, &client.colIdxTabForUser, ) if err != nil { return err } + client.rwlockForUser.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForUser @@ -923,6 +932,7 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForUser[genIdx](r, &nullableTgts) } } + client.rwlockForUser.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -932,11 +942,14 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForUser.RLock() if len(client.colIdxTabForUser) != len(colNames) { + client.rwlockForUser.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForUser, - `drop_cols`, + &client.rwlockForUser, + rs, &client.colIdxTabForUser, ) if err != nil { @@ -945,6 +958,7 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForUser.RUnlock() return err } } diff --git a/examples/query_argument_names/models/pggen_prelude.gen.go b/examples/query_argument_names/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/query_argument_names/models/pggen_prelude.gen.go +++ b/examples/query_argument_names/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/single_results/models/models.gen.go b/examples/single_results/models/models.gen.go index d83b16f..7c77c55 100644 --- a/examples/single_results/models/models.gen.go +++ b/examples/single_results/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" ) // PGClient wraps either a 'sql.DB' or a 'sql.Tx'. All pggen-generated @@ -24,9 +25,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForFoo sync.RWMutex colIdxTabForFoo []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -816,16 +821,20 @@ type Foo struct { } func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForFoo.RLock() if client.colIdxTabForFoo == nil { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `foos`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { return err } + client.rwlockForFoo.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForFoo @@ -838,6 +847,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForFoo[genIdx](r, &nullableTgts) } } + client.rwlockForFoo.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -847,11 +857,14 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForFoo.RLock() if len(client.colIdxTabForFoo) != len(colNames) { + client.rwlockForFoo.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForFoo, - `drop_cols`, + &client.rwlockForFoo, + rs, &client.colIdxTabForFoo, ) if err != nil { @@ -860,6 +873,7 @@ func (r *Foo) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForFoo.RUnlock() return err } } diff --git a/examples/single_results/models/pggen_prelude.gen.go b/examples/single_results/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/single_results/models/pggen_prelude.gen.go +++ b/examples/single_results/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/examples/timestamps/models/models.gen.go b/examples/timestamps/models/models.gen.go index d490235..0063be1 100644 --- a/examples/timestamps/models/models.gen.go +++ b/examples/timestamps/models/models.gen.go @@ -11,6 +11,7 @@ import ( "github.com/opendoor-labs/pggen/include" "github.com/opendoor-labs/pggen/unstable" "strings" + "sync" "time" ) @@ -25,9 +26,13 @@ type PGClient struct { // saw in the table we used to generate code. This means that you don't have to worry // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. + rwlockForUser sync.RWMutex colIdxTabForUser []int } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // @@ -915,16 +920,20 @@ type User struct { } func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockForUser.RLock() if client.colIdxTabForUser == nil { + client.rwlockForUser.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabForUser, - `users`, + &client.rwlockForUser, + rs, &client.colIdxTabForUser, ) if err != nil { return err } + client.rwlockForUser.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsForUser @@ -937,6 +946,7 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { scanTgts[runIdx] = scannerTabForUser[genIdx](r, &nullableTgts) } } + client.rwlockForUser.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -946,11 +956,14 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockForUser.RLock() if len(client.colIdxTabForUser) != len(colNames) { + client.rwlockForUser.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabForUser, - `drop_cols`, + &client.rwlockForUser, + rs, &client.colIdxTabForUser, ) if err != nil { @@ -959,6 +972,7 @@ func (r *User) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { return r.Scan(ctx, client, rs) } else { + client.rwlockForUser.RUnlock() return err } } diff --git a/examples/timestamps/models/pggen_prelude.gen.go b/examples/timestamps/models/pggen_prelude.gen.go index c87d246..060cacf 100644 --- a/examples/timestamps/models/pggen_prelude.gen.go +++ b/examples/timestamps/models/pggen_prelude.gen.go @@ -9,6 +9,7 @@ import ( "fmt" uuid "github.com/satori/go.uuid" "strings" + "sync" "time" "github.com/opendoor-labs/pggen" @@ -148,18 +149,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -167,15 +166,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -187,6 +184,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/gen/gen_pgclient.go b/gen/gen_pgclient.go index 9d2402f..8d1eeca 100644 --- a/gen/gen_pgclient.go +++ b/gen/gen_pgclient.go @@ -11,6 +11,7 @@ import ( func (g *Generator) genPGClient(into io.Writer, tables []config.TableConfig) error { g.imports[`"github.com/opendoor-labs/pggen"`] = true g.imports[`"database/sql"`] = true + g.imports[`"sync"`] = true type genCtx struct { ModelNames []string @@ -40,10 +41,14 @@ type PGClient struct { // about migrations merging in a slightly different order than their timestamps have // breaking 'SELECT *'. {{- range .ModelNames }} + rwlockFor{{ . }} sync.RWMutex colIdxTabFor{{ . }} []int {{- end }} } +// bogus usage so we can compile with no tables configured +var _ = sync.RWMutex{} + // NewPGClient creates a new PGClient out of a '*sql.DB' or a // custom wrapper around a db connection. // diff --git a/gen/gen_prelude.go b/gen/gen_prelude.go index 9eec57a..9dac471 100644 --- a/gen/gen_prelude.go +++ b/gen/gen_prelude.go @@ -37,6 +37,7 @@ import ( "database/sql/driver" "fmt" "strings" + "sync" "time" uuid "github.com/satori/go.uuid" @@ -177,18 +178,16 @@ func parenWrap(in string) string { func (p *PGClient) fillColPosTab( ctx context.Context, genTimeColIdxTab map[string]int, - tableName string, + rwlock *sync.RWMutex, + rows *sql.Rows, tab *[]int, // out ) error { - rows, err := p.topLevelDB.QueryContext(ctx, ` + "`" + ` - SELECT a.attname - FROM pg_attribute a - JOIN pg_class c ON (c.oid = a.attrelid) - WHERE a.attisdropped = false AND c.relname = $1 AND a.attnum > 0 - ` + "`" + `, tableName) - if err != nil { - return err - } + // We need to ensure that writes to the slice header are atomic. We want to + // aquire the lock sooner rather than later to avoid lots of reader goroutines + // queuing up computations to compute the position table and causing lock + // contention. + rwlock.Lock() + defer rwlock.Unlock() type idxMapping struct { gen int @@ -196,15 +195,13 @@ func (p *PGClient) fillColPosTab( } indicies := []idxMapping{} - for i := 0; rows.Next(); i++ { - var colName string - err = rows.Scan(&colName) - if err != nil { - return err - } - - genIdx, ok := genTimeColIdxTab[colName] - if !ok { + cols, err := rows.Columns() + if err != nil { + return fmt.Errorf("reading column names: %s", err.Error()) + } + for i, colName := range cols { + genIdx, inTable := genTimeColIdxTab[colName] + if !inTable { genIdx = -1 // this is a new column } @@ -216,6 +213,7 @@ func (p *PGClient) fillColPosTab( for _, mapping := range indicies { posTab[mapping.run] = mapping.gen } + *tab = posTab return nil diff --git a/gen/gen_table.go b/gen/gen_table.go index 5fd6581..30bc1da 100644 --- a/gen/gen_table.go +++ b/gen/gen_table.go @@ -22,6 +22,7 @@ func (g *Generator) genTables(into io.Writer, tables []config.TableConfig) error g.imports[`"context"`] = true g.imports[`"fmt"`] = true g.imports[`"strings"`] = true + g.imports[`"sync"`] = true g.imports[`"github.com/lib/pq"`] = true g.imports[`"github.com/opendoor-labs/pggen/include"`] = true g.imports[`"github.com/opendoor-labs/pggen/unstable"`] = true @@ -145,16 +146,20 @@ type {{ .GoName }} struct { {{- end}} } func (r *{{ .GoName }}) Scan(ctx context.Context, client *PGClient, rs *sql.Rows) error { + client.rwlockFor{{ .GoName }}.RLock() if client.colIdxTabFor{{ .GoName }} == nil { + client.rwlockFor{{ .GoName }}.RUnlock() // release the lock to allow the write lock to be aquired err := client.fillColPosTab( ctx, genTimeColIdxTabFor{{ .GoName }}, - ` + "`" + `{{ .PgName }}` + "`" + `, + &client.rwlockFor{{ .GoName }}, + rs, &client.colIdxTabFor{{ .GoName }}, ) if err != nil { return err } + client.rwlockFor{{ .GoName }}.RLock() // get the lock back for the rest of the routine } var nullableTgts nullableScanTgtsFor{{ .GoName }} @@ -167,6 +172,7 @@ func (r *{{ .GoName }}) Scan(ctx context.Context, client *PGClient, rs *sql.Rows scanTgts[runIdx] = scannerTabFor{{ .GoName }}[genIdx](r, &nullableTgts) } } + client.rwlockFor{{ .GoName }}.RUnlock() // we are now done referencing the idx tab in the happy path err := rs.Scan(scanTgts...) if err != nil { @@ -176,11 +182,14 @@ func (r *{{ .GoName }}) Scan(ctx context.Context, client *PGClient, rs *sql.Rows if colsErr != nil { return fmt.Errorf("pggen: checking column names: %s", colsErr.Error()) } + client.rwlockFor{{ .GoName }}.RLock() if len(client.colIdxTabFor{{ .GoName }}) != len(colNames) { + client.rwlockFor{{ .GoName }}.RUnlock() // release the lock to allow the write lock to be aquired err = client.fillColPosTab( ctx, genTimeColIdxTabFor{{ .GoName }}, - ` + "`" + `drop_cols` + "`" + `, + &client.rwlockFor{{ .GoName }}, + rs, &client.colIdxTabFor{{ .GoName }}, ) if err != nil { @@ -189,6 +198,7 @@ func (r *{{ .GoName }}) Scan(ctx context.Context, client *PGClient, rs *sql.Rows return r.Scan(ctx, client, rs) } else { + client.rwlockFor{{ .GoName }}.RUnlock() return err } } diff --git a/tools/test.bash b/tools/test.bash index d516cba..b06ba5c 100755 --- a/tools/test.bash +++ b/tools/test.bash @@ -31,8 +31,11 @@ psql "$DB_URL" < cmd/pggen/test/db.sql if [[ -n "${LINT+x}" ]] ; then golangci-lint run -E gofmt -E gosec -E gocyclo -E deadcode +elif go version | grep '1.11' 2>&1 >/dev/null ; then + # for some reason the race detector acts weird with go 1.11 + go test -p 1 ./... else # We have to serialize the tests because the example tests will re-write the database # schema dynamically. We could fix this by creating a dedicated database for the example tests. - go test -p 1 ./... + go test -race -p 1 ./... fi