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

@@ -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
 --

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() {

cmd/pggen/test/models/pggen.toml

@@ -431,3 +431,6 @@
 
 [[table]]
     name = "funky_enums"
+
+[[table]]
+    name = "offset_table_fillings"

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)
+}