From b8da3bc7e68ce5f82e625621d76bcbdcfe0f1c38 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Mon, 6 Dec 2021 10:36:20 -0600
Subject: [PATCH 01/51] checksum All() instead of Count(All()) to cover index
 keys

---
 client/client.go | 1 -
 ctl/backup.go    | 1 -
 ctl/chksum.go    | 6 +++---
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/client/client.go b/client/client.go
index 5fd3efa9c..211e7c3f7 100644
--- a/client/client.go
+++ b/client/client.go
@@ -808,7 +808,6 @@ func (c *Client) shardsMax() (map[string]uint64, error) {
 }
 
 // HTTPRequest sends an HTTP request to the Pilosa server (used by idk)
-// nolint: deadcode
 func (c *Client) HTTPRequest(method string, path string, data []byte, headers map[string]string) (status int, body []byte, err error) {
 	span := c.tracer.StartSpan("Client.HTTPRequest")
 
diff --git a/ctl/backup.go b/ctl/backup.go
index 6e7e52e83..98d6140c9 100644
--- a/ctl/backup.go
+++ b/ctl/backup.go
@@ -286,7 +286,6 @@ func (cmd *BackupCommand) backupShardNode(ctx context.Context, indexName string,
 func (cmd *BackupCommand) backupIndexTranslateData(ctx context.Context, name string) error {
 	partitionN := topology.DefaultPartitionN
 
-	// Back up all bitmap data for the index.
 	ch := make(chan int, partitionN)
 	for partitionID := 0; partitionID < partitionN; partitionID++ {
 		ch <- partitionID
diff --git a/ctl/chksum.go b/ctl/chksum.go
index 5514fc362..5d508dccc 100644
--- a/ctl/chksum.go
+++ b/ctl/chksum.go
@@ -8,7 +8,7 @@ import (
 	"io"
 
 	"github.com/cespare/xxhash"
-	"github.com/molecula/featurebase/v2"
+	pilosa "github.com/molecula/featurebase/v2"
 	"github.com/molecula/featurebase/v2/server"
 )
 
@@ -61,12 +61,12 @@ func (cmd *ChkSumCommand) Run(ctx context.Context) (err error) {
 	h := xxhash.New()
 
 	for _, ii := range schema.Indexes {
-		qa := &pilosa.QueryRequest{Index: ii.Name, Query: "Count(All())"}
+		qa := &pilosa.QueryRequest{Index: ii.Name, Query: "All()"}
 		rs, err := client.Query(ctx, ii.Name, qa)
 		if err != nil {
 			return err
 		}
-		all := rs.Results[0].(uint64)
+		all := rs.Results[0]
 		as := fmt.Sprintf("all=%v", all)
 		_, _ = h.Write([]byte(as))
 

From 7a8f0135b35ca43a58483873527bc992b8fcc740 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Mon, 6 Dec 2021 10:36:55 -0600
Subject: [PATCH 02/51] redirect GetTranslateData if node doesn't own partition

---
 api.go          | 18 ++++++++++++++++++
 http/handler.go |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/api.go b/api.go
index 07e971599..6fd8d38cc 100644
--- a/api.go
+++ b/api.go
@@ -834,6 +834,15 @@ func (api *API) FragmentData(ctx context.Context, indexName, fieldName, viewName
 	return f, nil
 }
 
+type RedirectError struct {
+	HostPort string
+	error    string
+}
+
+func (r RedirectError) Error() string {
+	return r.error
+}
+
 // TranslateData returns all translation data in the specified partition.
 func (api *API) TranslateData(ctx context.Context, indexName string, partition int) (io.WriterTo, error) {
 	span, _ := tracing.StartSpanFromContext(ctx, "API.TranslateData")
@@ -849,6 +858,15 @@ func (api *API) TranslateData(ctx context.Context, indexName string, partition i
 		return nil, newNotFoundError(ErrIndexNotFound, indexName)
 	}
 
+	snap := topology.NewClusterSnapshot(api.cluster.noder, api.cluster.Hasher, api.cluster.ReplicaN)
+	nodes := snap.PartitionNodes(partition)
+	if nodes[0].ID != api.server.NodeID() {
+		return nil, RedirectError{
+			HostPort: nodes[0].URI.HostPort(),
+			error:    fmt.Sprintf("can't translate data, this node(%s) does not partition %d", api.server.uri, partition),
+		}
+	}
+
 	// Retrieve translatestore from holder.
 	store := idx.TranslateStore(partition)
 	if store == nil {
diff --git a/http/handler.go b/http/handler.go
index 94665bbfa..9e626babf 100644
--- a/http/handler.go
+++ b/http/handler.go
@@ -2326,6 +2326,12 @@ func (h *Handler) handleGetTranslateData(w http.ResponseWriter, r *http.Request)
 
 	// Retrieve partition data from holder.
 	p, err := h.api.TranslateData(r.Context(), q.Get("index"), int(partition))
+	if redir, ok := err.(pilosa.RedirectError); ok {
+		newURL := *r.URL
+		newURL.Host = redir.HostPort
+		http.Redirect(w, r, newURL.String(), http.StatusSeeOther)
+		return
+	}
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusNotFound)
 		return

From 3761fc6d3ca7a4043889831464edba003ec0017f Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Mon, 6 Dec 2021 11:35:11 -0600
Subject: [PATCH 03/51] have Circle build release on branch instead of waiting
 for merge

---
 .circleci/config.yml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 498c56ea3..8e1713235 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -292,8 +292,6 @@ workflows:
           filters:
             tags:
               only: /^v.*/
-            branches:
-              only: master
       - publish_release:
           context: molecula
           requires:

From aff3d3ddd9248dc75f7ca121eed8b9ad83bcbb1f Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Tue, 7 Dec 2021 14:58:35 -0600
Subject: [PATCH 04/51] do a backup in a go test for coverage purposes

also found a weird issue with schema marshalling

if you create a field thru the api w/o specifying a field type, you
get slightly different behavior than going thru the HTTP handler which
is... not ideal. I changed the marshaler to accept an empty field type.
---
 ctl/backup.go    |  2 +-
 executor_test.go | 73 ++++++++++++++++++++++++++++++++++++------------
 field.go         |  4 +--
 3 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/ctl/backup.go b/ctl/backup.go
index 98d6140c9..4c6c006ec 100644
--- a/ctl/backup.go
+++ b/ctl/backup.go
@@ -18,7 +18,7 @@ import (
 	"golang.org/x/sync/errgroup"
 )
 
-// BackupCommand represents a command for backing up a Pilosa node.
+// BackupCommand represents a command for backing up a FeatureBase node.
 type BackupCommand struct { // nolint: maligned
 	tlsConfig *tls.Config
 
diff --git a/executor_test.go b/executor_test.go
index 56023107e..9c8aa595f 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -27,6 +27,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	pilosa "github.com/molecula/featurebase/v2"
 	"github.com/molecula/featurebase/v2/boltdb"
+	"github.com/molecula/featurebase/v2/ctl"
 	"github.com/molecula/featurebase/v2/disco"
 	"github.com/molecula/featurebase/v2/http"
 	"github.com/molecula/featurebase/v2/pql"
@@ -7015,11 +7016,16 @@ func TestMissingKeyRegression(t *testing.T) {
 // (single and multi-node clusters, different endpoints for the
 // queries (HTTP, GRPC, Postgres), etc.).
 func TestVariousQueries(t *testing.T) {
-	for _, clusterSize := range []int{1, 3, 4, 7} {
+	for _, clusterSize := range []int{1, 3, 7} {
 		clusterSize := clusterSize
 		t.Run(fmt.Sprintf("%d-node", clusterSize), func(t *testing.T) {
 			c := test.MustRunCluster(t, clusterSize)
 			defer c.Close()
+
+			// put a variety of data into the cluster
+			populateTestData(t, c)
+			backupTest(t, c)
+
 			variousQueries(t, c)
 			variousQueriesOnTimeFields(t, c)
 			variousQueriesOnPercentiles(t, c)
@@ -7028,6 +7034,30 @@ func TestVariousQueries(t *testing.T) {
 	}
 }
 
+func backupTest(t *testing.T, c *test.Cluster) {
+	// should this really be in executor? No. But all these
+	// integration-y query tests probably shouldn't be either. My goal
+	// putting this here is to take advantage of already-existing
+	// clusters and data.
+
+	td, err := testhook.TempDir(t, "backupTest")
+	if err != nil {
+		t.Fatalf("can't even get a temp dir, what a ripoff: %v", err)
+	}
+	td = td + "/backupTest"
+
+	buf := &bytes.Buffer{}
+	backupCommand := ctl.NewBackupCommand(nil, buf, buf)
+	backupCommand.Host = c.Nodes[len(c.Nodes)-1].URL() // don't pick node 0 so we don't always get primary (better code coverage)
+	backupCommand.Index = usersIndex
+	backupCommand.OutputDir = td
+
+	if err := backupCommand.Run(context.Background()); err != nil {
+		t.Log(buf.String())
+		t.Fatalf("running backup: %v", err)
+	}
+}
+
 // tests for abbreviating time values in queries
 func variousQueriesOnPercentiles(t *testing.T, c *test.Cluster) {
 	// todo, make rand more random, 42 isnt the answer to everything
@@ -7332,10 +7362,12 @@ func variousQueriesOnTimeFields(t *testing.T, c *test.Cluster) {
 	}
 }
 
-func variousQueries(t *testing.T, c *test.Cluster) {
+var usersIndex = "users"
+
+func populateTestData(t *testing.T, c *test.Cluster) {
 	// Create and populate "likenums" similar to "likes", but without keys on the field.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "likenums")
-	c.ImportIDKey(t, "users", "likenums", []test.KeyID{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "likenums")
+	c.ImportIDKey(t, usersIndex, "likenums", []test.KeyID{
 		{ID: 1, Key: "userA"},
 		{ID: 2, Key: "userB"},
 		{ID: 3, Key: "userC"},
@@ -7353,8 +7385,8 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 	})
 
 	// Create and populate "likes" field.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "likes", pilosa.OptFieldKeys())
-	c.ImportKeyKey(t, "users", "likes", [][2]string{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "likes", pilosa.OptFieldKeys())
+	c.ImportKeyKey(t, usersIndex, "likes", [][2]string{
 		{"molecula", "userA"},
 		{"pilosa", "userB"},
 		{"pangolin", "userC"},
@@ -7370,8 +7402,8 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 	})
 
 	// Create and populate "dinner" field.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "dinner", pilosa.OptFieldKeys())
-	c.ImportKeyKey(t, "users", "dinner", [][2]string{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "dinner", pilosa.OptFieldKeys())
+	c.ImportKeyKey(t, usersIndex, "dinner", [][2]string{
 		{"leftovers", "userB"},
 		{"pizza", "userA"},
 		{"pizza", "userB"},
@@ -7381,11 +7413,11 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 	})
 
 	// Create and populate "places_visited" time field.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "places_visited", pilosa.OptFieldKeys(), pilosa.OptFieldTypeTime(pilosa.TimeQuantum("YM")))
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "places_visited", pilosa.OptFieldKeys(), pilosa.OptFieldTypeTime(pilosa.TimeQuantum("YM")))
 	ts2019Jan01 := int64(1546300800) * 1e+9 // 2019 January 1st 0:00:00
 	ts2019Aug01 := int64(1564617600) * 1e+9 // 2019 August  1st 0:00:00
 	ts2020Jan01 := int64(1577836800) * 1e+9 // 2020 January 1st 0:00:00
-	c.ImportTimeQuantumKey(t, "users", "places_visited", []test.TimeQuantumKey{
+	c.ImportTimeQuantumKey(t, usersIndex, "places_visited", []test.TimeQuantumKey{
 		// 2019 January: nairobi, paris, austin, toronto
 		{RowKey: "nairobi", ColKey: "userB", Ts: ts2019Jan01},
 		{RowKey: "paris", ColKey: "userC", Ts: ts2019Jan01},
@@ -7405,8 +7437,8 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 	})
 
 	// Create and populate "affinity" int field with negative, positive, zero and null values.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "affinity", pilosa.OptFieldTypeInt(-1000, 1000))
-	c.ImportIntKey(t, "users", "affinity", []test.IntKey{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "affinity", pilosa.OptFieldTypeInt(-1000, 1000))
+	c.ImportIntKey(t, usersIndex, "affinity", []test.IntKey{
 		{Val: 10, Key: "userA"},
 		{Val: -10, Key: "userB"},
 		{Val: 5, Key: "userC"},
@@ -7415,8 +7447,8 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 	})
 
 	// Create and populate "net_worth" int field with positive values.
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "net_worth", pilosa.OptFieldTypeInt(-100000000, 100000000))
-	c.ImportIntKey(t, "users", "net_worth", []test.IntKey{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "net_worth", pilosa.OptFieldTypeInt(-100000000, 100000000))
+	c.ImportIntKey(t, usersIndex, "net_worth", []test.IntKey{
 		{Val: 1, Key: "userA"},
 		{Val: 10, Key: "userB"},
 		{Val: 100, Key: "userC"},
@@ -7425,8 +7457,8 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 		{Val: 100000, Key: "userF"},
 	})
 
-	c.CreateField(t, "users", pilosa.IndexOptions{Keys: true, TrackExistence: true}, "zip_code", pilosa.OptFieldTypeInt(0, 100000))
-	c.ImportIntKey(t, "users", "zip_code", []test.IntKey{
+	c.CreateField(t, usersIndex, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "zip_code", pilosa.OptFieldTypeInt(0, 100000))
+	c.ImportIntKey(t, usersIndex, "zip_code", []test.IntKey{
 		{Val: 78739, Key: "userA"},
 		{Val: 78739, Key: "userB"},
 		{Val: 19707, Key: "userC"},
@@ -7434,7 +7466,12 @@ func variousQueries(t *testing.T, c *test.Cluster) {
 		{Val: 86753, Key: "userE"},
 		{Val: 78739, Key: "userG"},
 	})
+}
 
+func variousQueries(t *testing.T, c *test.Cluster) {
+	// NOTE: this relies on populateTestData being called first
+
+	// define and run a bunch of tests
 	tests := []struct {
 		query       string
 		qrVerifier  func(t *testing.T, resp pilosa.QueryResponse)
@@ -7781,8 +7818,8 @@ leftovers,1
 
 	for i, tst := range tests {
 		t.Run(fmt.Sprintf("%d-%s", i, tst.query), func(t *testing.T) {
-			resp := c.Query(t, "users", tst.query)
-			tr := c.QueryGRPC(t, "users", tst.query)
+			resp := c.Query(t, usersIndex, tst.query)
+			tr := c.QueryGRPC(t, usersIndex, tst.query)
 			if tst.qrVerifier != nil {
 				tst.qrVerifier(t, resp)
 			}
diff --git a/field.go b/field.go
index 7076c6e98..2aa865a1f 100644
--- a/field.go
+++ b/field.go
@@ -1884,7 +1884,7 @@ func applyDefaultOptions(o *FieldOptions) FieldOptions {
 // are included.
 func (o *FieldOptions) MarshalJSON() ([]byte, error) {
 	switch o.Type {
-	case FieldTypeSet:
+	case FieldTypeSet, "":
 		return json.Marshal(struct {
 			Type      string `json:"type"`
 			CacheType string `json:"cacheType"`
@@ -1975,7 +1975,7 @@ func (o *FieldOptions) MarshalJSON() ([]byte, error) {
 			o.Type,
 		})
 	}
-	return nil, errors.New("invalid field type")
+	return nil, errors.Errorf("invalid field type: '%s'", o.Type)
 }
 
 // MinTimestamp returns the minimum value for a timestamp field.

From 3d3080df8bfdf518cbcf20b01ca1fa3c64a00ce5 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Tue, 7 Dec 2021 17:12:41 -0600
Subject: [PATCH 05/51] full backup/restore test in a Go test

---
 ctl/chksum.go    |  4 ++--
 executor_test.go | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/ctl/chksum.go b/ctl/chksum.go
index 5d508dccc..28ae7b23c 100644
--- a/ctl/chksum.go
+++ b/ctl/chksum.go
@@ -92,7 +92,7 @@ func (cmd *ChkSumCommand) Run(ctx context.Context) (err error) {
 				}
 				for _, item := range res.Results {
 					rowids := item.(*pilosa.RowIdentifiers)
-					//either rowids or keys
+					// either rowids or keys
 					for _, row := range rowids.Keys {
 						countPql := fmt.Sprintf(`Count(Row(%v="%v"))`, field.Name, row)
 						qr := &pilosa.QueryRequest{Index: ii.Name, Query: countPql}
@@ -121,7 +121,7 @@ func (cmd *ChkSumCommand) Run(ctx context.Context) (err error) {
 			}
 
 		}
-		fmt.Printf("hash:%x\n", h.Sum(nil))
+		fmt.Fprintf(cmd.Stdout, "hash:%x\n", h.Sum(nil))
 	}
 
 	return nil
diff --git a/executor_test.go b/executor_test.go
index 9c8aa595f..f73b93cc9 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -7040,6 +7040,35 @@ func backupTest(t *testing.T, c *test.Cluster) {
 	// putting this here is to take advantage of already-existing
 	// clusters and data.
 
+	sum := chkSumCluster(t, c)
+
+	backupDir := backupCluster(t, c)
+
+	cnew := test.MustRunCluster(t, 3) // this way we test 1->3 3->3 7->3
+	defer cnew.Close()
+
+	restoreCluster(t, backupDir, cnew)
+
+	sumNew := chkSumCluster(t, cnew)
+
+	if sum != sumNew {
+		t.Fatalf("old/new checksum mismatch, old:\n%s\nnew:\n:%s", sum, sumNew)
+	}
+}
+
+func chkSumCluster(t *testing.T, c *test.Cluster) string {
+	buf := &bytes.Buffer{}
+
+	chkSum := ctl.NewChkSumCommand(nil, buf, buf)
+	chkSum.Host = c.Nodes[len(c.Nodes)-1].URL()
+	if err := chkSum.Run(context.Background()); err != nil {
+		t.Fatalf("running checksum: %v", err)
+	}
+
+	return buf.String()
+}
+
+func backupCluster(t *testing.T, c *test.Cluster) (backupDir string) {
 	td, err := testhook.TempDir(t, "backupTest")
 	if err != nil {
 		t.Fatalf("can't even get a temp dir, what a ripoff: %v", err)
@@ -7056,6 +7085,18 @@ func backupTest(t *testing.T, c *test.Cluster) {
 		t.Log(buf.String())
 		t.Fatalf("running backup: %v", err)
 	}
+	return td
+}
+
+func restoreCluster(t *testing.T, backupDir string, c *test.Cluster) {
+	buf := &bytes.Buffer{}
+
+	restore := ctl.NewRestoreCommand(nil, buf, buf)
+	restore.Host = c.Nodes[len(c.Nodes)-1].URL()
+	restore.Path = backupDir
+	if err := restore.Run(context.Background()); err != nil {
+		t.Fatalf("restoring: %v", err)
+	}
 }
 
 // tests for abbreviating time values in queries

From b0d29bb425cbd0b6a1d16b2fc92cd0c354fb4b59 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Tue, 7 Dec 2021 18:30:51 -0600
Subject: [PATCH 06/51] fix unrelated data race that randomly cropped up in CI

---
 api.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/api.go b/api.go
index 6fd8d38cc..d39a61f3d 100644
--- a/api.go
+++ b/api.go
@@ -996,7 +996,10 @@ func (api *API) Usage(ctx context.Context, remote bool) (map[string]NodeUsage, e
 		return resp, nil
 	}
 
-	if api.usageCache.lastCalcDuration < usageCacheMinDuration {
+	api.usageCache.muAssign.Lock()
+	lastCalc := api.usageCache.lastCalcDuration
+	api.usageCache.muAssign.Unlock()
+	if lastCalc < usageCacheMinDuration {
 		err := api.ResetUsageCache()
 		if err != nil {
 			api.server.logger.Infof("could not reset usageCache: %s", err)

From 1980c8b8e59775a66858c46651f1d64590532bc1 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 13:49:15 -0600
Subject: [PATCH 07/51] featurebase backup: don't hide
 TranslateStoreNotFoundError

I think this shouldn't happen unless there's actually a problem
---
 ctl/backup.go | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ctl/backup.go b/ctl/backup.go
index 4c6c006ec..995fb2b23 100644
--- a/ctl/backup.go
+++ b/ctl/backup.go
@@ -317,9 +317,7 @@ func (cmd *BackupCommand) backupIndexPartitionTranslateData(ctx context.Context,
 	logger.Printf("backing up index translation data: %s/%d", name, partitionID)
 
 	rc, err := cmd.client.IndexTranslateDataReader(ctx, name, partitionID)
-	if err == pilosa.ErrTranslateStoreNotFound {
-		return nil
-	} else if err != nil {
+	if err != nil {
 		return fmt.Errorf("fetching translate data reader: %w", err)
 	}
 	defer rc.Close()

From ea267202bda5187bc20f9832caf02f369e2681ed Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 14:15:17 -0600
Subject: [PATCH 08/51] more complete backup/restore coverage in go tests

I think we can remove the shell version now
---
 executor_test.go | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/executor_test.go b/executor_test.go
index f73b93cc9..b2c9455d3 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -6749,7 +6749,7 @@ func variousQueriesCountDistinctTimestamp(t *testing.T, c *test.Cluster) {
 	field := "ts"
 
 	// create an index and timestamp field
-	c.CreateField(t, index, pilosa.IndexOptions{}, field, pilosa.OptFieldTypeTimestamp(time.Unix(0, 0), "s"))
+	c.CreateField(t, index, pilosa.IndexOptions{TrackExistence: true}, field, pilosa.OptFieldTypeTimestamp(time.Unix(0, 0), "s"))
 
 	// add some data
 	data := []string{"2010-01-02T12:32:00Z", "2010-04-20T12:32:00Z", "2011-04-20T12:32:00Z"}
@@ -7024,17 +7024,18 @@ func TestVariousQueries(t *testing.T) {
 
 			// put a variety of data into the cluster
 			populateTestData(t, c)
-			backupTest(t, c)
+			backupTest(t, c, usersIndex)
 
 			variousQueries(t, c)
 			variousQueriesOnTimeFields(t, c)
 			variousQueriesOnPercentiles(t, c)
 			variousQueriesCountDistinctTimestamp(t, c)
+			backupTest(t, c, "") // test backup/restore of all indexes
 		})
 	}
 }
 
-func backupTest(t *testing.T, c *test.Cluster) {
+func backupTest(t *testing.T, c *test.Cluster, index string) {
 	// should this really be in executor? No. But all these
 	// integration-y query tests probably shouldn't be either. My goal
 	// putting this here is to take advantage of already-existing
@@ -7042,7 +7043,7 @@ func backupTest(t *testing.T, c *test.Cluster) {
 
 	sum := chkSumCluster(t, c)
 
-	backupDir := backupCluster(t, c)
+	backupDir := backupCluster(t, c, index)
 
 	cnew := test.MustRunCluster(t, 3) // this way we test 1->3 3->3 7->3
 	defer cnew.Close()
@@ -7068,7 +7069,7 @@ func chkSumCluster(t *testing.T, c *test.Cluster) string {
 	return buf.String()
 }
 
-func backupCluster(t *testing.T, c *test.Cluster) (backupDir string) {
+func backupCluster(t *testing.T, c *test.Cluster, index string) (backupDir string) {
 	td, err := testhook.TempDir(t, "backupTest")
 	if err != nil {
 		t.Fatalf("can't even get a temp dir, what a ripoff: %v", err)
@@ -7078,7 +7079,7 @@ func backupCluster(t *testing.T, c *test.Cluster) (backupDir string) {
 	buf := &bytes.Buffer{}
 	backupCommand := ctl.NewBackupCommand(nil, buf, buf)
 	backupCommand.Host = c.Nodes[len(c.Nodes)-1].URL() // don't pick node 0 so we don't always get primary (better code coverage)
-	backupCommand.Index = usersIndex
+	backupCommand.Index = index
 	backupCommand.OutputDir = td
 
 	if err := backupCommand.Run(context.Background()); err != nil {
@@ -8430,7 +8431,7 @@ func MinMaxTimestampNodeTester(t *testing.T, numNodes int) {
 	defer c.Close()
 
 	// create an index and timestamp field
-	c.CreateField(t, index, pilosa.IndexOptions{}, field, pilosa.OptFieldTypeTimestamp(time.Unix(0, 0), "s"))
+	c.CreateField(t, index, pilosa.IndexOptions{TrackExistence: true}, field, pilosa.OptFieldTypeTimestamp(time.Unix(0, 0), "s"))
 
 	// add some data
 	expected := "2010-01-02T12:32:00Z"

From 53373240ef247ef11aab13e2e4f50407d59ab6f9 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 15:03:18 -0600
Subject: [PATCH 09/51] make chksum process All() results correctly for unkeyed
 indexes

---
 ctl/backup.go    | 13 ++++++++-----
 ctl/chksum.go    | 11 ++++++++---
 executor_test.go |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/ctl/backup.go b/ctl/backup.go
index 995fb2b23..0e8a257f1 100644
--- a/ctl/backup.go
+++ b/ctl/backup.go
@@ -183,12 +183,17 @@ func (cmd *BackupCommand) backupIDAllocData(ctx context.Context) error {
 func (cmd *BackupCommand) backupIndexTranslation(ctx context.Context, ii *pilosa.IndexInfo) error {
 	logger := cmd.Logger()
 	logger.Printf("backing up index translation: %q", ii.Name)
-	if err := cmd.backupIndexTranslateData(ctx, ii.Name); err != nil {
-		return err
+	if ii.Options.Keys {
+		if err := cmd.backupIndexTranslateData(ctx, ii.Name); err != nil {
+			return err
+		}
 	}
 
 	// Back up field translation data.
 	for _, fi := range ii.Fields {
+		if !fi.Options.Keys {
+			continue
+		}
 		if err := cmd.backupFieldTranslateData(ctx, ii.Name, fi.Name); err != nil {
 			return fmt.Errorf("cannot backup field translation data for field %q on index %q: %w", fi.Name, ii.Name, err)
 		}
@@ -346,9 +351,7 @@ func (cmd *BackupCommand) backupFieldTranslateData(ctx context.Context, indexNam
 	logger.Printf("backing up field translation data: %s/%s", indexName, fieldName)
 
 	rc, err := cmd.client.FieldTranslateDataReader(ctx, indexName, fieldName)
-	if err == pilosa.ErrTranslateStoreNotFound {
-		return nil
-	} else if err != nil {
+	if err != nil {
 		return fmt.Errorf("fetching translate data reader: %w", err)
 	}
 	defer rc.Close()
diff --git a/ctl/chksum.go b/ctl/chksum.go
index 28ae7b23c..af2430efb 100644
--- a/ctl/chksum.go
+++ b/ctl/chksum.go
@@ -66,9 +66,14 @@ func (cmd *ChkSumCommand) Run(ctx context.Context) (err error) {
 		if err != nil {
 			return err
 		}
-		all := rs.Results[0]
-		as := fmt.Sprintf("all=%v", all)
-		_, _ = h.Write([]byte(as))
+
+		all := rs.Results[0].(*pilosa.Row)
+		if len(all.Keys) > 0 {
+			allString := fmt.Sprintf("%v", all.Keys)
+			_, _ = h.Write([]byte(allString))
+		} else {
+			_, _ = h.Write(all.Roaring())
+		}
 
 		for _, field := range ii.Fields {
 			switch field.Options.Type {
diff --git a/executor_test.go b/executor_test.go
index b2c9455d3..a06b312b0 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -7053,7 +7053,7 @@ func backupTest(t *testing.T, c *test.Cluster, index string) {
 	sumNew := chkSumCluster(t, cnew)
 
 	if sum != sumNew {
-		t.Fatalf("old/new checksum mismatch, old:\n%s\nnew:\n:%s", sum, sumNew)
+		t.Fatalf("old/new checksum mismatch, old:\n%s\nnew:\n%s", sum, sumNew)
 	}
 }
 

From f2a6ee738c3d15943475f0f5f100e2e340bb3df0 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 15:12:41 -0600
Subject: [PATCH 10/51] remove old shell-based backup/restore tests

---
 .circleci/config.yml | 14 -------
 Dockerfile.pilosa    | 37 -----------------
 Dockerfile.runner    | 23 -----------
 Makefile             |  9 ----
 docker-compose-3.yml | 97 --------------------------------------------
 testBackupRestore.sh | 57 --------------------------
 6 files changed, 237 deletions(-)
 delete mode 100644 Dockerfile.pilosa
 delete mode 100644 Dockerfile.runner
 delete mode 100644 docker-compose-3.yml
 delete mode 100755 testBackupRestore.sh

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 8e1713235..c3e1a43e4 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -144,16 +144,6 @@ jobs:
       - run:
           command: make test-external-lookup EXTERNAL_LOOKUP_DSN=postgresql://postgres:password@localhost/circle_test?sslmode=disable
           no_output_timeout: 30m
-  test-backup-restore:
-    executor:
-      name: golang
-    steps:
-      - checkout-plus
-      - skip-if-root-unchanged
-      - setup_remote_docker
-      - run: echo -n $DOCKER_PASS | docker login -u $DOCKER_USER --password-stdin
-      - run: make backuptests-build 
-      - run: make backuptests
   cluster-tests:
     executor:
       name: golang
@@ -273,10 +263,6 @@ workflows:
           context: molecula
           requires:
             - setup
-      - test-backup-restore:
-          context: molecula
-          requires:
-            - setup
       - cluster-tests:
           context: molecula
           requires:
diff --git a/Dockerfile.pilosa b/Dockerfile.pilosa
deleted file mode 100644
index 6ee2e0bbf..000000000
--- a/Dockerfile.pilosa
+++ /dev/null
@@ -1,37 +0,0 @@
-ARG GO_VERSION=latest
-
-######################
-### Pilosa builder ###
-######################
-
-FROM golang:${GO_VERSION} as pilosa-builder
-ARG MAKE_FLAGS
-WORKDIR /pilosa
-
-COPY . ./
-
-RUN make build FLAGS="-o build/featurebase" ${MAKE_FLAGS}
-
-#####################
-### Pilosa runner ###
-#####################
-
-FROM alpine:3.13.2 as runner
-
-LABEL maintainer "dev@molecula.com"
-
-RUN apk add --no-cache curl jq
-
-COPY --from=pilosa-builder /pilosa/build/featurebase /
-
-COPY NOTICE /NOTICE
-
-EXPOSE 10101
-VOLUME /data
-
-ENV PILOSA_DATA_DIR /data
-ENV PILOSA_BIND 0.0.0.0:10101
-ENV PILOSA_BIND_GRPC 0.0.0.0:20101
-
-ENTRYPOINT ["/featurebase"]
-CMD ["server"]
diff --git a/Dockerfile.runner b/Dockerfile.runner
deleted file mode 100644
index 12f49e13c..000000000
--- a/Dockerfile.runner
+++ /dev/null
@@ -1,23 +0,0 @@
-ARG GO_VERSION=latest
-
-######################
-### Pilosa builder ###
-######################
-
-FROM golang:${GO_VERSION} as pilosa-builder
-ARG MAKE_FLAGS
-WORKDIR /pilosa
-
-COPY . ./
-
-RUN make build FLAGS="-o build/featurebase" ${MAKE_FLAGS}
-
-FROM moleculacorp/idk as idk
-LABEL maintainer "dev@molecula.com"
-RUN apt-get update -y
-RUN apt-get install -y bash curl jq
-
-
-COPY --from=pilosa-builder /pilosa/build/featurebase /
-COPY testBackupRestore.sh /
-CMD ["bash","/testBackupRestore.sh"]
diff --git a/Makefile b/Makefile
index f72517684..a24d8c40a 100644
--- a/Makefile
+++ b/Makefile
@@ -155,15 +155,6 @@ clustertests: vendor
 clustertests-build: vendor
 	docker-compose -f $(DOCKER_COMPOSE) down -v
 	docker-compose -f $(DOCKER_COMPOSE) up --exit-code-from=client1 --build
-# Test Cluster backup and restore
-backuptests-build: vendor
-	docker-compose -f docker-compose-3.yml down
-	docker-compose -f docker-compose-3.yml build 
-
-backuptests: vendor
-	docker-compose -f docker-compose-3.yml down -v
-	docker-compose -f docker-compose-3.yml up --exit-code-from=client1  --abort-on-container-exit
-
 
 # Install Pilosa
 install:
diff --git a/docker-compose-3.yml b/docker-compose-3.yml
deleted file mode 100644
index ef31424d0..000000000
--- a/docker-compose-3.yml
+++ /dev/null
@@ -1,97 +0,0 @@
-version: "3"
-services:
-  pilosa0:
-    image: build/pilosa                                                                                                                                                     
-    build:                                                                                                                                                                 
-      context: .                                                                                                                                                           
-      dockerfile: Dockerfile.pilosa              
-    environment:
-      PILOSA_ADVERTISE: pilosa0:10101
-      PILOSA_ADVERTISE_GRPC: pilosa0:20101
-      PILOSA_CLUSTER_REPLICAS: 1
-      PILOSA_DATA_DIR: /data/pilosa0
-      PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS: http://pilosa0:10201
-      PILOSA_ETCD_ADVERTISE_PEER_ADDRESS: http://pilosa0:10301
-      PILOSA_ETCD_INITIAL_CLUSTER: pilosa0=http://pilosa0:10301,pilosa1=http://pilosa1:10301,pilosa2=http://pilosa2:10301
-      PILOSA_ETCD_LISTEN_CLIENT_ADDRESS: http://0.0.0.0:10201
-      PILOSA_ETCD_LISTEN_PEER_ADDRESS: http://0.0.0.0:10301
-      PILOSA_NAME: pilosa0
-      PILOSA_STORAGE_BACKEND: ${PILOSA_STORAGE_BACKEND:-rbf}
-    volumes:
-    - data:/data
-    healthcheck:
-      test: x=$$(curl -s localhost:10101/status | jq -r ".state") && [[ "$$x" == "NORMAL"  ]] || $$(exit 1)
-      interval: 10s
-      timeout: 5s
-      retries: 5
-  pilosa1:
-    image: build/pilosa                                                                                                                                                     
-    build:                                                                                                                                                                 
-      context: .                                                                                                                                                           
-      dockerfile: Dockerfile.pilosa              
-    environment:
-      PILOSA_ADVERTISE: pilosa1:10101
-      PILOSA_ADVERTISE_GRPC: pilosa1:20101
-      PILOSA_CLUSTER_REPLICAS: 1
-      PILOSA_DATA_DIR: /data/pilosa1
-      PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS: http://pilosa1:10201
-      PILOSA_ETCD_ADVERTISE_PEER_ADDRESS: http://pilosa1:10301
-      PILOSA_ETCD_INITIAL_CLUSTER: pilosa0=http://pilosa0:10301,pilosa1=http://pilosa1:10301,pilosa2=http://pilosa2:10301
-      PILOSA_ETCD_LISTEN_CLIENT_ADDRESS: http://0.0.0.0:10201
-      PILOSA_ETCD_LISTEN_PEER_ADDRESS: http://0.0.0.0:10301
-      PILOSA_NAME: pilosa1
-      PILOSA_STORAGE_BACKEND: ${PILOSA_STORAGE_BACKEND:-rbf}
-    volumes:
-    - data:/data
-  pilosa2:
-    image: build/pilosa                                                                                                                                                     
-    build:                                                                                                                                                                 
-      context: .                                                                                                                                                           
-      dockerfile: Dockerfile.pilosa              
-    environment:
-      PILOSA_ADVERTISE: pilosa2:10101
-      PILOSA_ADVERTISE_GRPC: pilosa2:20101
-      PILOSA_CLUSTER_REPLICAS: 1
-      PILOSA_DATA_DIR: /data/pilosa2
-      PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS: http://pilosa2:10201
-      PILOSA_ETCD_ADVERTISE_PEER_ADDRESS: http://pilosa2:10301
-      PILOSA_ETCD_INITIAL_CLUSTER: pilosa0=http://pilosa0:10301,pilosa1=http://pilosa1:10301,pilosa2=http://pilosa2:10301
-      PILOSA_ETCD_LISTEN_CLIENT_ADDRESS: http://0.0.0.0:10201
-      PILOSA_ETCD_LISTEN_PEER_ADDRESS: http://0.0.0.0:10301
-      PILOSA_NAME: pilosa2
-      PILOSA_STORAGE_BACKEND: ${PILOSA_STORAGE_BACKEND:-rbf}
-    volumes:
-    - data:/data
-  pilosax:
-    image: build/pilosa                                                                                                                                                     
-    build:                                                                                                                                                                 
-      context: .                                                                                                                                                           
-      dockerfile: Dockerfile.pilosa              
-    environment:
-      PILOSA_ADVERTISE: pilosax:10101
-      PILOSA_ADVERTISE_GRPC: pilosax:20101
-      PILOSA_CLUSTER_REPLICAS: 1
-      PILOSA_DATA_DIR: /data/pilosax
-      PILOSA_ETCD_ADVERTISE_CLIENT_ADDRESS: http://pilosax:10201
-      PILOSA_ETCD_ADVERTISE_PEER_ADDRESS: http://pilosax:10301
-      PILOSA_ETCD_INITIAL_CLUSTER: pilosax=http://pilosax:10301
-      PILOSA_ETCD_LISTEN_CLIENT_ADDRESS: http://0.0.0.0:10201
-      PILOSA_ETCD_LISTEN_PEER_ADDRESS: http://0.0.0.0:10301
-      PILOSA_NAME: pilosax
-      PILOSA_STORAGE_BACKEND: ${PILOSA_STORAGE_BACKEND:-rbf}
-    volumes:
-    - data:/data
-  client1:
-    image: tgruben/bash
-    build:
-      context: .
-      dockerfile: Dockerfile.runner
-    environment:
-        - GO111MODULE=on
-    volumes:
-        - /var/run/docker.sock:/var/run/docker.sock
-    depends_on: 
-      - pilosa0
-    
-volumes:
-    data:
diff --git a/testBackupRestore.sh b/testBackupRestore.sh
deleted file mode 100755
index 7bcbe3894..000000000
--- a/testBackupRestore.sh
+++ /dev/null
@@ -1,57 +0,0 @@
-#!/bin/bash
-
-set -eux
-
-declare STATUS="NORMAL"
-declare TIMEOUT=30
-sleep 4
-STATUS=$STATUS timeout -s TERM $TIMEOUT bash -c \
-    'while [[ ${STATUS_RECEIVED} != ${STATUS} ]];\
-    	do STATUS_RECEIVED=$(curl --connect-timeout 1 -s pilosa0:10101/status | jq  -r ".state") && \
-        echo "received status: $STATUS_RECEIVED" && \
-        sleep 1;\
-    done;'
-echo "NOW DO STUFF"
-datagen --source kitchensink_keyed -e 9999 --pilosa.index sink --pilosa.batch-size 10000 --pilosa.hosts pilosa0:10101
-before=$(/featurebase chksum --host pilosa0:10101)
-/featurebase backup -o backupdir --host pilosa0:10101
-curl -X DELETE -s pilosa0:10101/index/sink
-/featurebase restore -s backupdir --host pilosa0:10101
-after=$(/featurebase chksum --host pilosa0:10101)
-if [ "$before" = "$after" ]; then
-	echo "PASS Cluster"
-else
-	echo "FAIL Single"
-	exit 1
-fi
-/featurebase restore -s backupdir --host pilosax:10101
-single=$(/featurebase chksum --host pilosax:10101)
-if [ "$before" = "$single" ]; then
-	echo "PASS Single"
-	exit 0
-else
-	echo "FAIL Single"
-	exit 1
-fi
-
-datagen --source texas_health -e 9999 --pilosa.index newsink --pilosa.batch-size 10000 --pilosa.hosts pilosa0:10101
-before=$(/featurebase chksum --host pilosa0:10101)
-/featurebase backup -o newbackupdir --host pilosa0:10101 --index newsink
-curl -X DELETE -s pilosa0:10101/index/newsink
-/featurebase restore -s newbackupdir --host pilosa0:10101
-after=$(/featurebase chksum --host pilosa0:10101)
-if [ "$before" = "$after" ]; then
-	echo "PASS Cluster Table"
-else
-	echo "FAIL Single Table"
-	exit 1
-fi
-/featurebase restore -s newbackupdir --host pilosax:10101
-single=$(/featurebase chksum --host pilosax:10101)
-if [ "$before" = "$single" ]; then
-	echo "PASS Single Table"
-	exit 0
-else
-	echo "FAIL Single Table"
-	exit 1
-fi

From 081184f4367e9395c9eec3cd1c1e3020ba5e9315 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 15:59:12 -0600
Subject: [PATCH 11/51] another data race? fuck

---
 translate.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/translate.go b/translate.go
index a9d547cb7..be1c47306 100644
--- a/translate.go
+++ b/translate.go
@@ -568,6 +568,8 @@ func (s *InMemTranslateStore) WriteTo(w io.Writer) (int64, error) {
 // don't expect to use InMemTranslateStore much, it's mostly there to
 // avoid disk load during testing.
 func (s *InMemTranslateStore) ReadFrom(r io.Reader) (count int64, err error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
 	var bytes []byte
 	bytes, err = ioutil.ReadAll(r)
 	count = int64(len(bytes))

From e8972e437ee42dd4ab9d50653c0bd34b777654cd Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Wed, 8 Dec 2021 17:12:50 -0600
Subject: [PATCH 12/51] smaller clusters to take less memory... test-race
 getting oom killed

---
 executor_test.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/executor_test.go b/executor_test.go
index a06b312b0..432499511 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -7016,7 +7016,7 @@ func TestMissingKeyRegression(t *testing.T) {
 // (single and multi-node clusters, different endpoints for the
 // queries (HTTP, GRPC, Postgres), etc.).
 func TestVariousQueries(t *testing.T) {
-	for _, clusterSize := range []int{1, 3, 7} {
+	for _, clusterSize := range []int{1, 3, 5} {
 		clusterSize := clusterSize
 		t.Run(fmt.Sprintf("%d-node", clusterSize), func(t *testing.T) {
 			c := test.MustRunCluster(t, clusterSize)
@@ -7045,7 +7045,7 @@ func backupTest(t *testing.T, c *test.Cluster, index string) {
 
 	backupDir := backupCluster(t, c, index)
 
-	cnew := test.MustRunCluster(t, 3) // this way we test 1->3 3->3 7->3
+	cnew := test.MustRunCluster(t, 3) // this way we test 1->3 3->3 5->3
 	defer cnew.Close()
 
 	restoreCluster(t, backupDir, cnew)

From 01e4baab04398fe4296afd4bd2316fc4332e1b22 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Fri, 10 Dec 2021 14:07:24 -0600
Subject: [PATCH 13/51] determine permission for user access to index

---
 auth/auth.go             | 114 +++++++++++++++++++
 auth/auth_test.go        | 231 +++++++++++++++++++++++++++++++++++++++
 ctl/server.go            |   2 +-
 install/featurebase.conf |   3 +-
 server/config.go         |  10 ++
 5 files changed, 358 insertions(+), 2 deletions(-)
 create mode 100644 auth/auth_test.go

diff --git a/auth/auth.go b/auth/auth.go
index 4e617c998..344055337 100644
--- a/auth/auth.go
+++ b/auth/auth.go
@@ -14,6 +14,15 @@
 
 package auth
 
+import (
+	"fmt"
+	"io/ioutil"
+	"log"
+	"path/filepath"
+
+	"gopkg.in/yaml.v2"
+)
+
 type Auth struct {
 	// Enable AuthZ/AuthN for featurebase server
 	Enable bool `toml:"enable"`
@@ -35,4 +44,109 @@ type Auth struct {
 
 	// Scope URL
 	ScopeURL string `toml:"scope-url"`
+
+	// Permissions file for groups
+	PermissionsFile string `toml:"permissions"`
+}
+
+type GroupPermissions struct {
+	Permissions []Permissions `yaml:"group_permissions"`
+}
+
+type Permissions struct {
+	GroupId    string `yaml:"groupId"`
+	Index      string `yaml:"index"`
+	Permission string `yaml:"permission"`
+}
+
+func ReadPermissionsFile(filePath string) (yamlData []byte) {
+	filePathAbs, _ := filepath.Abs(filePath)
+	yamlData, err := ioutil.ReadFile(filePathAbs)
+	if err != nil {
+		panic(err)
+	}
+	return yamlData
+}
+
+func (p *GroupPermissions) CreatePermissionsStruct(data []byte) {
+	err := yaml.Unmarshal([]byte(data), &p)
+	if err != nil {
+		log.Fatalf("Error %s", err)
+	}
+}
+
+func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) {
+	// read yaml permissions file
+	yamlData := ReadPermissionsFile(Auth.PermissionsFile)
+
+	// get group permissions
+	var p GroupPermissions
+	p.CreatePermissionsStruct(yamlData)
+
+	// check permissions for all groups and index, and return most permissive
+	return p.ResolvePermissions(groups, index)
+}
+
+func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) {
+
+	// get union of groups the user is part of obtained from identity provider and groups in permissions file
+	var groupMatch []Permissions
+	for _, group := range groups {
+		for i := range p.Permissions {
+			if group["id"] == p.Permissions[i].GroupId {
+				groupMatch = append(groupMatch, p.Permissions[i])
+			}
+		}
+	}
+
+	if len(groupMatch) == 0 {
+		return "", fmt.Errorf("User is NOT allowed access to FeatureBase")
+	}
+
+	// check that user's groups have access to the index user want to access
+	var indexMatch []Permissions
+	indexCheck := map[string]bool{}
+	for _, g := range groupMatch {
+		for _, idx := range index {
+			if idx == g.Index {
+				indexMatch = append(indexMatch, g)
+				indexCheck[idx] = true
+			}
+		}
+	}
+
+	// check that user has access to every index
+	indexCount := 0
+	for _, value := range indexCheck {
+		if value {
+			indexCount += 1
+		}
+	}
+
+	if indexCount != len(index) {
+		return "", fmt.Errorf("User is not allowed access to index: %s", index)
+	}
+
+	// check permissions for index user has access to
+	allPermissions := map[string]bool{
+		"admin": false,
+		"write": false,
+		"read":  false,
+	}
+
+	for _, g := range indexMatch {
+		if !allPermissions[g.Permission] {
+			allPermissions[g.Permission] = true
+		}
+	}
+
+	if allPermissions["admin"] {
+		return "admin", error(nil)
+	} else if allPermissions["write"] {
+		return "write", error(nil)
+	} else if allPermissions["read"] {
+		return "read", error(nil)
+	} else {
+		return "", fmt.Errorf("No permissions found")
+	}
 }
diff --git a/auth/auth_test.go b/auth/auth_test.go
new file mode 100644
index 000000000..ddd081e80
--- /dev/null
+++ b/auth/auth_test.go
@@ -0,0 +1,231 @@
+// Copyright 2017 Pilosa Corp.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package auth_test
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/molecula/featurebase/v2/auth"
+)
+
+func createStruct(inputs [][]string) (permissions auth.GroupPermissions) {
+	var sliceStruct []auth.Permissions
+	for _, i := range inputs {
+		groupId := i[0]
+		index := i[1]
+		permission := i[2]
+		p := auth.Permissions{groupId, index, permission}
+		sliceStruct = append(sliceStruct, p)
+	}
+	permissions = auth.GroupPermissions{Permissions: sliceStruct}
+	return permissions
+}
+
+func TestAuth_CreatePermissionsStruct(t *testing.T) {
+	var singleInput = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
+    index: "test"
+    permission: "read"`)
+
+	var emptyInput = []byte(`group_permissions:
+  - group:
+    groupId: ""
+    index: ""
+    permission: ""`)
+
+	var multiInput = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
+    index: "test"
+    permission: "read"
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "admin"`)
+
+	var slice1 [][]string
+	var slice2 [][]string
+	var slice3 [][]string
+	var subslice1 []string
+	var subslice2 []string
+	var subslice3 []string
+	subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read")
+	subslice2 = append(subslice2, "", "", "")
+	subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin")
+	slice1 = append(slice1, subslice1)
+	slice2 = append(slice2, subslice2)
+	slice3 = append(slice3, subslice1, subslice3)
+	singleStruct := createStruct(slice1)
+	emptyStruct := createStruct(slice2)
+	multiStruct := createStruct(slice3)
+
+	tests := []struct {
+		input  []byte
+		output auth.GroupPermissions
+	}{
+		{singleInput, singleStruct},
+		{emptyInput, emptyStruct},
+		{multiInput, multiStruct},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			var p auth.GroupPermissions
+			p.CreatePermissionsStruct(test.input)
+
+			if !reflect.DeepEqual(p, test.output) {
+				t.Fatalf("Expected output %s, but got %s", test.output, p)
+			}
+		},
+		)
+	}
+}
+
+func createGroupMaps(groups []string) []map[string]string {
+
+	var group1 []map[string]string
+	for _, i := range groups {
+		map1 := map[string]string{}
+		map1["id"] = i
+		group1 = append(group1, map1)
+	}
+	return group1
+}
+
+func TestAuth_ResolvePermissions(t *testing.T) {
+	// initializes different example of permissions file in yaml
+	var permissions1 = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
+    index: "test"
+    permission: "read"`)
+
+	var permissions2 = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "read"
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "write"`)
+
+	var permissions3 = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "read"
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "admin"`)
+
+	var permissions4 = []byte(`group_permissions:
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
+    index: "test"
+    permission: ""
+  - group:
+    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
+    index: "test"
+    permission: "admin"`)
+
+	// initializes groups that are returned from identity provider
+	groupsList1 := []string{}
+	groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"}
+	groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"}
+
+	tests := []struct {
+		permissions []byte
+		groups      []map[string]string
+		index       []string
+		userAccess  string
+		err         string
+	}{
+		{
+			permissions1,
+			createGroupMaps(groupsList1),
+			[]string{"test"},
+			"",
+			"User is NOT allowed access to FeatureBase",
+		},
+		{
+			permissions1,
+			createGroupMaps(groupsList2),
+			[]string{"test1"},
+			"",
+			"User is not allowed access to index",
+		},
+		{
+			permissions1,
+			createGroupMaps(groupsList2),
+			[]string{"test"},
+			"read",
+			"",
+		},
+		{
+			permissions2,
+			createGroupMaps(groupsList3),
+			[]string{"test"},
+			"write",
+			"",
+		},
+		{
+			permissions3,
+			createGroupMaps(groupsList3),
+			[]string{"test"},
+			"admin",
+			"",
+		},
+		{
+			permissions2,
+			createGroupMaps(groupsList3),
+			[]string{"test"},
+			"write",
+			"",
+		},
+		{
+			permissions4,
+			createGroupMaps(groupsList2),
+			[]string{"test"},
+			"",
+			"No permissions found",
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+
+			var p auth.GroupPermissions
+			p.CreatePermissionsStruct(test.permissions)
+
+			p1, err := p.ResolvePermissions(test.groups, test.index)
+
+			if p1 != test.userAccess {
+				t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1)
+			}
+
+			if err != nil {
+				if !strings.Contains(err.Error(), test.err) {
+					t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error())
+				}
+			}
+
+		})
+	}
+}
diff --git a/ctl/server.go b/ctl/server.go
index c5d43a937..06f7d1bb2 100644
--- a/ctl/server.go
+++ b/ctl/server.go
@@ -130,5 +130,5 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) {
 	flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.")
 	flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.")
 	flags.StringVar(&srv.Config.Auth.ScopeURL, "auth.scope-url", srv.Config.Auth.ScopeURL, "Identity Provider's Scope URL.")
-
+	flags.StringVar(&srv.Config.Auth.PermissionsFile, "auth.permissions", srv.Config.Auth.PermissionsFile, "Permissions' file with group authorization.")
 }
diff --git a/install/featurebase.conf b/install/featurebase.conf
index 540a410f4..5b1e73345 100644
--- a/install/featurebase.conf
+++ b/install/featurebase.conf
@@ -380,4 +380,5 @@ log-path = "/var/log/molecula/featurebase.log"
 #  authorize-url = ""
 #  token-url = ""
 #  group-endpoint-url = ""
-#  scope-url = ""
\ No newline at end of file
+#  scope-url = ""
+#  permissions = ""
\ No newline at end of file
diff --git a/server/config.go b/server/config.go
index 0c1989c7a..6f74d4f25 100644
--- a/server/config.go
+++ b/server/config.go
@@ -620,6 +620,7 @@ func (c *Config) ValidateAuth() ([]error, error) {
 		"TokenURL":         c.Auth.TokenURL,
 		"GroupEndpointURL": c.Auth.GroupEndpointURL,
 		"ScopeURL":         c.Auth.ScopeURL,
+		"PermissionsFile":  c.Auth.PermissionsFile,
 	}
 
 	errors := make([]error, 0)
@@ -636,6 +637,15 @@ func (c *Config) ValidateAuth() ([]error, error) {
 				continue
 			}
 		}
+
+		if strings.Contains(name, "File") {
+			yamlData := auth.ReadPermissionsFile(value)
+			var p auth.GroupPermissions
+			p.CreatePermissionsStruct(yamlData)
+			if len(p.Permissions) == 0 {
+				errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value))
+			}
+		}
 	}
 	if len(errors) > 0 {
 		return errors, fmt.Errorf("there were errors validating config")

From 9e2cf81127a32e918453e920582ee02afa4984bc Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Mon, 13 Dec 2021 10:35:56 -0600
Subject: [PATCH 14/51] added unit tests

---
 server/config.go               | 35 +++++++++++++++++--------
 server/config_internal_test.go | 47 +++++++++++++++++++++++++++++-----
 2 files changed, 66 insertions(+), 16 deletions(-)

diff --git a/server/config.go b/server/config.go
index 52f798abf..1f3752577 100644
--- a/server/config.go
+++ b/server/config.go
@@ -7,6 +7,7 @@ import (
 	"log"
 	"net"
 	"net/url"
+	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -613,37 +614,51 @@ func (c *Config) ValidateAuth() ([]error, error) {
 	errors := make([]error, 0)
 	for name, value := range authConfig {
 		if value == "" {
-			errors = append(errors, fmt.Errorf("empty string for auth config %s", name))
+			errors = append(errors, fmt.Errorf("Empty string for auth config %s", name))
 			continue
 		}
 
 		if strings.Contains(name, "URL") {
 			_, err := url.ParseRequestURI(value)
 			if err != nil {
-				errors = append(errors, fmt.Errorf("invalid URL for auth config %s: %s", name, err))
+				errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err))
 				continue
 			}
 		}
 
 		if strings.Contains(name, "File") {
-			yamlData := auth.ReadPermissionsFile(value)
-			var p auth.GroupPermissions
-			p.CreatePermissionsStruct(yamlData)
-			if len(p.Permissions) == 0 {
-				errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value))
+			fileExt := filepath.Ext(value)
+			if (fileExt != ".yaml") && (fileExt != ".yml") {
+				errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value))
+				continue
 			}
 		}
 	}
+
 	if len(errors) > 0 {
-		return errors, fmt.Errorf("there were errors validating config")
+		return errors, fmt.Errorf("There were errors validating config")
 	}
 	return errors, nil
 }
 
+func (c *Config) ValidatePermissions() (err error) {
+
+	yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile)
+	var p auth.GroupPermissions
+	p.CreatePermissionsStruct(yamlData)
+	if len(p.Permissions) == 0 {
+		return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile)
+	}
+	return nil
+}
+
 func (c *Config) MustValidateAuth() {
 	if errors, err := c.ValidateAuth(); err != nil {
-		for _, e := range errors {
-			log.Println(e)
+		for _, e1 := range errors {
+			log.Println(e1)
+		}
+		if e2 := c.ValidatePermissions(); e2 != nil {
+			log.Println(e2)
 		}
 		log.Fatal(err)
 	}
diff --git a/server/config_internal_test.go b/server/config_internal_test.go
index 7c762b23e..8003a1f2f 100644
--- a/server/config_internal_test.go
+++ b/server/config_internal_test.go
@@ -279,12 +279,15 @@ func TestConfig_validateAddrsGRPC(t *testing.T) {
 }
 
 func TestConfig_validateAuth(t *testing.T) {
-	errorMesgEmpty := "empty string"
-	errorMesgURL := "invalid URL"
+	errorMesgEmpty := "Empty string"
+	errorMesgURL := "Invalid URL"
+	errorMesgPermissions := "Invalid file extension"
 	validTestURL := "https://url.com/"
 	validClientID := "clientid"
 	validClientSecret := "clientSecret"
-	notValidURL := "not-a-url"
+	validFilename := "permissions.yaml"
+	invalidFilename := "permissions.txt"
+	invalidURL := "not-a-url"
 	emptyString := ""
 	enable := true
 	disable := false
@@ -303,6 +306,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -312,6 +316,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -322,6 +327,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -331,6 +337,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -341,6 +348,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -350,6 +358,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -359,6 +368,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -368,6 +378,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -376,6 +387,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -385,6 +397,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -392,6 +405,7 @@ func TestConfig_validateAuth(t *testing.T) {
 			[]string{
 				errorMesgEmpty,
 				errorMesgEmpty,
+				errorMesgEmpty,
 			},
 			auth.Auth{
 				Enable:           enable,
@@ -401,6 +415,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         validTestURL,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -412,10 +427,11 @@ func TestConfig_validateAuth(t *testing.T) {
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
-				AuthorizeURL:     notValidURL,
+				AuthorizeURL:     invalidURL,
 				TokenURL:         validTestURL,
 				GroupEndpointURL: validTestURL,
 				ScopeURL:         validTestURL,
+				PermissionsFile:  validFilename,
 			},
 		},
 		{
@@ -429,9 +445,26 @@ func TestConfig_validateAuth(t *testing.T) {
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
 				AuthorizeURL:     validTestURL,
-				TokenURL:         notValidURL,
-				GroupEndpointURL: notValidURL,
+				TokenURL:         invalidURL,
+				GroupEndpointURL: invalidURL,
 				ScopeURL:         validTestURL,
+				PermissionsFile:  validFilename,
+			},
+		},
+		{
+			// Auth enabled, permissions file is set to invalid string
+			[]string{
+				errorMesgPermissions,
+			},
+			auth.Auth{
+				Enable:           enable,
+				ClientId:         validClientID,
+				ClientSecret:     validClientSecret,
+				AuthorizeURL:     validTestURL,
+				TokenURL:         validTestURL,
+				GroupEndpointURL: validTestURL,
+				ScopeURL:         validTestURL,
+				PermissionsFile:  invalidFilename,
 			},
 		},
 		{
@@ -445,6 +478,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         validTestURL,
 				GroupEndpointURL: validTestURL,
 				ScopeURL:         validTestURL,
+				PermissionsFile:  validFilename,
 			},
 		},
 		{
@@ -458,6 +492,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
+				PermissionsFile:  emptyString,
 			},
 		},
 	}

From 32228bb1344a0cc55aa33fdbb1d7ea74cdd259d0 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Mon, 13 Dec 2021 10:48:40 -0600
Subject: [PATCH 15/51] updated go.mod

---
 go.mod | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/go.mod b/go.mod
index add6082e3..b66980688 100644
--- a/go.mod
+++ b/go.mod
@@ -54,7 +54,7 @@ require (
 	golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d // indirect
 	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
 	google.golang.org/grpc v1.28.0
-	gopkg.in/yaml.v2 v2.3.0 // indirect
+	gopkg.in/yaml.v2 v2.3.0
 	modernc.org/mathutil v1.0.0
 	modernc.org/strutil v1.0.0
 	sigs.k8s.io/yaml v1.2.0 // indirect

From 3c8a7384baae950bffc54ea8c52241ea279c96aa Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Mon, 13 Dec 2021 13:05:35 -0600
Subject: [PATCH 16/51] added reviewer's suggestions

---
 auth/auth.go                   | 35 ++++++++++-----------
 auth/auth_test.go              | 56 +++++++++++++---------------------
 server/config.go               | 10 +++---
 server/config_internal_test.go |  6 ++--
 4 files changed, 46 insertions(+), 61 deletions(-)

diff --git a/auth/auth.go b/auth/auth.go
index 0d19f9461..7376e2551 100644
--- a/auth/auth.go
+++ b/auth/auth.go
@@ -56,9 +56,9 @@ func ReadPermissionsFile(filePath string) (yamlData []byte) {
 }
 
 func (p *GroupPermissions) CreatePermissionsStruct(data []byte) {
-	err := yaml.Unmarshal([]byte(data), &p)
+	err := yaml.Unmarshal([]byte(data), p)
 	if err != nil {
-		log.Fatalf("Error %s", err)
+		log.Fatalf("error %s", err)
 	}
 }
 
@@ -87,7 +87,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index
 	}
 
 	if len(groupMatch) == 0 {
-		return "", fmt.Errorf("User is NOT allowed access to FeatureBase")
+		return "", fmt.Errorf("user is NOT allowed access to FeatureBase")
 	}
 
 	// check that user's groups have access to the index user want to access
@@ -102,16 +102,15 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index
 		}
 	}
 
-	// check that user has access to every index
-	indexCount := 0
-	for _, value := range indexCheck {
-		if value {
-			indexCount += 1
+	// check which index user does NOT have access to, and return in error mesg
+	if len(indexCheck) != len(index) {
+		var indexNotFound []string
+		for _, idx := range index {
+			if !indexCheck[idx] {
+				indexNotFound = append(indexNotFound, idx)
+			}
 		}
-	}
-
-	if indexCount != len(index) {
-		return "", fmt.Errorf("User is not allowed access to index: %s", index)
+		return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound)
 	}
 
 	// check permissions for index user has access to
@@ -122,18 +121,16 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index
 	}
 
 	for _, g := range indexMatch {
-		if !allPermissions[g.Permission] {
-			allPermissions[g.Permission] = true
-		}
+		allPermissions[g.Permission] = true
 	}
 
 	if allPermissions["admin"] {
-		return "admin", error(nil)
+		return "admin", nil
 	} else if allPermissions["write"] {
-		return "write", error(nil)
+		return "write", nil
 	} else if allPermissions["read"] {
-		return "read", error(nil)
+		return "read", nil
 	} else {
-		return "", fmt.Errorf("No permissions found")
+		return "", fmt.Errorf("no permissions found")
 	}
 }
diff --git a/auth/auth_test.go b/auth/auth_test.go
index ddd081e80..e27463c59 100644
--- a/auth/auth_test.go
+++ b/auth/auth_test.go
@@ -22,19 +22,6 @@ import (
 	"github.com/molecula/featurebase/v2/auth"
 )
 
-func createStruct(inputs [][]string) (permissions auth.GroupPermissions) {
-	var sliceStruct []auth.Permissions
-	for _, i := range inputs {
-		groupId := i[0]
-		index := i[1]
-		permission := i[2]
-		p := auth.Permissions{groupId, index, permission}
-		sliceStruct = append(sliceStruct, p)
-	}
-	permissions = auth.GroupPermissions{Permissions: sliceStruct}
-	return permissions
-}
-
 func TestAuth_CreatePermissionsStruct(t *testing.T) {
 	var singleInput = []byte(`group_permissions:
   - group:
@@ -58,21 +45,22 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) {
     index: "test"
     permission: "admin"`)
 
-	var slice1 [][]string
-	var slice2 [][]string
-	var slice3 [][]string
-	var subslice1 []string
-	var subslice2 []string
-	var subslice3 []string
-	subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read")
-	subslice2 = append(subslice2, "", "", "")
-	subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin")
-	slice1 = append(slice1, subslice1)
-	slice2 = append(slice2, subslice2)
-	slice3 = append(slice3, subslice1, subslice3)
-	singleStruct := createStruct(slice1)
-	emptyStruct := createStruct(slice2)
-	multiStruct := createStruct(slice3)
+	singleStruct := auth.GroupPermissions{
+		Permissions: []auth.Permissions{
+			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
+		},
+	}
+	emptyStruct := auth.GroupPermissions{
+		Permissions: []auth.Permissions{
+			{"", "", ""},
+		},
+	}
+	multiStruct := auth.GroupPermissions{
+		Permissions: []auth.Permissions{
+			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
+			{"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"},
+		},
+	}
 
 	tests := []struct {
 		input  []byte
@@ -89,7 +77,7 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) {
 			p.CreatePermissionsStruct(test.input)
 
 			if !reflect.DeepEqual(p, test.output) {
-				t.Fatalf("Expected output %s, but got %s", test.output, p)
+				t.Fatalf("expected output %s, but got %s", test.output, p)
 			}
 		},
 		)
@@ -162,14 +150,14 @@ func TestAuth_ResolvePermissions(t *testing.T) {
 			createGroupMaps(groupsList1),
 			[]string{"test"},
 			"",
-			"User is NOT allowed access to FeatureBase",
+			"user is NOT allowed access to FeatureBase",
 		},
 		{
 			permissions1,
 			createGroupMaps(groupsList2),
 			[]string{"test1"},
 			"",
-			"User is not allowed access to index",
+			"user is not allowed access to index",
 		},
 		{
 			permissions1,
@@ -204,7 +192,7 @@ func TestAuth_ResolvePermissions(t *testing.T) {
 			createGroupMaps(groupsList2),
 			[]string{"test"},
 			"",
-			"No permissions found",
+			"no permissions found",
 		},
 	}
 
@@ -217,12 +205,12 @@ func TestAuth_ResolvePermissions(t *testing.T) {
 			p1, err := p.ResolvePermissions(test.groups, test.index)
 
 			if p1 != test.userAccess {
-				t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1)
+				t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1)
 			}
 
 			if err != nil {
 				if !strings.Contains(err.Error(), test.err) {
-					t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error())
+					t.Errorf("expected error to contain %s, but got %s", test.err, err.Error())
 				}
 			}
 
diff --git a/server/config.go b/server/config.go
index 1f3752577..595697067 100644
--- a/server/config.go
+++ b/server/config.go
@@ -614,14 +614,14 @@ func (c *Config) ValidateAuth() ([]error, error) {
 	errors := make([]error, 0)
 	for name, value := range authConfig {
 		if value == "" {
-			errors = append(errors, fmt.Errorf("Empty string for auth config %s", name))
+			errors = append(errors, fmt.Errorf("empty string for auth config %s", name))
 			continue
 		}
 
 		if strings.Contains(name, "URL") {
 			_, err := url.ParseRequestURI(value)
 			if err != nil {
-				errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err))
+				errors = append(errors, fmt.Errorf("invalid URL for auth config %s: %s", name, err))
 				continue
 			}
 		}
@@ -629,14 +629,14 @@ func (c *Config) ValidateAuth() ([]error, error) {
 		if strings.Contains(name, "File") {
 			fileExt := filepath.Ext(value)
 			if (fileExt != ".yaml") && (fileExt != ".yml") {
-				errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value))
+				errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value))
 				continue
 			}
 		}
 	}
 
 	if len(errors) > 0 {
-		return errors, fmt.Errorf("There were errors validating config")
+		return errors, fmt.Errorf("there were errors validating config")
 	}
 	return errors, nil
 }
@@ -647,7 +647,7 @@ func (c *Config) ValidatePermissions() (err error) {
 	var p auth.GroupPermissions
 	p.CreatePermissionsStruct(yamlData)
 	if len(p.Permissions) == 0 {
-		return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile)
+		return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile)
 	}
 	return nil
 }
diff --git a/server/config_internal_test.go b/server/config_internal_test.go
index 8003a1f2f..7f387172e 100644
--- a/server/config_internal_test.go
+++ b/server/config_internal_test.go
@@ -279,9 +279,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) {
 }
 
 func TestConfig_validateAuth(t *testing.T) {
-	errorMesgEmpty := "Empty string"
-	errorMesgURL := "Invalid URL"
-	errorMesgPermissions := "Invalid file extension"
+	errorMesgEmpty := "empty string"
+	errorMesgURL := "invalid URL"
+	errorMesgPermissions := "invalid file extension"
 	validTestURL := "https://url.com/"
 	validClientID := "clientid"
 	validClientSecret := "clientSecret"

From c25ab78b03a915475749a1ada0697a819aa42c21 Mon Sep 17 00:00:00 2001
From: Todd Gruben <todd@molecula.com>
Date: Mon, 13 Dec 2021 13:10:16 -0600
Subject: [PATCH 17/51] use higherlevel iterator in order to account for ops
 log

---
 cmd/roaring-migrate/main.go | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/cmd/roaring-migrate/main.go b/cmd/roaring-migrate/main.go
index 3ccc909a6..4ca91bec0 100644
--- a/cmd/roaring-migrate/main.go
+++ b/cmd/roaring-migrate/main.go
@@ -273,21 +273,19 @@ func Migrate(dataDir, backupPath string) error {
 	})
 	//raw is now sorted by shard
 
-	// need index/field/shard
-	// make rbf file in backup
-	rowSize := uint64(0) //?
-	clear := false
-	log := false
 	cache := &rbfFile{
 		temp: filepath.Join(backupPath, "_SCRATCH"),
 	}
+	bm := roaring.NewSliceBitmap()
 	for _, filename := range raw {
 		index, field, view, shard := Extract(filename)
+
 		content, err := ioutil.ReadFile(dataDir + filename)
 		if err != nil {
 			return err
 		}
-		itr, err := roaring.NewRoaringIterator(content)
+		err = bm.UnmarshalBinary(content)
+
 		if err != nil {
 			return err
 		}
@@ -300,10 +298,13 @@ func Migrate(dataDir, backupPath string) error {
 			return err
 		}
 		key := string(txkey.Prefix(index, field, view, shard))
-		_, _, err = tx.ImportRoaringBits(key, itr, clear, log, rowSize)
-		if err != nil {
-			tx.Rollback()
-			return err
+		itr, ok := bm.Containers.Iterator(0)
+		if ok {
+			for itr.Next() {
+				k, v := itr.Value()
+				tx.PutContainer(key, k, v)
+
+			}
 		}
 		err = tx.Commit()
 		if err != nil {

From 7141662c037cfb1cec75a20d4164de1fbfffce32 Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Wed, 15 Dec 2021 07:16:30 -0700
Subject: [PATCH 18/51] Defer unlock & rollback during rbf.DB.Begin()

---
 rbf/db.go | 19 ++++++-------------
 rbf/tx.go | 10 +++++++---
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index cc4b65700..99c4f0e2f 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -444,15 +444,10 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	}
 
 	db.mu.Lock()
-	// note: We cannot defer db.mu.Unlock() here because
-	// we call tx.Rollback() before if db.readMetaPage
-	// returns an error, and thus we will deadlock against
-	// ourselves when the Rollback tries to acquire the db.mu.
-	// This is why db.mu.Unlock() is done manually below.
+	defer db.mu.Unlock()
 
 	if !db.opened {
 		cleanup()
-		db.mu.Unlock()
 		return nil, ErrClosed
 	}
 
@@ -470,6 +465,11 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 
 		DeleteEmptyContainer: true,
 	}
+	defer func() {
+		if err != nil {
+			tx.rollback(true)
+		}
+	}()
 
 	if writable {
 		tx.dirtyPages = make(map[uint32][]byte)
@@ -480,10 +480,6 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	// This page is only written at the end of a dirty transaction.
 	page, err := db.readMetaPage()
 	if err != nil {
-		// we will deadlock in tx.Rollback()
-		// on db.mu.Lock unless we manually db.mu.Unlock first.
-		db.mu.Unlock()
-		tx.Rollback()
 		return nil, err
 	}
 	copy(tx.meta[:], page)
@@ -499,13 +495,10 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	// this avoids recomputing the cache if there are no write txs for a while.
 	if db.rootRecords == nil {
 		if db.rootRecords, err = tx.RootRecords(); err != nil {
-			db.mu.Unlock()
-			tx.Rollback()
 			return nil, err
 		}
 	}
 
-	db.mu.Unlock()
 	return tx, nil
 }
 
diff --git a/rbf/tx.go b/rbf/tx.go
index 37c76a351..38fed48f6 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -126,7 +126,9 @@ func (tx *Tx) Commit() error {
 	return tx.db.removeTx(tx)
 }
 
-func (tx *Tx) Rollback() {
+func (tx *Tx) Rollback() { tx.rollback(false) }
+
+func (tx *Tx) rollback(hasDBLock bool) {
 	tx.mu.Lock()
 	defer tx.mu.Unlock()
 
@@ -141,8 +143,10 @@ func (tx *Tx) Rollback() {
 	}
 
 	// Disconnect transaction from DB.
-	tx.db.mu.Lock()
-	defer tx.db.mu.Unlock()
+	if !hasDBLock {
+		tx.db.mu.Lock()
+		defer tx.db.mu.Unlock()
+	}
 	vprint.PanicOn(tx.db.removeTx(tx))
 }
 

From 48913caafdbb3a8ae37ed356eceb29831732be26 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Wed, 15 Dec 2021 13:35:30 -0600
Subject: [PATCH 19/51] renamed package to authz, inmplemented reviewer's
 feedback

---
 auth/auth.go => authz/authz.go           |  61 ++++++------
 auth/auth_test.go => authz/authz_test.go | 112 +++++++++++------------
 server/config.go                         |  20 +++-
 server/config_internal_test.go           |  26 +++---
 server/server.go                         |  14 +++
 5 files changed, 128 insertions(+), 105 deletions(-)
 rename auth/auth.go => authz/authz.go (62%)
 rename auth/auth_test.go => authz/authz_test.go (61%)

diff --git a/auth/auth.go b/authz/authz.go
similarity index 62%
rename from auth/auth.go
rename to authz/authz.go
index 7376e2551..f48943c04 100644
--- a/auth/auth.go
+++ b/authz/authz.go
@@ -1,11 +1,23 @@
-// Copyright 2021 Molecula Corp. All rights reserved.
-package auth
+// Copyright 2017 Pilosa Corp.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package authz
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
-	"log"
-	"path/filepath"
 
 	"gopkg.in/yaml.v2"
 )
@@ -46,48 +58,39 @@ type Permissions struct {
 	Permission string `yaml:"permission"`
 }
 
-func ReadPermissionsFile(filePath string) (yamlData []byte) {
-	filePathAbs, _ := filepath.Abs(filePath)
-	yamlData, err := ioutil.ReadFile(filePathAbs)
+type Group struct {
+	ID   string `json:"id"`
+	Name string `json:"displayName"`
+}
+
+func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) {
+	permsData, err := ioutil.ReadAll(permsFile)
 	if err != nil {
-		panic(err)
+		return fmt.Errorf("reading permissions failed with error: %s", err)
 	}
-	return yamlData
-}
 
-func (p *GroupPermissions) CreatePermissionsStruct(data []byte) {
-	err := yaml.Unmarshal([]byte(data), p)
+	err = yaml.Unmarshal(permsData, p)
 	if err != nil {
-		log.Fatalf("error %s", err)
+		return fmt.Errorf("unmarshalling permissions failed with error: %s", err)
 	}
+
+	return nil
 }
 
-func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) {
-	// read yaml permissions file
-	yamlData := ReadPermissionsFile(Auth.PermissionsFile)
-
-	// get group permissions
-	var p GroupPermissions
-	p.CreatePermissionsStruct(yamlData)
-
-	// check permissions for all groups and index, and return most permissive
-	return p.ResolvePermissions(groups, index)
-}
-
-func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) {
+func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) {
 
 	// get union of groups the user is part of obtained from identity provider and groups in permissions file
 	var groupMatch []Permissions
 	for _, group := range groups {
 		for i := range p.Permissions {
-			if group["id"] == p.Permissions[i].GroupId {
+			if group.ID == p.Permissions[i].GroupId {
 				groupMatch = append(groupMatch, p.Permissions[i])
 			}
 		}
 	}
 
 	if len(groupMatch) == 0 {
-		return "", fmt.Errorf("user is NOT allowed access to FeatureBase")
+		return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups)
 	}
 
 	// check that user's groups have access to the index user want to access
@@ -110,7 +113,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index
 				indexNotFound = append(indexNotFound, idx)
 			}
 		}
-		return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound)
+		return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound)
 	}
 
 	// check permissions for index user has access to
diff --git a/auth/auth_test.go b/authz/authz_test.go
similarity index 61%
rename from auth/auth_test.go
rename to authz/authz_test.go
index e27463c59..eaec5df13 100644
--- a/auth/auth_test.go
+++ b/authz/authz_test.go
@@ -11,7 +11,7 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package auth_test
+package authz_test
 
 import (
 	"fmt"
@@ -19,23 +19,23 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/molecula/featurebase/v2/auth"
+	"github.com/molecula/featurebase/v2/authz"
 )
 
-func TestAuth_CreatePermissionsStruct(t *testing.T) {
-	var singleInput = []byte(`group_permissions:
+func TestAuth_ReadPermissionsFile(t *testing.T) {
+	var singleInput = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
     index: "test"
-    permission: "read"`)
+    permission: "read"`
 
-	var emptyInput = []byte(`group_permissions:
+	var emptyInput = `group_permissions:
   - group:
     groupId: ""
     index: ""
-    permission: ""`)
+    permission: ""`
 
-	var multiInput = []byte(`group_permissions:
+	var multiInput = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
     index: "test"
@@ -43,28 +43,28 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) {
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
-    permission: "admin"`)
+    permission: "admin"`
 
-	singleStruct := auth.GroupPermissions{
-		Permissions: []auth.Permissions{
+	singleStruct := authz.GroupPermissions{
+		Permissions: []authz.Permissions{
 			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
 		},
 	}
-	emptyStruct := auth.GroupPermissions{
-		Permissions: []auth.Permissions{
+	emptyStruct := authz.GroupPermissions{
+		Permissions: []authz.Permissions{
 			{"", "", ""},
 		},
 	}
-	multiStruct := auth.GroupPermissions{
-		Permissions: []auth.Permissions{
+	multiStruct := authz.GroupPermissions{
+		Permissions: []authz.Permissions{
 			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
 			{"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"},
 		},
 	}
 
 	tests := []struct {
-		input  []byte
-		output auth.GroupPermissions
+		input  string
+		output authz.GroupPermissions
 	}{
 		{singleInput, singleStruct},
 		{emptyInput, emptyStruct},
@@ -73,8 +73,11 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) {
 
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
-			var p auth.GroupPermissions
-			p.CreatePermissionsStruct(test.input)
+			permFile := strings.NewReader(test.input)
+			var p authz.GroupPermissions
+			if err := p.ReadPermissionsFile(permFile); err != nil {
+				t.Fatalf("readPermissionsFile error: %s", err)
+			}
 
 			if !reflect.DeepEqual(p, test.output) {
 				t.Fatalf("expected output %s, but got %s", test.output, p)
@@ -84,26 +87,15 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) {
 	}
 }
 
-func createGroupMaps(groups []string) []map[string]string {
-
-	var group1 []map[string]string
-	for _, i := range groups {
-		map1 := map[string]string{}
-		map1["id"] = i
-		group1 = append(group1, map1)
-	}
-	return group1
-}
-
 func TestAuth_ResolvePermissions(t *testing.T) {
 	// initializes different example of permissions file in yaml
-	var permissions1 = []byte(`group_permissions:
+	var permissions1 = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
     index: "test"
-    permission: "read"`)
+    permission: "read"`
 
-	var permissions2 = []byte(`group_permissions:
+	var permissions2 = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
@@ -111,9 +103,9 @@ func TestAuth_ResolvePermissions(t *testing.T) {
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
-    permission: "write"`)
+    permission: "write"`
 
-	var permissions3 = []byte(`group_permissions:
+	var permissions3 = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
@@ -121,9 +113,9 @@ func TestAuth_ResolvePermissions(t *testing.T) {
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
-    permission: "admin"`)
+    permission: "admin"`
 
-	var permissions4 = []byte(`group_permissions:
+	var permissions4 = `group_permissions:
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
     index: "test"
@@ -131,65 +123,68 @@ func TestAuth_ResolvePermissions(t *testing.T) {
   - group:
     groupId: "dca35310-ecda-4f23-86cd-876aee559900"
     index: "test"
-    permission: "admin"`)
+    permission: "admin"`
 
 	// initializes groups that are returned from identity provider
-	groupsList1 := []string{}
-	groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"}
-	groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"}
+	groupsList1 := []authz.Group{}
+	groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}}
+	groupsList3 := []authz.Group{
+		{"dca35310-ecda-4f23-86cd-876aee55906b", "name"},
+		{"dca35310-ecda-4f23-86cd-876aee559900", "name"},
+	}
 
 	tests := []struct {
-		permissions []byte
-		groups      []map[string]string
-		index       []string
-		userAccess  string
-		err         string
+		yamlData   string
+		groups     []authz.Group
+		index      []string
+		userAccess string
+		err        string
 	}{
 		{
 			permissions1,
-			createGroupMaps(groupsList1),
+			groupsList1,
 			[]string{"test"},
 			"",
-			"user is NOT allowed access to FeatureBase",
+			"NOT allowed access to FeatureBase",
 		},
 		{
 			permissions1,
-			createGroupMaps(groupsList2),
+			groupsList2,
 			[]string{"test1"},
 			"",
-			"user is not allowed access to index",
+			"NOT allowed access to index",
 		},
 		{
 			permissions1,
-			createGroupMaps(groupsList2),
+			groupsList2,
 			[]string{"test"},
 			"read",
 			"",
 		},
 		{
 			permissions2,
-			createGroupMaps(groupsList3),
+			groupsList3,
 			[]string{"test"},
 			"write",
 			"",
 		},
 		{
 			permissions3,
-			createGroupMaps(groupsList3),
+			groupsList3,
 			[]string{"test"},
 			"admin",
 			"",
 		},
 		{
 			permissions2,
-			createGroupMaps(groupsList3),
+			groupsList3,
 			[]string{"test"},
 			"write",
 			"",
 		},
 		{
 			permissions4,
-			createGroupMaps(groupsList2),
+			groupsList2,
 			[]string{"test"},
 			"",
 			"no permissions found",
@@ -199,10 +194,11 @@ func TestAuth_ResolvePermissions(t *testing.T) {
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 
-			var p auth.GroupPermissions
-			p.CreatePermissionsStruct(test.permissions)
+			permFile := strings.NewReader(test.yamlData)
+			var p authz.GroupPermissions
+			p.ReadPermissionsFile(permFile)
 
-			p1, err := p.ResolvePermissions(test.groups, test.index)
+			p1, err := p.GetPermissions(test.groups, test.index)
 
 			if p1 != test.userAccess {
 				t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1)
diff --git a/server/config.go b/server/config.go
index 595697067..1f3b62b4a 100644
--- a/server/config.go
+++ b/server/config.go
@@ -7,13 +7,14 @@ import (
 	"log"
 	"net"
 	"net/url"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
 	"time"
 
-	"github.com/molecula/featurebase/v2/auth"
+	"github.com/molecula/featurebase/v2/authz"
 	petcd "github.com/molecula/featurebase/v2/etcd"
 	rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg"
 	"github.com/molecula/featurebase/v2/storage"
@@ -232,7 +233,7 @@ type Config struct {
 	SchemaDetailsOn bool `toml:"schema-details-on"`
 
 	// Enable AuthZ/AuthN
-	Auth auth.Auth `toml:"auth"`
+	Auth authz.Auth `toml:"auth"`
 }
 
 // Namespace returns the namespace to use based on the Future flag.
@@ -642,13 +643,22 @@ func (c *Config) ValidateAuth() ([]error, error) {
 }
 
 func (c *Config) ValidatePermissions() (err error) {
+	permsFile, err := os.Open(c.Auth.PermissionsFile)
+	if err != nil {
+		return err
+	}
+
+	var p *authz.GroupPermissions
+	if err := p.ReadPermissionsFile(permsFile); err != nil {
+		return err
+	}
 
-	yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile)
-	var p auth.GroupPermissions
-	p.CreatePermissionsStruct(yamlData)
 	if len(p.Permissions) == 0 {
 		return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile)
 	}
+
+	defer permsFile.Close()
+
 	return nil
 }
 
diff --git a/server/config_internal_test.go b/server/config_internal_test.go
index 7f387172e..4d6dcc71c 100644
--- a/server/config_internal_test.go
+++ b/server/config_internal_test.go
@@ -9,7 +9,7 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/molecula/featurebase/v2/auth"
+	"github.com/molecula/featurebase/v2/authz"
 )
 
 type addrs struct{ bind, advertise string }
@@ -294,7 +294,7 @@ func TestConfig_validateAuth(t *testing.T) {
 
 	tests := []struct {
 		expErrs []string
-		input   auth.Auth
+		input   authz.Auth
 	}{
 
 		{
@@ -308,7 +308,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         emptyString,
 				ClientSecret:     emptyString,
@@ -329,7 +329,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     emptyString,
@@ -350,7 +350,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         emptyString,
 				ClientSecret:     validClientSecret,
@@ -370,7 +370,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -389,7 +389,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -407,7 +407,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -423,7 +423,7 @@ func TestConfig_validateAuth(t *testing.T) {
 			[]string{
 				errorMesgURL,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -440,7 +440,7 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgURL,
 				errorMesgURL,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -456,7 +456,7 @@ func TestConfig_validateAuth(t *testing.T) {
 			[]string{
 				errorMesgPermissions,
 			},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -470,7 +470,7 @@ func TestConfig_validateAuth(t *testing.T) {
 		{
 			// Auth enabled, all configs are set properly
 			[]string{},
-			auth.Auth{
+			authz.Auth{
 				Enable:           enable,
 				ClientId:         validClientID,
 				ClientSecret:     validClientSecret,
@@ -484,7 +484,7 @@ func TestConfig_validateAuth(t *testing.T) {
 		{
 			// Auth disabled, all configs are set to empty string
 			[]string{},
-			auth.Auth{
+			authz.Auth{
 				Enable:           disable,
 				ClientId:         emptyString,
 				ClientSecret:     emptyString,
diff --git a/server/server.go b/server/server.go
index a6d0049ae..b876f3eb6 100644
--- a/server/server.go
+++ b/server/server.go
@@ -29,6 +29,7 @@ import (
 	"golang.org/x/sync/errgroup"
 
 	pilosa "github.com/molecula/featurebase/v2"
+	"github.com/molecula/featurebase/v2/authz"
 	"github.com/molecula/featurebase/v2/boltdb"
 	"github.com/molecula/featurebase/v2/encoding/proto"
 	petcd "github.com/molecula/featurebase/v2/etcd"
@@ -224,6 +225,19 @@ func (m *Command) Start() (err error) {
 
 	if m.Config.Auth.Enable {
 		m.Config.MustValidateAuth()
+
+		// Read permissions file
+		permsFile, err := os.Open(m.Config.Auth.PermissionsFile)
+		if err != nil {
+			return err
+		}
+
+		var p authz.GroupPermissions
+		if err := p.ReadPermissionsFile(permsFile); err != nil {
+			return err
+		}
+
+		defer permsFile.Close()
 	}
 
 	// Initialize server.

From 484cbbcd0943bb5bc5d453d9248188904a90a5f2 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Wed, 15 Dec 2021 14:19:12 -0600
Subject: [PATCH 20/51] fixed func name

---
 authz/authz_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/authz/authz_test.go b/authz/authz_test.go
index eaec5df13..b1fbc4d6a 100644
--- a/authz/authz_test.go
+++ b/authz/authz_test.go
@@ -87,7 +87,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) {
 	}
 }
 
-func TestAuth_ResolvePermissions(t *testing.T) {
+func TestAuth_GetPermissions(t *testing.T) {
 	// initializes different example of permissions file in yaml
 	var permissions1 = `group_permissions:
   - group:

From c59ce837b3b15d2baa15c5d3a367475fbaf83a1a Mon Sep 17 00:00:00 2001
From: reesporte <reesedporter@gmail.com>
Date: Wed, 15 Dec 2021 16:09:45 -0600
Subject: [PATCH 21/51] ensure field bit depth is set when restoring shard

we have to manually set the cache value here bc it wont get set until the node is restarted otherwise
---
 api.go | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/api.go b/api.go
index d39a61f3d..70a27da94 100644
--- a/api.go
+++ b/api.go
@@ -2768,6 +2768,14 @@ func (api *API) RestoreShard(ctx context.Context, indexName string, shard uint64
 		if err != nil {
 			return err
 		}
+		bd, err := view.bitDepth([]uint64{shard})
+		if err != nil {
+			return err
+		}
+		err = fld.cacheBitDepth(bd)
+		if err != nil {
+			return err
+		}
 	}
 
 	return nil

From 2ca29e6018621b203462296f72be3675e7405ffb Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Fri, 17 Dec 2021 11:45:35 -0600
Subject: [PATCH 22/51] addressed reviewer's comments and added more tests

---
 authz/authz.go                 |  77 ++++-------
 authz/authz_test.go            | 157 +++++++++-------------
 server/config.go               | 118 +++++++++++-----
 server/config_internal_test.go | 238 +++++++++++++--------------------
 server/server.go               |   7 +-
 5 files changed, 268 insertions(+), 329 deletions(-)

diff --git a/authz/authz.go b/authz/authz.go
index f48943c04..42c3bba2f 100644
--- a/authz/authz.go
+++ b/authz/authz.go
@@ -49,27 +49,23 @@ type Auth struct {
 }
 
 type GroupPermissions struct {
-	Permissions []Permissions `yaml:"group_permissions"`
-}
-
-type Permissions struct {
-	GroupId    string `yaml:"groupId"`
-	Index      string `yaml:"index"`
-	Permission string `yaml:"permission"`
+	Permissions map[string]map[string]string
 }
 
 type Group struct {
-	ID   string `json:"id"`
-	Name string `json:"displayName"`
+	UserID    string
+	GroupID   string `json:"id"`
+	GroupName string `json:"displayName"`
 }
 
 func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) {
 	permsData, err := ioutil.ReadAll(permsFile)
+
 	if err != nil {
 		return fmt.Errorf("reading permissions failed with error: %s", err)
 	}
 
-	err = yaml.Unmarshal(permsData, p)
+	err = yaml.UnmarshalStrict(permsData, &p.Permissions)
 	if err != nil {
 		return fmt.Errorf("unmarshalling permissions failed with error: %s", err)
 	}
@@ -77,54 +73,33 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error)
 	return nil
 }
 
-func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) {
+func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) {
 
-	// get union of groups the user is part of obtained from identity provider and groups in permissions file
-	var groupMatch []Permissions
-	for _, group := range groups {
-		for i := range p.Permissions {
-			if group.ID == p.Permissions[i].GroupId {
-				groupMatch = append(groupMatch, p.Permissions[i])
-			}
-		}
-	}
-
-	if len(groupMatch) == 0 {
-		return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups)
-	}
-
-	// check that user's groups have access to the index user want to access
-	var indexMatch []Permissions
-	indexCheck := map[string]bool{}
-	for _, g := range groupMatch {
-		for _, idx := range index {
-			if idx == g.Index {
-				indexMatch = append(indexMatch, g)
-				indexCheck[idx] = true
-			}
-		}
-	}
-
-	// check which index user does NOT have access to, and return in error mesg
-	if len(indexCheck) != len(index) {
-		var indexNotFound []string
-		for _, idx := range index {
-			if !indexCheck[idx] {
-				indexNotFound = append(indexNotFound, idx)
-			}
-		}
-		return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound)
-	}
-
-	// check permissions for index user has access to
 	allPermissions := map[string]bool{
 		"admin": false,
 		"write": false,
 		"read":  false,
 	}
 
-	for _, g := range indexMatch {
-		allPermissions[g.Permission] = true
+	if len(groups) == 0 {
+		return "", fmt.Errorf("user is not part of any groups in identity provider")
+	}
+
+	var groupsDenied []string
+	for _, group := range groups {
+		if _, ok := p.Permissions[group.GroupID]; ok {
+			if perm, ok := p.Permissions[group.GroupID][index]; ok {
+				allPermissions[perm] = true
+			} else {
+				return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index)
+			}
+		} else {
+			groupsDenied = append(groupsDenied, group.GroupID)
+		}
+	}
+
+	if len(groupsDenied) == len(groups) {
+		return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied)
 	}
 
 	if allPermissions["admin"] {
diff --git a/authz/authz_test.go b/authz/authz_test.go
index b1fbc4d6a..fcec43185 100644
--- a/authz/authz_test.go
+++ b/authz/authz_test.go
@@ -23,64 +23,43 @@ import (
 )
 
 func TestAuth_ReadPermissionsFile(t *testing.T) {
-	var singleInput = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
-    index: "test"
-    permission: "read"`
+	singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b":
+  "test": "read"`
 
-	var emptyInput = `group_permissions:
-  - group:
-    groupId: ""
-    index: ""
-    permission: ""`
+	multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b":
+  "test": "read"
+"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": "admin"`
 
-	var multiInput = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
-    index: "test"
-    permission: "read"
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "admin"`
-
-	singleStruct := authz.GroupPermissions{
-		Permissions: []authz.Permissions{
-			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
-		},
+	singleStruct := map[string]map[string]string{
+		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
 	}
-	emptyStruct := authz.GroupPermissions{
-		Permissions: []authz.Permissions{
-			{"", "", ""},
-		},
-	}
-	multiStruct := authz.GroupPermissions{
-		Permissions: []authz.Permissions{
-			{"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"},
-			{"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"},
-		},
+
+	multiStruct := map[string]map[string]string{
+		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
+		"dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"},
 	}
 
 	tests := []struct {
 		input  string
-		output authz.GroupPermissions
+		output map[string]map[string]string
 	}{
 		{singleInput, singleStruct},
-		{emptyInput, emptyStruct},
 		{multiInput, multiStruct},
 	}
 
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 			permFile := strings.NewReader(test.input)
+
 			var p authz.GroupPermissions
-			if err := p.ReadPermissionsFile(permFile); err != nil {
+			err := p.ReadPermissionsFile(permFile)
+			if err != nil {
 				t.Fatalf("readPermissionsFile error: %s", err)
 			}
 
-			if !reflect.DeepEqual(p, test.output) {
-				t.Fatalf("expected output %s, but got %s", test.output, p)
+			if !reflect.DeepEqual(p.Permissions, test.output) {
+				t.Fatalf("expected output %s, but got %s", test.output, p.Permissions)
 			}
 		},
 		)
@@ -89,103 +68,84 @@ func TestAuth_ReadPermissionsFile(t *testing.T) {
 
 func TestAuth_GetPermissions(t *testing.T) {
 	// initializes different example of permissions file in yaml
-	var permissions1 = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
-    index: "test"
-    permission: "read"`
+	permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b":
+  "test": "read"`
 
-	var permissions2 = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "read"
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "write"`
+	permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": "write"`
 
-	var permissions3 = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "read"
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "admin"`
+	permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b":
+  "test": "write"
+  "test2": "read"
+"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": "admin"`
 
-	var permissions4 = `group_permissions:
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee55906b"
-    index: "test"
-    permission: ""
-  - group:
-    groupId: "dca35310-ecda-4f23-86cd-876aee559900"
-    index: "test"
-    permission: "admin"`
+	permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": ""`
 
 	// initializes groups that are returned from identity provider
+	groupName := "name"
+	userId := "user-id"
 	groupsList1 := []authz.Group{}
-	groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}}
+	groupsList2 := []authz.Group{{userId, "fake-group", groupName}}
 	groupsList3 := []authz.Group{
-		{"dca35310-ecda-4f23-86cd-876aee55906b", "name"},
-		{"dca35310-ecda-4f23-86cd-876aee559900", "name"},
+		{userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName},
+		{userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName},
 	}
 
 	tests := []struct {
 		yamlData   string
 		groups     []authz.Group
-		index      []string
+		index      string
 		userAccess string
 		err        string
 	}{
 		{
 			permissions1,
 			groupsList1,
-			[]string{"test"},
+			"test",
+			"",
+			"user is not part of any groups in identity provider",
+		},
+		{
+			permissions1,
+			groupsList3,
+			"test1",
+			"",
+			"NOT allowed access to index",
+		},
+		{
+			permissions2,
+			groupsList2,
+			"test",
 			"",
 			"NOT allowed access to FeatureBase",
 		},
 		{
 			permissions1,
-			groupsList2,
-			[]string{"test1"},
-			"",
-			"NOT allowed access to index",
-		},
-		{
-			permissions1,
-			groupsList2,
-			[]string{"test"},
+			groupsList3,
+			"test",
 			"read",
 			"",
 		},
 		{
 			permissions2,
 			groupsList3,
-			[]string{"test"},
+			"test",
 			"write",
 			"",
 		},
 		{
 			permissions3,
 			groupsList3,
-			[]string{"test"},
+			"test",
 			"admin",
 			"",
 		},
-		{
-			permissions2,
-			groupsList3,
-			[]string{"test"},
-			"write",
-			"",
-		},
 		{
 			permissions4,
-			groupsList2,
-			[]string{"test"},
+			groupsList3,
+			"test",
 			"",
 			"no permissions found",
 		},
@@ -195,8 +155,11 @@ func TestAuth_GetPermissions(t *testing.T) {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 
 			permFile := strings.NewReader(test.yamlData)
+
 			var p authz.GroupPermissions
-			p.ReadPermissionsFile(permFile)
+			if err := p.ReadPermissionsFile(permFile); err != nil {
+				t.Errorf("Error: %s", err)
+			}
 
 			p1, err := p.GetPermissions(test.groups, test.index)
 
diff --git a/server/config.go b/server/config.go
index 1f3b62b4a..866ffb3ff 100644
--- a/server/config.go
+++ b/server/config.go
@@ -4,6 +4,7 @@ package server
 import (
 	"context"
 	"fmt"
+	"io"
 	"log"
 	"net"
 	"net/url"
@@ -598,9 +599,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin
 	return addrs[0].String(), nil
 }
 
-func (c *Config) ValidateAuth() ([]error, error) {
+func (c *Config) ValidateAuth() (errors []error) {
 	if !c.Auth.Enable {
-		return []error{}, nil
+		return errors
 	}
 	authConfig := map[string]string{
 		"ClientId":         c.Auth.ClientId,
@@ -609,10 +610,8 @@ func (c *Config) ValidateAuth() ([]error, error) {
 		"TokenURL":         c.Auth.TokenURL,
 		"GroupEndpointURL": c.Auth.GroupEndpointURL,
 		"ScopeURL":         c.Auth.ScopeURL,
-		"PermissionsFile":  c.Auth.PermissionsFile,
 	}
 
-	errors := make([]error, 0)
 	for name, value := range authConfig {
 		if value == "" {
 			errors = append(errors, fmt.Errorf("empty string for auth config %s", name))
@@ -626,50 +625,99 @@ func (c *Config) ValidateAuth() ([]error, error) {
 				continue
 			}
 		}
+	}
 
-		if strings.Contains(name, "File") {
-			fileExt := filepath.Ext(value)
-			if (fileExt != ".yaml") && (fileExt != ".yml") {
-				errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value))
+	if len(errors) > 0 {
+		return errors
+	}
+	return nil
+}
+
+func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) {
+
+	var p authz.GroupPermissions
+	if err := p.ReadPermissionsFile(permsFile); err != nil {
+		return append(errors, err)
+	}
+
+	if len(p.Permissions) == 0 {
+		return append(errors, fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile))
+	}
+
+	for groupId, indexPerm := range p.Permissions {
+		if groupId == "" {
+			errors = append(errors, fmt.Errorf("empty string for group id in permissions file %s", c.Auth.PermissionsFile))
+			continue
+		}
+
+		for index, perm := range indexPerm {
+			if index == "" {
+				errors = append(errors, fmt.Errorf("empty string for index for group id %s in permissions file %s ", groupId, c.Auth.PermissionsFile))
+				continue
+			}
+
+			if perm == "" {
+				errors = append(errors, fmt.Errorf("empty string for permission for group id %s and index %s in permissions file %s", groupId, index, c.Auth.PermissionsFile))
+				continue
+			}
+
+			if !((perm == "admin") || (perm == "write") || (perm == "read")) {
+				errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile))
 				continue
 			}
 		}
 	}
-
 	if len(errors) > 0 {
-		return errors, fmt.Errorf("there were errors validating config")
+		return errors
 	}
-	return errors, nil
-}
-
-func (c *Config) ValidatePermissions() (err error) {
-	permsFile, err := os.Open(c.Auth.PermissionsFile)
-	if err != nil {
-		return err
-	}
-
-	var p *authz.GroupPermissions
-	if err := p.ReadPermissionsFile(permsFile); err != nil {
-		return err
-	}
-
-	if len(p.Permissions) == 0 {
-		return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile)
-	}
-
-	defer permsFile.Close()
 
 	return nil
 }
 
+func (c *Config) ValidatePermissionsFile() (err error) {
+
+	if c.Auth.PermissionsFile == "" {
+		return fmt.Errorf("empty string for auth config permissions file")
+	}
+
+	fileExt := filepath.Ext(c.Auth.PermissionsFile)
+	if (fileExt != ".yaml") && (fileExt != ".yml") {
+		return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile)
+	}
+	return nil
+}
+
 func (c *Config) MustValidateAuth() {
-	if errors, err := c.ValidateAuth(); err != nil {
-		for _, e1 := range errors {
-			log.Println(e1)
+
+	errorsAuth := c.ValidateAuth()
+	if len(errorsAuth) > 0 {
+		for _, e := range errorsAuth {
+			log.Println(e)
 		}
-		if e2 := c.ValidatePermissions(); e2 != nil {
-			log.Println(e2)
+	}
+
+	var errorsPerm []error
+	errorsPermFile := c.ValidatePermissionsFile()
+	if errorsPermFile == nil {
+		permsFile, err := os.Open(c.Auth.PermissionsFile)
+		if err != nil {
+			log.Println(err)
 		}
-		log.Fatal(err)
+
+		defer permsFile.Close()
+
+		errorsPerm = c.ValidatePermissions(permsFile)
+		if len(errorsPerm) > 0 {
+			for _, e := range errorsPerm {
+				log.Println(e)
+			}
+		}
+
+	} else {
+		log.Println(errorsPermFile)
+	}
+
+	if len(errorsAuth) > 0 || len(errorsPerm) > 0 || errorsPermFile != nil {
+		log.Fatal(fmt.Errorf("there were errors validating authN/authZ config and/or permissions"))
 	}
 }
diff --git a/server/config_internal_test.go b/server/config_internal_test.go
index 4d6dcc71c..48ba081db 100644
--- a/server/config_internal_test.go
+++ b/server/config_internal_test.go
@@ -281,12 +281,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) {
 func TestConfig_validateAuth(t *testing.T) {
 	errorMesgEmpty := "empty string"
 	errorMesgURL := "invalid URL"
-	errorMesgPermissions := "invalid file extension"
 	validTestURL := "https://url.com/"
 	validClientID := "clientid"
 	validClientSecret := "clientSecret"
-	validFilename := "permissions.yaml"
-	invalidFilename := "permissions.txt"
 	invalidURL := "not-a-url"
 	emptyString := ""
 	enable := true
@@ -306,7 +303,6 @@ func TestConfig_validateAuth(t *testing.T) {
 				errorMesgEmpty,
 				errorMesgEmpty,
 				errorMesgEmpty,
-				errorMesgEmpty,
 			},
 			authz.Auth{
 				Enable:           enable,
@@ -316,106 +312,6 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
-			},
-		},
-		{
-			// Auth enabled, some configs are set to empty string
-			[]string{
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     emptyString,
-				AuthorizeURL:     emptyString,
-				TokenURL:         emptyString,
-				GroupEndpointURL: emptyString,
-				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
-			},
-		},
-		{
-			// Auth enabled, some configs are set to empty string
-			[]string{
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         emptyString,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     emptyString,
-				TokenURL:         emptyString,
-				GroupEndpointURL: emptyString,
-				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
-			},
-		},
-		{
-			// Auth enabled, some configs are set to empty string
-			[]string{
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     emptyString,
-				TokenURL:         emptyString,
-				GroupEndpointURL: emptyString,
-				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
-			},
-		},
-		{
-			// Auth enabled, some configs are set to empty string
-			[]string{
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     validTestURL,
-				TokenURL:         emptyString,
-				GroupEndpointURL: emptyString,
-				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
-			},
-		},
-		{
-			// Auth enabled, some configs are set to empty string
-			[]string{
-				errorMesgEmpty,
-				errorMesgEmpty,
-				errorMesgEmpty,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     validTestURL,
-				TokenURL:         validTestURL,
-				GroupEndpointURL: emptyString,
-				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
 			},
 		},
 		{
@@ -431,40 +327,6 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         validTestURL,
 				GroupEndpointURL: validTestURL,
 				ScopeURL:         validTestURL,
-				PermissionsFile:  validFilename,
-			},
-		},
-		{
-			// Auth enabled, some strings are set to invalid URL
-			[]string{
-				errorMesgURL,
-				errorMesgURL,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     validTestURL,
-				TokenURL:         invalidURL,
-				GroupEndpointURL: invalidURL,
-				ScopeURL:         validTestURL,
-				PermissionsFile:  validFilename,
-			},
-		},
-		{
-			// Auth enabled, permissions file is set to invalid string
-			[]string{
-				errorMesgPermissions,
-			},
-			authz.Auth{
-				Enable:           enable,
-				ClientId:         validClientID,
-				ClientSecret:     validClientSecret,
-				AuthorizeURL:     validTestURL,
-				TokenURL:         validTestURL,
-				GroupEndpointURL: validTestURL,
-				ScopeURL:         validTestURL,
-				PermissionsFile:  invalidFilename,
 			},
 		},
 		{
@@ -478,7 +340,6 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         validTestURL,
 				GroupEndpointURL: validTestURL,
 				ScopeURL:         validTestURL,
-				PermissionsFile:  validFilename,
 			},
 		},
 		{
@@ -492,7 +353,6 @@ func TestConfig_validateAuth(t *testing.T) {
 				TokenURL:         emptyString,
 				GroupEndpointURL: emptyString,
 				ScopeURL:         emptyString,
-				PermissionsFile:  emptyString,
 			},
 		},
 	}
@@ -502,9 +362,9 @@ func TestConfig_validateAuth(t *testing.T) {
 			c := NewConfig()
 			c.Auth = test.input
 
-			errors, err := c.ValidateAuth()
+			errors := c.ValidateAuth()
 			if len(test.expErrs) > 0 {
-				if err == nil {
+				if errors == nil {
 					t.Fatal("expected errors, but none were found")
 				}
 			}
@@ -522,3 +382,97 @@ func TestConfig_validateAuth(t *testing.T) {
 		})
 	}
 }
+
+func TestConfig_validatePermissions(t *testing.T) {
+	permissions0 := ``
+
+	permissions1 := `"":
+  "test": "read"`
+
+	permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900":
+  "": "write"`
+
+	permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": ""`
+
+	permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900":
+  "test": "readwrite"`
+
+	tests := []struct {
+		err   string
+		input string
+	}{
+		{
+			"no group permissions found in permissions file",
+			permissions0,
+		},
+		{
+			"empty string for group id",
+			permissions1,
+		},
+		{
+			"empty string for index",
+			permissions2,
+		},
+		{
+			"empty string for permission",
+			permissions3,
+		},
+		{
+			"not a valid permission",
+			permissions4,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+
+			c := NewConfig()
+			c.Auth.PermissionsFile = "test.yaml"
+
+			permFile := strings.NewReader(test.input)
+			errors := c.ValidatePermissions(permFile)
+
+			if errors == nil {
+				t.Fatal("expected errors, but none were found")
+			}
+
+			for _, err := range errors {
+				if !strings.Contains(err.Error(), test.err) {
+					t.Errorf("expected error to contain %s, but got %s", test.err, err.Error())
+
+				}
+			}
+		})
+	}
+}
+
+func TestConfig_validatePermissionsFilename(t *testing.T) {
+
+	tests := []struct {
+		err   string
+		input string
+	}{
+		{
+			"empty string for auth config permissions file",
+			"",
+		},
+		{
+			"invalid file extension for auth config permissions file",
+			"permissions.txt",
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			c := NewConfig()
+			c.Auth.PermissionsFile = test.input
+
+			if err := c.ValidatePermissionsFile(); err != nil {
+				if !strings.Contains(err.Error(), test.err) {
+					t.Errorf("expected error to contain %s, but got %s", test.err, err.Error())
+				}
+			}
+		})
+	}
+}
diff --git a/server/server.go b/server/server.go
index b876f3eb6..c2418630b 100644
--- a/server/server.go
+++ b/server/server.go
@@ -226,18 +226,17 @@ func (m *Command) Start() (err error) {
 	if m.Config.Auth.Enable {
 		m.Config.MustValidateAuth()
 
-		// Read permissions file
 		permsFile, err := os.Open(m.Config.Auth.PermissionsFile)
 		if err != nil {
 			return err
 		}
 
+		defer permsFile.Close()
+
 		var p authz.GroupPermissions
-		if err := p.ReadPermissionsFile(permsFile); err != nil {
+		if err = p.ReadPermissionsFile(permsFile); err != nil {
 			return err
 		}
-
-		defer permsFile.Close()
 	}
 
 	// Initialize server.

From 3ed4487ae269be42307ad19fa217a751b38177d4 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 12:31:13 -0600
Subject: [PATCH 23/51] scratch space for FB-992: create benchmark for
 checkpointing

Note also the commented-out debug printf in checkpoint, there as a
reference. This is interesting because it turns out that MOST of checkpoint
writes is not actually writing new pages in most cases.

The actual "pages in WAL : pages in map" ratio is typically around 30:1
apparently. This would likely be different in cases where we were
updating existing data, though.

This is scratch space to prep for an actual work. The final
results will likely be different.
---
 rbf/db.go      |  1 +
 rbf/db_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/rbf/db.go b/rbf/db.go
index 99c4f0e2f..65794c9eb 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -200,6 +200,7 @@ func (db *DB) checkpoint() error {
 		return nil
 	}
 
+	// fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size)
 	for i := 0; i < db.walPageN; i++ {
 		page, err := db.readWALPageAt(i)
 		if err != nil {
diff --git a/rbf/db_test.go b/rbf/db_test.go
index 56170d6eb..614def465 100644
--- a/rbf/db_test.go
+++ b/rbf/db_test.go
@@ -13,6 +13,7 @@ import (
 
 	_ "net/http/pprof"
 
+	"github.com/felixge/fgprof"
 	"github.com/molecula/featurebase/v2/rbf"
 	rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg"
 	"golang.org/x/sync/errgroup"
@@ -336,6 +337,91 @@ func TestDB_MultiTx(t *testing.T) {
 	}
 }
 
+// benchmarkOneCheckpoint
+func benchmarkOneCheckpoint(b *testing.B) {
+	cfg := rbfcfg.NewDefaultConfig()
+	// extremely low to force checkpointing
+	cfg.MinWALCheckpointSize = rbf.PageSize * 16
+	cfg.MaxWALCheckpointSize = rbf.PageSize * 64
+	var _ rbfcfg.Config
+	db := MustOpenDB(b, cfg)
+	defer MustCloseDB(b, db)
+
+	// Run multiple readers in separate goroutines.
+	ctx, cancel := context.WithCancel(context.Background())
+	g, ctx := errgroup.WithContext(ctx)
+	for i := 0; i < 4; i++ {
+		g.Go(func() error {
+			for {
+				if ctx.Err() != nil {
+					return nil // cancelled, return no error
+				} else if err := func() error {
+					tx, err := db.Begin(false)
+					if err != nil {
+						return err
+					}
+					defer tx.Rollback()
+
+					// time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond))))
+
+					for i := 0; i < rand.Intn(1000); i++ {
+						v := rand.Intn(1 << 20)
+						if _, err := tx.Contains("x", uint64(v)); err != nil {
+							return err
+						}
+					}
+					return nil
+				}(); err != nil {
+					return err
+				}
+
+				// time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond))))
+			}
+		})
+	}
+
+	// Continuously set/clear bits while readers are executing.
+	for i := 0; i < 1000; i++ {
+		func() {
+			tx, err := db.Begin(true)
+			if err != nil {
+				b.Fatal(err)
+			}
+			defer tx.Rollback()
+
+			for j := 0; j < rand.Intn(100); j++ {
+				v := rand.Intn(1 << 20)
+				if _, err := tx.Add("x", uint64(v)); err != nil {
+					b.Fatal(err)
+				}
+
+			}
+
+			if err := tx.Commit(); err != nil {
+				b.Fatal(err)
+			}
+		}()
+	}
+
+	// Stop readers & wait.
+	cancel()
+	if err := g.Wait(); err != nil {
+		b.Fatal(err)
+	}
+}
+
+func BenchmarkDbCheckpoint(b *testing.B) {
+	out, err := os.Create("cp.out")
+	if err != nil {
+		b.Fatalf("creating log file: %v", err)
+	}
+	done := fgprof.Start(out, fgprof.FormatPprof)
+	for i := 0; i < b.N; i++ {
+		benchmarkOneCheckpoint(b)
+	}
+	done()
+}
+
 // better diagnosis of deadlocks/hung situations versus just really slow "Quick" tests.
 func TestMain(m *testing.M) {
 	l, err := net.Listen("tcp", ":0")

From 5c889c72bdab24f960d69470adc99301f558fa96 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 13:04:45 -0600
Subject: [PATCH 24/51] make test hit the lock harder

Discovered test was running slightly strange and spending an unreasonable
amount of time on rand.Intn(), possibly because we weren't caching the
value used as the loop condition. Tweaked that, also made the pool a
bit different. Now it takes ~50 seconds for benchtime 100x, and produces
a profile with a TON of time spent waiting on sleeps (expected) and
the condition variable for waiting on checkpoints (the thing we want
to measure, really).
---
 rbf/db_test.go | 54 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/rbf/db_test.go b/rbf/db_test.go
index 614def465..5c8504b1b 100644
--- a/rbf/db_test.go
+++ b/rbf/db_test.go
@@ -337,8 +337,11 @@ func TestDB_MultiTx(t *testing.T) {
 	}
 }
 
+// premake pool of random values
+const randPool = (1 << 18)
+
 // benchmarkOneCheckpoint
-func benchmarkOneCheckpoint(b *testing.B) {
+func benchmarkOneCheckpoint(b *testing.B, randInts []int) {
 	cfg := rbfcfg.NewDefaultConfig()
 	// extremely low to force checkpointing
 	cfg.MinWALCheckpointSize = rbf.PageSize * 16
@@ -350,7 +353,8 @@ func benchmarkOneCheckpoint(b *testing.B) {
 	// Run multiple readers in separate goroutines.
 	ctx, cancel := context.WithCancel(context.Background())
 	g, ctx := errgroup.WithContext(ctx)
-	for i := 0; i < 4; i++ {
+	for i := 0; i < 8; i++ {
+		i := i
 		g.Go(func() error {
 			for {
 				if ctx.Err() != nil {
@@ -362,10 +366,11 @@ func benchmarkOneCheckpoint(b *testing.B) {
 					}
 					defer tx.Rollback()
 
-					// time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond))))
+					time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond))))
 
-					for i := 0; i < rand.Intn(1000); i++ {
-						v := rand.Intn(1 << 20)
+					times := rand.Intn(1000) + 1
+					for j := 0; j < times; j++ {
+						v := randInts[((i<<10)+j)%(randPool-1)]
 						if _, err := tx.Contains("x", uint64(v)); err != nil {
 							return err
 						}
@@ -374,13 +379,13 @@ func benchmarkOneCheckpoint(b *testing.B) {
 				}(); err != nil {
 					return err
 				}
-
 				// time.Sleep(time.Duration(rand.Intn(int(3 * time.Millisecond))))
 			}
 		})
 	}
 
 	// Continuously set/clear bits while readers are executing.
+	next := 0
 	for i := 0; i < 1000; i++ {
 		func() {
 			tx, err := db.Begin(true)
@@ -389,14 +394,22 @@ func benchmarkOneCheckpoint(b *testing.B) {
 			}
 			defer tx.Rollback()
 
-			for j := 0; j < rand.Intn(100); j++ {
-				v := rand.Intn(1 << 20)
-				if _, err := tx.Add("x", uint64(v)); err != nil {
-					b.Fatal(err)
+			times := rand.Intn(100)
+			for j := 0; j < times; j++ {
+				v := randInts[next]
+				next = (next + 1) % (randPool - 1)
+				if j&7 == 0 {
+					// some removes but they're less frequent
+					if _, err := tx.Remove("x", uint64(v)); err != nil {
+						b.Fatal(err)
+					}
+				} else {
+					if _, err := tx.Add("x", uint64(v)); err != nil {
+						b.Fatal(err)
+					}
 				}
 
 			}
-
 			if err := tx.Commit(); err != nil {
 				b.Fatal(err)
 			}
@@ -416,9 +429,24 @@ func BenchmarkDbCheckpoint(b *testing.B) {
 		b.Fatalf("creating log file: %v", err)
 	}
 	done := fgprof.Start(out, fgprof.FormatPprof)
-	for i := 0; i < b.N; i++ {
-		benchmarkOneCheckpoint(b)
+	b.StopTimer()
+	// premake these because otherwise it's >5% of CPU in the reads
+	randInts := make([]int, randPool)
+	for i := range randInts {
+		v1, v2 := rand.Intn(1<<24), rand.Intn(1<<24)
+		// minimum gives us a skewed distribution which makes lower values more
+		// likely than higher values, so we get a mix of container types
+		if v1 < v2 {
+			randInts[i] = v1
+		} else {
+			randInts[i] = v2
+		}
 	}
+	b.StartTimer()
+	for i := 0; i < b.N; i++ {
+		benchmarkOneCheckpoint(b, randInts)
+	}
+	b.StopTimer()
 	done()
 }
 

From a631e25dc517e5473ea70863bae8633e0232ce00 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 13:14:42 -0600
Subject: [PATCH 25/51] refactor: removeTx responsible for getting/releasing
 its own lock

We change nothing substantive here, except that there's a window
between when a write transaction updates the root pages and when
it removes itself from the db tx list and possibly causes a checkpoint
where it's not holding the db lock.

The issue here is that we want to be able to *keep* the lock but
still return, so no one else can start transactions, but the specific
Rollback or Commit that removed the last outstanding transaction
doesn't block forever. This will, later, allow us to exercise
finer-grained control over when we allow transactions. This is
a separate commit so we can run the test suite against it, and
verify that this part in particular didn't break anything.
---
 rbf/db.go |  7 ++++++-
 rbf/tx.go | 14 +++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 65794c9eb..7f27ca418 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -503,8 +503,13 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	return tx, nil
 }
 
-// removeTx removes an active transaction from the database.
+// removeTx removes an active transaction from the database. it obtains
+// the db lock, and currently drops it, but will later possibly be leaving
+// it retained by an asynchronous op that wants to happen before we start
+// running new tx.
 func (db *DB) removeTx(tx *Tx) error {
+	db.mu.Lock()
+	defer db.mu.Unlock()
 	// Release writer lock if tx is writable.
 	if tx.writable {
 		tx.db.rwmu.Unlock()
diff --git a/rbf/tx.go b/rbf/tx.go
index 38fed48f6..2cf7420db 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -109,20 +109,24 @@ func (tx *Tx) Commit() error {
 		// future plan: after checkpoint is moved to background
 		// or not every removeTx, then we can move the
 		// tx.db.rootRecords = tx.rootRecords into removeTx().
-
+		//
+		// ... or maybe not: let's do that part here, and then removeTx
+		// may or may not start a checkpoint, possibly asynchronously.
+		//
 		// avoid race detector firing on a write race here
-		// vs the read of rootRecords at db.Begin()
+		// vs the read of rootRecords at db.Begin(), then release
+		// the lock, because we need removeTx to grab the lock to
+		// work, but if it wants to checkpoint, it wants to be able to return
+		// to us here and still be holding the lock.
 		tx.db.mu.Lock()
-		defer tx.db.mu.Unlock()
 		tx.db.rootRecords = tx.rootRecords
 		tx.db.pageMap = tx.pageMap
 		tx.db.walPageN = tx.walPageN
+		tx.db.mu.Unlock()
 		return tx.db.removeTx(tx)
 	}
 
 	// Disconnect transaction from DB.
-	tx.db.mu.Lock()
-	defer tx.db.mu.Unlock()
 	return tx.db.removeTx(tx)
 }
 

From b8f59d922c72e1e1b8d4b1ee36bbd89e6e0d9096 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 14:37:59 -0600
Subject: [PATCH 26/51] checkpoint rework/refactoring: logger, async-ish
 checkpoint

Trying to make the checkpoint be asynchronous-at-all, and
also allowing it to log.
---
 rbf/cfg/cfg.go  |   6 ++
 rbf/db.go       | 162 ++++++++++++++++++++++++++++++++++++++----------
 rbf/rbf_test.go |   8 +++
 3 files changed, 144 insertions(+), 32 deletions(-)

diff --git a/rbf/cfg/cfg.go b/rbf/cfg/cfg.go
index cc2cf7a8a..671c6fe43 100644
--- a/rbf/cfg/cfg.go
+++ b/rbf/cfg/cfg.go
@@ -2,6 +2,7 @@
 package cfg
 
 import (
+	"github.com/molecula/featurebase/v2/logger"
 	"github.com/spf13/pflag"
 )
 
@@ -35,6 +36,11 @@ type Config struct {
 	// CursorCacheSize is the number of copies of Cursor{} to keep in our
 	// readyCursorCh arena to avoid GC pressure.
 	CursorCacheSize int64 `toml:"cursor-cache-size"`
+
+	// Logger specifies a logger for asynchronous errors, such as
+	// background checkpoints. It cannot be set from toml. The default is
+	// to use stderr.
+	Logger logger.Logger `toml:"-"`
 }
 
 func NewDefaultConfig() *Config {
diff --git a/rbf/db.go b/rbf/db.go
index 7f27ca418..830e31f5a 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -11,6 +11,7 @@ import (
 	"syscall"
 
 	"github.com/benbjohnson/immutable"
+	"github.com/molecula/featurebase/v2/logger"
 	rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg"
 	"github.com/molecula/featurebase/v2/syswrap"
 )
@@ -38,6 +39,7 @@ type DB struct {
 	pageMap     *PageMap             // pgno-to-WALID mapping
 	txs         map[*Tx]struct{}     // active transactions
 	opened      bool                 // true if open
+	logger      logger.Logger        // for diagnostics from async things
 
 	wal      []byte   // wal mmap
 	walFile  *os.File // wal file descriptor
@@ -62,6 +64,11 @@ func NewDB(path string, cfg *rbfcfg.Config) *DB {
 		txs:     make(map[*Tx]struct{}),
 		pageMap: NewPageMap(),
 		Path:    path,
+		logger:  cfg.Logger,
+	}
+	if db.logger == nil {
+		// default to writing to stdout if not told otherwise
+		db.logger = logger.NewStandardLogger(os.Stderr)
 	}
 	db.haltCond = sync.NewCond(&db.mu)
 
@@ -134,7 +141,7 @@ func (db *DB) Open() (err error) {
 	if err := db.openWAL(); err != nil {
 		return fmt.Errorf("wal open: %w", err)
 	} else if err := db.checkpoint(); err != nil {
-		return fmt.Errorf("checkpoint: %w", err)
+		return fmt.Errorf("startup checkpoint: %w", err)
 	}
 
 	return nil
@@ -158,10 +165,12 @@ func (db *DB) openWAL() (err error) {
 
 	// Determine the number of whole pages in the WAL.
 	var pageN int
+	var fileSize int64
 	if fi, err := db.walFile.Stat(); err != nil {
 		return fmt.Errorf("wal stat: %w", err)
 	} else {
-		pageN = int(fi.Size() / PageSize)
+		fileSize = fi.Size()
+		pageN = int(fileSize / PageSize)
 	}
 
 	// Read backwards through the WAL to find the last valid meta page.
@@ -169,14 +178,45 @@ func (db *DB) openWAL() (err error) {
 		if page, err := db.readWALPageAt(pageN - 1); err != nil {
 			return err
 		} else if IsMetaPage(page) {
+			// We now face a challenge. Probably this is a meta page.
+			// But consider a sequence of pages written which gets
+			// interrupted right before the meta page is written.
+			// If the last page is a bitmap page, it could LOOK LIKE a meta
+			// page. So we have to check the page before it. If that page
+			// is a bitmap header, then actually this is a bitmap page, right?
+			// If that page doesn't exist, of course, we're fine, except
+			// for the philosophical question of why we wrote a meta page
+			// when no pages had changed.
+			if pageN > 1 {
+				if page, err = db.readWALPageAt(pageN - 2); err != nil {
+					return err
+				}
+				if IsBitmapHeader(page) {
+					// But wait!
+					// What if this *is* a meta page, and the page before it is
+					// actually a *bitmap page* that looks like a bitmap header? And
+					// so on.
+					//
+					// Rather than try to resolve this, in this insanely unlikely
+					// situation, we read from the beginning which allows us to
+					// always know what we're seeing, because every bitmap page
+					// comes *after* a bitmap header page, and thus, we know when
+					// we might be seeing one.
+					pageN, err = db.methodicalWALPageN(pageN)
+					if err != nil {
+						return err
+					}
+				}
+			}
 			break
 		}
 	}
-
-	// Truncate WAL to the last valid meta page.
-	if err := db.walFile.Truncate(int64(pageN * PageSize)); err != nil {
-		return fmt.Errorf("wal truncate: %w", err)
-	} else if _, err := db.walFile.Seek(int64(pageN*PageSize), io.SeekStart); err != nil {
+	if fileSize != int64(pageN*PageSize) {
+		if err := db.walFile.Truncate(int64(pageN * PageSize)); err != nil {
+			return fmt.Errorf("wal truncate: %w", err)
+		}
+	}
+	if _, err := db.walFile.Seek(int64(pageN*PageSize), io.SeekStart); err != nil {
 		return fmt.Errorf("wal seek: %w", err)
 	}
 	db.walPageN = pageN
@@ -184,6 +224,27 @@ func (db *DB) openWAL() (err error) {
 	return nil
 }
 
+// methodicalWALPageN tries to determine the last meta page in a very reliable
+// but slow way. This handles the theoretical but hard to imagine creating
+// edge case where we have a bitmap page which happens to look like a meta
+// page, and the write got interrupted before the meta page got written.
+func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) {
+	for i := 0; i < pageN; i++ {
+		var page []byte
+		if page, err = db.readWALPageAt(i); err != nil {
+			return -1, err
+		}
+		switch {
+		case IsMetaPage(page):
+			lastMeta = i
+		case IsBitmapHeader(page):
+			// skip the bitmap page, which we can't usefully evaluate
+			i++
+		}
+	}
+	return lastMeta, nil
+}
+
 // checkpoint moves all WAL pages to the main DB file.
 // Must be called by a write transaction while under db.mu lock.
 func (db *DB) checkpoint() error {
@@ -199,28 +260,53 @@ func (db *DB) checkpoint() error {
 	if db.walPageN == 0 {
 		return nil
 	}
+	// We might have either a *PageMap or just the file. If we have the file,
+	// building the PageMap is fairly expensive because it's fancy and immutable.
+	// If we have the PageMap *or* some other map, that's two different things
+	// to iterate. If we have the PageMap, building a map from it is relatively
+	// cheap, so we'll do it that way.
+	pages := make(map[uint32]int)
+	if db.pageMap.size == 0 {
+		// you'd think we're done, but actually this PROBABLY means that
+		// this is initial startup, and we haven't read the file yet. We scan
+		// the file for pages, because it turns out most of them probably
+		// got overwritten.
+		for i := 0; i < db.walPageN; i++ {
+			page, err := db.readWALPageAt(i)
+			if err != nil {
+				return err
+			}
+
+			// Determine page number. Meta pages are always on zero & bitmap
+			// headers specify the page number of the next page in the WAL.
+			// All other pages have their page number in the page data.
+			var pgno uint32
+			if IsBitmapHeader(page) {
+				pgno = readPageNo(page)
+				if page, err = db.readWALPageAt(i + 1); err != nil {
+					return err
+				}
+				i++ // bitmaps in WAL are two pages
+			} else if !IsMetaPage(page) {
+				pgno = readPageNo(page)
+			}
+			// record where in the file we have this page
+			pages[pgno] = i
+		}
+	} else {
+		itr := db.pageMap.Iterator()
+		itr.First()
+		for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() {
+			pages[k] = int(v)
+		}
+	}
 
 	// fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size)
-	for i := 0; i < db.walPageN; i++ {
-		page, err := db.readWALPageAt(i)
+	for pgno, walID := range pages {
+		page, err := db.readWALPageAt(walID)
 		if err != nil {
 			return err
 		}
-
-		// Determine page number. Meta pages are always on zero & bitmap
-		// headers specify the page number of the next page in the WAL.
-		// All other pages have their page number in the page data.
-		var pgno uint32
-		if IsBitmapHeader(page) {
-			pgno = readPageNo(page)
-			if page, err = db.readWALPageAt(i + 1); err != nil {
-				return err
-			}
-			i++ // bitmaps in WAL are two pages
-		} else if !IsMetaPage(page) {
-			pgno = readPageNo(page)
-		}
-
 		// Write data to the data file.
 		if err := db.writeDBPage(pgno, page); err != nil {
 			return err
@@ -509,23 +595,35 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 // running new tx.
 func (db *DB) removeTx(tx *Tx) error {
 	db.mu.Lock()
-	defer db.mu.Unlock()
-	// Release writer lock if tx is writable.
+	// release the write lock. we have to do this for now. some day we won't,
+	// and will want to hold it, but right now we can't be sure we can get it.
 	if tx.writable {
 		tx.db.rwmu.Unlock()
 	}
-
+	// remove ourselves from the list of transactions the db is keeping.
 	delete(tx.db.txs, tx)
 
 	// Disassociate from db.
 	tx.db = nil
 
-	// Write pages from WAL to DB.
-	// TODO(bbj): Move this to an async goroutine.
+	// Write pages from WAL to DB. As of this instant, we are the ONLY
+	// transaction, which means that no transaction has an older version
+	// of the PageMap than we do, and if we're a write, Commit() already
+	// updated the page map to our page map. So, if we *can* checkpoint,
+	// the checkpoint gets spawned asynchronously. We could block writes,
+	// except doing so will deadlock in a weird way.
 	if len(db.txs) == 0 && db.walSize() > db.cfg.MinWALCheckpointSize {
-		if err := db.checkpoint(); err != nil {
-			return fmt.Errorf("checkpoint: %w", err)
-		}
+		// We are doing this function with the db lock held, which means
+		// we're *not* releasing the db lock, even though we're returning.
+		// This is a weird special case, and probably a bad idea.
+		go func() {
+			defer db.mu.Unlock()
+			if err := db.checkpoint(); err != nil {
+				db.logger.Errorf("async checkpoint: %w", err)
+			}
+		}()
+	} else {
+		defer db.mu.Unlock()
 	}
 
 	return nil
diff --git a/rbf/rbf_test.go b/rbf/rbf_test.go
index 48f16335a..be3461e37 100644
--- a/rbf/rbf_test.go
+++ b/rbf/rbf_test.go
@@ -11,6 +11,7 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/molecula/featurebase/v2/logger"
 	"github.com/molecula/featurebase/v2/rbf"
 	rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg"
 	"github.com/molecula/featurebase/v2/testhook"
@@ -65,6 +66,13 @@ func NewDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB {
 // MustOpenDB returns a db opened on a temporary file. On error, fail test.
 func MustOpenDB(tb testing.TB, cfg ...*rbfcfg.Config) *rbf.DB {
 	tb.Helper()
+	if len(cfg) == 0 || cfg[0] == nil {
+		newconf := rbfcfg.NewDefaultConfig()
+		newconf.Logger = logger.NewLogfLogger(tb)
+		cfg = []*rbfcfg.Config{newconf}
+	} else if cfg[0].Logger == nil {
+		cfg[0].Logger = logger.NewLogfLogger(tb)
+	}
 	db := NewDB(tb, cfg...)
 	if err := db.Open(); err != nil {
 		tb.Fatal(err)

From 806669fa0f5c738a5e1d70ac749c67453540bf1c Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 15:27:16 -0600
Subject: [PATCH 27/51] make db able to fail out if it can't checkpoint, fix
 silly wrong-units error

PageMap uses "WALID", which is a WAL page ID relative to the "base" ID of the
WAL, rather than the wal page count you'd get just reading the file. So everything
it reports has a fixed offset at any given time. I think this may be left
over from a point where there were partial checkpoints. Anyway, the net
outcome is that each new transaction was getting different page IDs, but
the actual WAL pages did not always reflect that. Each checkpoint increases
the offset. This might imply that we can start having problems after
4 billion pages written even if most of them were redundant?

Anyway, with that fixed, this seems to work. I think.
---
 rbf/db.go | 76 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 830e31f5a..40c98e22a 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -49,6 +49,8 @@ type DB struct {
 	rwmu     sync.Mutex   // mutex for restricting single writer
 	haltCond *sync.Cond   // condition for resuming txs after checkpoint
 
+	isDead error // this database died in an unrecoverable way, error out opens
+
 	// Path represents the path to the database file.
 	Path string
 }
@@ -247,7 +249,7 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) {
 
 // checkpoint moves all WAL pages to the main DB file.
 // Must be called by a write transaction while under db.mu lock.
-func (db *DB) checkpoint() error {
+func (db *DB) checkpoint() (err error) {
 	if !db.opened {
 		return nil
 	} else if len(db.txs) > 0 {
@@ -260,21 +262,31 @@ func (db *DB) checkpoint() error {
 	if db.walPageN == 0 {
 		return nil
 	}
+	// wake up things waiting on haltCond when we're done, even if we fail.
+	// Otherwise, we deadlock with them all stuck waiting on that forever.
+	defer func() {
+		if err != nil && db.isDead == nil {
+			db.isDead = err
+		}
+		db.haltCond.Broadcast()
+	}()
+	var page []byte
 	// We might have either a *PageMap or just the file. If we have the file,
 	// building the PageMap is fairly expensive because it's fancy and immutable.
 	// If we have the PageMap *or* some other map, that's two different things
 	// to iterate. If we have the PageMap, building a map from it is relatively
 	// cheap, so we'll do it that way.
 	pages := make(map[uint32]int)
+
 	if db.pageMap.size == 0 {
 		// you'd think we're done, but actually this PROBABLY means that
 		// this is initial startup, and we haven't read the file yet. We scan
 		// the file for pages, because it turns out most of them probably
 		// got overwritten.
 		for i := 0; i < db.walPageN; i++ {
-			page, err := db.readWALPageAt(i)
+			page, err = db.readWALPageAt(i)
 			if err != nil {
-				return err
+				return fmt.Errorf("reading WAL page %d: %w", i, err)
 			}
 
 			// Determine page number. Meta pages are always on zero & bitmap
@@ -283,8 +295,12 @@ func (db *DB) checkpoint() error {
 			var pgno uint32
 			if IsBitmapHeader(page) {
 				pgno = readPageNo(page)
-				if page, err = db.readWALPageAt(i + 1); err != nil {
-					return err
+				if i+1 < db.walPageN {
+					if page, err = db.readWALPageAt(i + 1); err != nil {
+						return err
+					}
+				} else {
+					return fmt.Errorf("last page of WAL file (%d) is bitmap header", i)
 				}
 				i++ // bitmaps in WAL are two pages
 			} else if !IsMetaPage(page) {
@@ -294,41 +310,37 @@ func (db *DB) checkpoint() error {
 			pages[pgno] = i
 		}
 	} else {
+		walBase := db.baseWALID()
 		itr := db.pageMap.Iterator()
 		itr.First()
 		for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() {
-			pages[k] = int(v)
+			pages[k] = int(v - walBase - 1)
 		}
 	}
 
 	// fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size)
 	for pgno, walID := range pages {
-		page, err := db.readWALPageAt(walID)
+		page, err = db.readWALPageAt(walID)
 		if err != nil {
-			return err
+			return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err)
 		}
 		// Write data to the data file.
-		if err := db.writeDBPage(pgno, page); err != nil {
-			return err
+		if err = db.writeDBPage(pgno, page); err != nil {
+			return fmt.Errorf("writing page %d: %v", pgno, err)
 		}
 	}
-
 	// Ensure database file is synced and then truncate the WAL file.
-	if err := db.fsync(db.file); err != nil {
+	if err = db.fsync(db.file); err != nil {
 		return fmt.Errorf("db file sync: %w", err)
-	} else if err := db.walFile.Truncate(0); err != nil {
+	} else if err = db.walFile.Truncate(0); err != nil {
 		return fmt.Errorf("truncate wal file: %w", err)
-	} else if err := db.fsync(db.walFile); err != nil {
+	} else if err = db.fsync(db.walFile); err != nil {
 		return fmt.Errorf("wal file sync: %w", err)
-	} else if _, err := db.walFile.Seek(0, io.SeekStart); err != nil {
+	} else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil {
 		return fmt.Errorf("seek wal file: %w", err)
 	}
 	db.walPageN = 0
 	db.pageMap = NewPageMap()
-
-	// Notify halted transactions that the WAL has been checkpointed.
-	db.haltCond.Broadcast()
-
 	return nil
 }
 
@@ -537,9 +549,21 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 		cleanup()
 		return nil, ErrClosed
 	}
+	if db.isDead != nil {
+		err := db.isDead
+		cleanup()
+		db.mu.Unlock()
+		return nil, err
+	}
 
 	// Wait for WAL size to be below threshold.
 	for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize {
+		if db.isDead != nil {
+			err := db.isDead
+			cleanup()
+			db.mu.Unlock()
+			return nil, err
+		}
 		db.haltCond.Wait()
 	}
 
@@ -616,14 +640,14 @@ func (db *DB) removeTx(tx *Tx) error {
 		// We are doing this function with the db lock held, which means
 		// we're *not* releasing the db lock, even though we're returning.
 		// This is a weird special case, and probably a bad idea.
-		go func() {
-			defer db.mu.Unlock()
-			if err := db.checkpoint(); err != nil {
-				db.logger.Errorf("async checkpoint: %w", err)
-			}
-		}()
-	} else {
+		// go func() {
 		defer db.mu.Unlock()
+		if err := db.checkpoint(); err != nil {
+			db.logger.Errorf("async checkpoint: %v", err)
+		}
+		// }()
+	} else {
+		db.mu.Unlock()
 	}
 
 	return nil

From 6d68e719338bb825ec1ff537b35b9538ec1581b4 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 19 Nov 2021 15:32:58 -0600
Subject: [PATCH 28/51] make the checkpoint async

---
 rbf/db.go | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 40c98e22a..9d4769ff3 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -640,12 +640,12 @@ func (db *DB) removeTx(tx *Tx) error {
 		// We are doing this function with the db lock held, which means
 		// we're *not* releasing the db lock, even though we're returning.
 		// This is a weird special case, and probably a bad idea.
-		// go func() {
-		defer db.mu.Unlock()
-		if err := db.checkpoint(); err != nil {
-			db.logger.Errorf("async checkpoint: %v", err)
-		}
-		// }()
+		go func() {
+			defer db.mu.Unlock()
+			if err := db.checkpoint(); err != nil {
+				db.logger.Errorf("async checkpoint: %v", err)
+			}
+		}()
 	} else {
 		db.mu.Unlock()
 	}

From c3c02eabb0587a37a58fcf7e99d3b3016a6b8885 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Mon, 22 Nov 2021 14:02:53 -0600
Subject: [PATCH 29/51] almost but not quite support async checkpoint

This gets us to being able to run reads during a checkpoint, but
now we have to wait for new reads to end before we can release
the write lock, etc.

This is actually slightly slower, but if we could get ONE more step,
we could allow new writes during that phase, to a different WAL,
if we had a different WAL to write to.
---
 rbf/db.go | 157 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 126 insertions(+), 31 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 9d4769ff3..3db8889ca 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -28,6 +28,17 @@ var cursorSyncPool = &sync.Pool{
 	},
 }
 
+// txWaiter is a representation of "i need to wait for txs to complete".
+// it is created with a function, and will run that function, with the db
+// lock held, at some point after every Tx that was open when it was created
+// has closed. WARNING: A txWaiter may hold db.rwmu.
+type txWaiter struct {
+	mu        sync.Mutex
+	cond      *sync.Cond
+	waitingOn map[*Tx]struct{}
+	callback  func()
+}
+
 // DB options like 	MaxSize, FsyncEnabled, DoAllocZero
 // can be set before calling DB.Open().
 type DB struct {
@@ -49,6 +60,8 @@ type DB struct {
 	rwmu     sync.Mutex   // mutex for restricting single writer
 	haltCond *sync.Cond   // condition for resuming txs after checkpoint
 
+	txWaiters []*txWaiter // things waiting for Txs to close
+
 	isDead error // this database died in an unrecoverable way, error out opens
 
 	// Path represents the path to the database file.
@@ -142,8 +155,12 @@ func (db *DB) Open() (err error) {
 	// Open write-ahead log & checkpoint to the end since no transactions are open.
 	if err := db.openWAL(); err != nil {
 		return fmt.Errorf("wal open: %w", err)
-	} else if err := db.checkpoint(); err != nil {
-		return fmt.Errorf("startup checkpoint: %w", err)
+	} else {
+		// checkpoint wants to hold the rwmu lock.
+		db.rwmu.Lock()
+		if err := db.checkpoint(); err != nil {
+			return fmt.Errorf("startup checkpoint: %w", err)
+		}
 	}
 
 	return nil
@@ -247,9 +264,18 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) {
 	return lastMeta, nil
 }
 
-// checkpoint moves all WAL pages to the main DB file.
-// Must be called by a write transaction while under db.mu lock.
+// checkpoint moves all WAL pages to the main DB file. Must be called
+// while holding both db.mu and db.rwmu. Should release db.rwmu, but not
+// db.mu.
 func (db *DB) checkpoint() (err error) {
+	// if we don't spin off a possible async waiter, we should release the
+	// write lock, if we do, that will release it.
+	releaseLock := true
+	defer func() {
+		if releaseLock {
+			db.rwmu.Unlock()
+		}
+	}()
 	if !db.opened {
 		return nil
 	} else if len(db.txs) > 0 {
@@ -332,15 +358,26 @@ func (db *DB) checkpoint() (err error) {
 	// Ensure database file is synced and then truncate the WAL file.
 	if err = db.fsync(db.file); err != nil {
 		return fmt.Errorf("db file sync: %w", err)
-	} else if err = db.walFile.Truncate(0); err != nil {
-		return fmt.Errorf("truncate wal file: %w", err)
-	} else if err = db.fsync(db.walFile); err != nil {
-		return fmt.Errorf("wal file sync: %w", err)
-	} else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil {
-		return fmt.Errorf("seek wal file: %w", err)
 	}
-	db.walPageN = 0
-	db.pageMap = NewPageMap()
+	// now we've updated the file. There are existing transactions that are still
+	// using the WAL, though. So we wait for them to terminate before we unlock
+	// the rwmu and update the metadata about the WAL.
+	releaseLock = false
+	// fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n")
+	db.afterCurrentTx(func() {
+		// fmt.Printf("truncating WAL\n")
+		defer db.rwmu.Unlock()
+		if err = db.walFile.Truncate(0); err != nil {
+			db.logger.Errorf("truncate wal file: %w", err)
+		} else if err = db.fsync(db.walFile); err != nil {
+			db.logger.Errorf("wal file sync: %w", err)
+		} else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil {
+			db.logger.Errorf("seek wal file: %w", err)
+		}
+		db.walPageN = 0
+		db.pageMap = NewPageMap()
+		// fmt.Printf("checkpoint actually done\n")
+	})
 	return nil
 }
 
@@ -613,43 +650,101 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	return tx, nil
 }
 
+// afterCurrentTx produces runs the provided callback, with the db lock
+// held, after all current Tx terminate. It should be called with the db
+// lock held.
+func (db *DB) afterCurrentTx(callback func()) {
+	if len(db.txs) == 0 {
+		callback()
+		return
+	}
+	txw := &txWaiter{}
+	txw.cond = sync.NewCond(&txw.mu)
+	txw.callback = callback
+	txw.waitingOn = make(map[*Tx]struct{}, len(db.txs))
+	for k := range db.txs {
+		txw.waitingOn[k] = struct{}{}
+	}
+	db.txWaiters = append(db.txWaiters, txw)
+	txw.mu.Lock()
+	go func() {
+		for len(txw.waitingOn) > 0 {
+			// fmt.Printf("afterCurrentTx: %d left\n", len(txw.waitingOn))
+			txw.cond.Wait()
+		}
+		// fmt.Printf("afterCurrentTx: locking db\n")
+		db.mu.Lock()
+		defer db.mu.Unlock()
+		// remove us from the db's list
+		for i, v := range db.txWaiters {
+			if v == txw {
+				// remove us from the list
+				copy(db.txWaiters[i:], db.txWaiters[i+1:])
+				db.txWaiters = db.txWaiters[:len(db.txWaiters)-1]
+				break
+			}
+		}
+		// fmt.Printf("afterCurrentTx: running callback\n")
+		txw.callback()
+	}()
+	return
+}
+
 // removeTx removes an active transaction from the database. it obtains
 // the db lock, and currently drops it, but will later possibly be leaving
 // it retained by an asynchronous op that wants to happen before we start
 // running new tx.
 func (db *DB) removeTx(tx *Tx) error {
 	db.mu.Lock()
-	// release the write lock. we have to do this for now. some day we won't,
-	// and will want to hold it, but right now we can't be sure we can get it.
+	defer db.mu.Unlock()
+	// We might want to trigger a checkpoint. Only for writable
+	// transactions, and only when either there's nothing else open or we
+	// really need to.
+	checkpoint := false
 	if tx.writable {
-		tx.db.rwmu.Unlock()
+		walSize := db.walSize()
+		if walSize > db.cfg.MinWALCheckpointSize {
+			// Might be a good time for a checkpoint. We'll do a checkpoint
+			// if we're the only transaction, or if we have to.
+			if len(db.txs) == 1 || walSize > db.cfg.MaxWALCheckpointSize {
+				checkpoint = true
+			}
+		}
+		// During checkpointing, we'll be preventing writes, but allowing reads.
+		if !checkpoint {
+			tx.db.rwmu.Unlock()
+		}
 	}
 	// remove ourselves from the list of transactions the db is keeping.
 	delete(tx.db.txs, tx)
+	for _, txw := range tx.db.txWaiters {
+		delete(txw.waitingOn, tx)
+		// let it know we're done. we've still got db.mu.lock, so it won't
+		// happen just yet, but it'll be able to continue.
+		if len(txw.waitingOn) == 0 {
+			txw.cond.Broadcast()
+		}
+	}
 
 	// Disassociate from db.
 	tx.db = nil
 
-	// Write pages from WAL to DB. As of this instant, we are the ONLY
-	// transaction, which means that no transaction has an older version
-	// of the PageMap than we do, and if we're a write, Commit() already
-	// updated the page map to our page map. So, if we *can* checkpoint,
-	// the checkpoint gets spawned asynchronously. We could block writes,
-	// except doing so will deadlock in a weird way.
-	if len(db.txs) == 0 && db.walSize() > db.cfg.MinWALCheckpointSize {
-		// We are doing this function with the db lock held, which means
-		// we're *not* releasing the db lock, even though we're returning.
-		// This is a weird special case, and probably a bad idea.
-		go func() {
-			defer db.mu.Unlock()
+	if checkpoint {
+		// We need to run a checkpoint. This can be semi-asynchronous.
+		// It needs to wait until every existing transaction has finished,
+		// because every existing transaction could want to look up pages
+		// which are in the database before our operations, but which should
+		// now be in the WAL. We want them to use the WAL instead.
+		// fmt.Printf("possibly-async checkpoint...\n")
+		db.afterCurrentTx(func() {
+			// We still hold db.rwmu here. checkpoint unlocks it when it's
+			// ready.
+			// fmt.Printf("checkpoint starting\n")
 			if err := db.checkpoint(); err != nil {
 				db.logger.Errorf("async checkpoint: %v", err)
 			}
-		}()
-	} else {
-		db.mu.Unlock()
+		})
 	}
-
 	return nil
 }
 

From 4279e2cb2d8010639de375021a99e40a64538e9f Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Thu, 16 Dec 2021 09:17:13 -0700
Subject: [PATCH 30/51] rebase fixes

---
 rbf/cursor_test.go | 4 ++--
 rbf/db.go          | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/rbf/cursor_test.go b/rbf/cursor_test.go
index 4f7464bd1..940798c04 100644
--- a/rbf/cursor_test.go
+++ b/rbf/cursor_test.go
@@ -973,8 +973,8 @@ func TestCursor_SplitBranchCells(t *testing.T) {
 	}
 	//
 	c, _ := tx.Cursor("x") //added just for dot code coverage
-	c.Dump("ignore for coverage")
-
+	c.Dump("test.dump")
+	os.Remove("test.dump")
 }
 
 func TestCursor_RemoveCells(t *testing.T) {
diff --git a/rbf/db.go b/rbf/db.go
index 3db8889ca..21596d144 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -695,8 +695,6 @@ func (db *DB) afterCurrentTx(callback func()) {
 // it retained by an asynchronous op that wants to happen before we start
 // running new tx.
 func (db *DB) removeTx(tx *Tx) error {
-	db.mu.Lock()
-	defer db.mu.Unlock()
 	// We might want to trigger a checkpoint. Only for writable
 	// transactions, and only when either there's nothing else open or we
 	// really need to.

From 29f5f6d7c2dc84aebfae9d4a67bf9f7e65bcb3c2 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Wed, 15 Dec 2021 12:09:40 -0600
Subject: [PATCH 31/51] copy things rows after getting them and before their
 finishers during writes

When a qcx is a write, every Tx under it closes immediately, thus
invalidating all returned data. Thus, if you do a Not() inside a Store(),
you're doing a difference on an existence row and some other row
call... and both of those rows were run, individually, as separate
transactions that got invalidated the moment they were fetched. Oops.
---
 executor.go | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/executor.go b/executor.go
index 226759b55..8b0409eb7 100644
--- a/executor.go
+++ b/executor.go
@@ -4493,7 +4493,11 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string,
 			return nil, err
 		}
 		defer finisher(&err0)
-		return frag.row(tx, rowID)
+		row, err := frag.row(tx, rowID)
+		if qcx.write && err == nil {
+			row = row.Clone()
+		}
+		return row, err
 	}
 
 	// If no quantum exists then return an empty bitmap.
@@ -4532,15 +4536,21 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string,
 	if len(rows) == 0 {
 		return &Row{}, nil
 	} else if len(rows) == 1 {
+		if qcx.write {
+			return rows[0].Clone(), nil
+		}
 		return rows[0], nil
 	}
 	row := rows[0].Union(rows[1:]...)
+	if qcx.write {
+		row = row.Clone()
+	}
 	return row, nil
 
 }
 
 // executeRowBSIGroupShard executes a range(bsiGroup) call for a local shard.
-func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shard uint64) (_ *Row, err0 error) {
+func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shard uint64) (cloneable *Row, err0 error) {
 	span, _ := tracing.StartSpanFromContext(ctx, "Executor.executeRowBSIGroupShard")
 	defer span.Finish()
 
@@ -4572,6 +4582,11 @@ func (e *executor) executeRowBSIGroupShard(ctx context.Context, qcx *Qcx, index
 		return nil, err
 	}
 	defer finisher(&err0)
+	defer func() {
+		if qcx.write && cloneable != nil {
+			cloneable = cloneable.Clone()
+		}
+	}()
 
 	// EQ null           _exists - frag.NotNull()
 	// NEQ null          frag.NotNull()
@@ -4822,6 +4837,9 @@ func (e *executor) executeNotShard(ctx context.Context, qcx *Qcx, index string,
 		if existenceRow, err = existenceFrag.row(tx, 0); err != nil {
 			return nil, err
 		}
+		if qcx.write {
+			existenceRow = existenceRow.Clone()
+		}
 	}
 	// the finishers returned by a write tx, which we might be in if there's
 	// a higher-level write in this call OR ANY OTHER CALL, are safe to

From 5764d98f6d0198b2b61b32f5e336de5d59b110d2 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Thu, 16 Dec 2021 11:06:17 -0600
Subject: [PATCH 32/51] test fixes and order of operations on changing
 db.PageMap

We need to update db.PageMap after we write the db, but before
we truncate the WAL, so new transactions don't pick up the old
PageMap and then get a truncated WAL.

Also, checkpoint should not abort if there's txs -- that's okay now.
---
 rbf/db.go      | 30 +++++++++++++++++-------------
 rbf/db_test.go |  6 ++++--
 rbf/tx.go      |  5 ++---
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 21596d144..cfa5ede43 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -278,8 +278,6 @@ func (db *DB) checkpoint() (err error) {
 	}()
 	if !db.opened {
 		return nil
-	} else if len(db.txs) > 0 {
-		return nil // skip if transactions open
 	}
 
 	// Check if there are any WAL pages, if not do nothing as
@@ -364,6 +362,8 @@ func (db *DB) checkpoint() (err error) {
 	// the rwmu and update the metadata about the WAL.
 	releaseLock = false
 	// fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n")
+	db.walPageN = 0
+	db.pageMap = NewPageMap()
 	db.afterCurrentTx(func() {
 		// fmt.Printf("truncating WAL\n")
 		defer db.rwmu.Unlock()
@@ -374,8 +374,6 @@ func (db *DB) checkpoint() (err error) {
 		} else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil {
 			db.logger.Errorf("seek wal file: %w", err)
 		}
-		db.walPageN = 0
-		db.pageMap = NewPageMap()
 		// fmt.Printf("checkpoint actually done\n")
 	})
 	return nil
@@ -589,19 +587,22 @@ func (db *DB) Begin(writable bool) (_ *Tx, err error) {
 	if db.isDead != nil {
 		err := db.isDead
 		cleanup()
-		db.mu.Unlock()
 		return nil, err
 	}
 
-	// Wait for WAL size to be below threshold.
-	for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize {
-		if db.isDead != nil {
-			err := db.isDead
-			cleanup()
-			db.mu.Unlock()
-			return nil, err
+	// Wait for WAL size to be below threshold, if we're going to write.
+	// Reads don't care.
+	if writable {
+		for int64(db.walPageN*PageSize) > db.cfg.MaxWALCheckpointSize {
+			if db.isDead != nil {
+				err := db.isDead
+				cleanup()
+				return nil, err
+			}
+			// This implicitly releases db.mu.Lock and comes back with it
+			// held again.
+			db.haltCond.Wait()
 		}
-		db.haltCond.Wait()
 	}
 
 	tx := &Tx{
@@ -775,6 +776,9 @@ func (db *DB) baseWALID() int64 {
 
 // readWALPageByID reads a WAL page by WAL ID.
 func (db *DB) readWALPageByID(id int64) ([]byte, error) {
+	if id == db.baseWALID() {
+		fmt.Printf("id %d oops\n", id)
+	}
 	return db.readWALPageAt(int(id - db.baseWALID() - 1))
 }
 
diff --git a/rbf/db_test.go b/rbf/db_test.go
index 5c8504b1b..c0eb3b3c7 100644
--- a/rbf/db_test.go
+++ b/rbf/db_test.go
@@ -291,7 +291,8 @@ func TestDB_MultiTx(t *testing.T) {
 
 					time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond)
 
-					for i := 0; i < rand.Intn(1000); i++ {
+					n := rand.Intn(500) + 500
+					for i := 0; i < n; i++ {
 						v := rand.Intn(1 << 20)
 						if _, err := tx.Contains("x", uint64(v)); err != nil {
 							return err
@@ -316,7 +317,8 @@ func TestDB_MultiTx(t *testing.T) {
 			}
 			defer tx.Rollback()
 
-			for j := 0; j < rand.Intn(100); j++ {
+			n := rand.Intn(90) + 10
+			for j := 0; j < n; j++ {
 				v := rand.Intn(1 << 20)
 				if _, err := tx.Add("x", uint64(v)); err != nil {
 					t.Fatal(err)
diff --git a/rbf/tx.go b/rbf/tx.go
index 2cf7420db..bfb8fc00c 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -102,6 +102,8 @@ func (tx *Tx) Commit() error {
 
 	// If any pages have been written, ensure we write a new meta page with
 	// the commit flag to mark the end of the transaction.
+	tx.db.mu.Lock()
+	defer tx.db.mu.Unlock()
 	if tx.dirty() {
 		if err := tx.flush(); err != nil {
 			return err
@@ -118,12 +120,9 @@ func (tx *Tx) Commit() error {
 		// the lock, because we need removeTx to grab the lock to
 		// work, but if it wants to checkpoint, it wants to be able to return
 		// to us here and still be holding the lock.
-		tx.db.mu.Lock()
 		tx.db.rootRecords = tx.rootRecords
 		tx.db.pageMap = tx.pageMap
 		tx.db.walPageN = tx.walPageN
-		tx.db.mu.Unlock()
-		return tx.db.removeTx(tx)
 	}
 
 	// Disconnect transaction from DB.

From 994cc03e88717642f3b06614eef6d7afa859329b Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Thu, 16 Dec 2021 14:01:38 -0600
Subject: [PATCH 33/51] fix locking and list management for afterCurrentTx

Two issues: First, there was a race condition because we were never
using the mutex for anything but the condvar broadcast, second, there
was no reason for the afterCurrentTx to need to maintain the list since
we already know where in the list we are when we are waking it up.

afterCurrentTx still wants to run with the db lock held, because
the degenerate case (no outstanding Tx) means that it will be running
with it held already. That's for another commit.
---
 rbf/db.go | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index cfa5ede43..0662f59d6 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -676,15 +676,6 @@ func (db *DB) afterCurrentTx(callback func()) {
 		// fmt.Printf("afterCurrentTx: locking db\n")
 		db.mu.Lock()
 		defer db.mu.Unlock()
-		// remove us from the db's list
-		for i, v := range db.txWaiters {
-			if v == txw {
-				// remove us from the list
-				copy(db.txWaiters[i:], db.txWaiters[i+1:])
-				db.txWaiters = db.txWaiters[:len(db.txWaiters)-1]
-				break
-			}
-		}
 		// fmt.Printf("afterCurrentTx: running callback\n")
 		txw.callback()
 	}()
@@ -716,12 +707,23 @@ func (db *DB) removeTx(tx *Tx) error {
 	}
 	// remove ourselves from the list of transactions the db is keeping.
 	delete(tx.db.txs, tx)
-	for _, txw := range tx.db.txWaiters {
+	for i := 0; i < len(tx.db.txWaiters); i++ {
+		txw := tx.db.txWaiters[i]
+		// in practice this probably never matters, but theoretically the
+		// goroutine that's waiting on the condition variable may
+		// not have performed its first test on len(txw.waitingOn) yet.
+		txw.mu.Lock()
 		delete(txw.waitingOn, tx)
+		txw.mu.Unlock()
 		// let it know we're done. we've still got db.mu.lock, so it won't
 		// happen just yet, but it'll be able to continue.
 		if len(txw.waitingOn) == 0 {
+			// remove us from the db's list
+			copy(db.txWaiters[i:], db.txWaiters[i+1:])
+			db.txWaiters = db.txWaiters[:len(db.txWaiters)-1]
 			txw.cond.Broadcast()
+			// decrement i so we don't skip an entry we just copied in to [i]
+			i--
 		}
 	}
 

From 47e098c3b1a84c4e0108b10ba050ddf37381448e Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Thu, 16 Dec 2021 15:54:06 -0600
Subject: [PATCH 34/51] simplify txWaiter

We don't need a condition variable for a thing with a single waiter
which waits only once, and a data structure which only one side ever
modifies. That's a closable channel.
---
 rbf/db.go | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 0662f59d6..4129ac1fe 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -33,8 +33,7 @@ var cursorSyncPool = &sync.Pool{
 // lock held, at some point after every Tx that was open when it was created
 // has closed. WARNING: A txWaiter may hold db.rwmu.
 type txWaiter struct {
-	mu        sync.Mutex
-	cond      *sync.Cond
+	ready     chan struct{}
 	waitingOn map[*Tx]struct{}
 	callback  func()
 }
@@ -660,19 +659,15 @@ func (db *DB) afterCurrentTx(callback func()) {
 		return
 	}
 	txw := &txWaiter{}
-	txw.cond = sync.NewCond(&txw.mu)
+	txw.ready = make(chan struct{})
 	txw.callback = callback
 	txw.waitingOn = make(map[*Tx]struct{}, len(db.txs))
 	for k := range db.txs {
 		txw.waitingOn[k] = struct{}{}
 	}
 	db.txWaiters = append(db.txWaiters, txw)
-	txw.mu.Lock()
 	go func() {
-		for len(txw.waitingOn) > 0 {
-			// fmt.Printf("afterCurrentTx: %d left\n", len(txw.waitingOn))
-			txw.cond.Wait()
-		}
+		<-txw.ready
 		// fmt.Printf("afterCurrentTx: locking db\n")
 		db.mu.Lock()
 		defer db.mu.Unlock()
@@ -712,16 +707,14 @@ func (db *DB) removeTx(tx *Tx) error {
 		// in practice this probably never matters, but theoretically the
 		// goroutine that's waiting on the condition variable may
 		// not have performed its first test on len(txw.waitingOn) yet.
-		txw.mu.Lock()
 		delete(txw.waitingOn, tx)
-		txw.mu.Unlock()
 		// let it know we're done. we've still got db.mu.lock, so it won't
 		// happen just yet, but it'll be able to continue.
 		if len(txw.waitingOn) == 0 {
 			// remove us from the db's list
 			copy(db.txWaiters[i:], db.txWaiters[i+1:])
 			db.txWaiters = db.txWaiters[:len(db.txWaiters)-1]
-			txw.cond.Broadcast()
+			close(txw.ready)
 			// decrement i so we don't skip an entry we just copied in to [i]
 			i--
 		}

From 57ca5591a264740daddb7e819f745f71c9163951 Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Fri, 17 Dec 2021 10:59:09 -0700
Subject: [PATCH 35/51] Unlock rbf.DB during WAL copy & fsync()

---
 rbf/db.go | 149 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 79 insertions(+), 70 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index 4129ac1fe..e5a81690e 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -51,9 +51,10 @@ type DB struct {
 	opened      bool                 // true if open
 	logger      logger.Logger        // for diagnostics from async things
 
-	wal      []byte   // wal mmap
-	walFile  *os.File // wal file descriptor
-	walPageN int      // wal page count
+	wal       []byte   // wal mmap
+	walFile   *os.File // wal file descriptor
+	walPageN  int      // wal page count
+	baseWALID int64    // WAL ID of first page
 
 	mu       sync.RWMutex // general mutex
 	rwmu     sync.Mutex   // mutex for restricting single writer
@@ -238,6 +239,7 @@ func (db *DB) openWAL() (err error) {
 		return fmt.Errorf("wal seek: %w", err)
 	}
 	db.walPageN = pageN
+	db.baseWALID = readMetaWALID(db.data)
 
 	return nil
 }
@@ -293,79 +295,92 @@ func (db *DB) checkpoint() (err error) {
 		}
 		db.haltCond.Broadcast()
 	}()
-	var page []byte
-	// We might have either a *PageMap or just the file. If we have the file,
-	// building the PageMap is fairly expensive because it's fancy and immutable.
-	// If we have the PageMap *or* some other map, that's two different things
-	// to iterate. If we have the PageMap, building a map from it is relatively
-	// cheap, so we'll do it that way.
-	pages := make(map[uint32]int)
 
-	if db.pageMap.size == 0 {
-		// you'd think we're done, but actually this PROBABLY means that
-		// this is initial startup, and we haven't read the file yet. We scan
-		// the file for pages, because it turns out most of them probably
-		// got overwritten.
-		for i := 0; i < db.walPageN; i++ {
-			page, err = db.readWALPageAt(i)
-			if err != nil {
-				return fmt.Errorf("reading WAL page %d: %w", i, err)
-			}
+	// Copy the pages from the WAL back to the database outside of the lock.
+	if err := func() error {
+		db.mu.Unlock() // This is intentionally reversed so run w/o lock
+		defer db.mu.Lock()
 
-			// Determine page number. Meta pages are always on zero & bitmap
-			// headers specify the page number of the next page in the WAL.
-			// All other pages have their page number in the page data.
-			var pgno uint32
-			if IsBitmapHeader(page) {
-				pgno = readPageNo(page)
-				if i+1 < db.walPageN {
-					if page, err = db.readWALPageAt(i + 1); err != nil {
-						return err
-					}
-				} else {
-					return fmt.Errorf("last page of WAL file (%d) is bitmap header", i)
+		var page []byte
+		// We might have either a *PageMap or just the file. If we have the file,
+		// building the PageMap is fairly expensive because it's fancy and immutable.
+		// If we have the PageMap *or* some other map, that's two different things
+		// to iterate. If we have the PageMap, building a map from it is relatively
+		// cheap, so we'll do it that way.
+		pages := make(map[uint32]int)
+
+		if db.pageMap.size == 0 {
+			// you'd think we're done, but actually this PROBABLY means that
+			// this is initial startup, and we haven't read the file yet. We scan
+			// the file for pages, because it turns out most of them probably
+			// got overwritten.
+			for i := 0; i < db.walPageN; i++ {
+				page, err = db.readWALPageAt(i)
+				if err != nil {
+					return fmt.Errorf("reading WAL page %d: %w", i, err)
 				}
-				i++ // bitmaps in WAL are two pages
-			} else if !IsMetaPage(page) {
-				pgno = readPageNo(page)
+
+				// Determine page number. Meta pages are always on zero & bitmap
+				// headers specify the page number of the next page in the WAL.
+				// All other pages have their page number in the page data.
+				var pgno uint32
+				if IsBitmapHeader(page) {
+					pgno = readPageNo(page)
+					if i+1 < db.walPageN {
+						if page, err = db.readWALPageAt(i + 1); err != nil {
+							return err
+						}
+					} else {
+						return fmt.Errorf("last page of WAL file (%d) is bitmap header", i)
+					}
+					i++ // bitmaps in WAL are two pages
+				} else if !IsMetaPage(page) {
+					pgno = readPageNo(page)
+				}
+				// record where in the file we have this page
+				pages[pgno] = i
+			}
+		} else {
+			itr := db.pageMap.Iterator()
+			itr.First()
+			for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() {
+				pages[k] = int(v - db.baseWALID - 1)
 			}
-			// record where in the file we have this page
-			pages[pgno] = i
 		}
-	} else {
-		walBase := db.baseWALID()
-		itr := db.pageMap.Iterator()
-		itr.First()
-		for k, v, ok := itr.Next(); ok; k, v, ok = itr.Next() {
-			pages[k] = int(v - walBase - 1)
+
+		// fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size)
+		for pgno, walID := range pages {
+			page, err = db.readWALPageAt(walID)
+			if err != nil {
+				return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err)
+			}
+
+			// Write data to the data file.
+			if err = db.writeDBPage(pgno, page); err != nil {
+				return fmt.Errorf("writing page %d: %v", pgno, err)
+			}
 		}
+
+		// Ensure database file is synced and then truncate the WAL file.
+		if err = db.fsync(db.file); err != nil {
+			return fmt.Errorf("db file sync: %w", err)
+		}
+
+		return nil
+	}(); err != nil {
+		return err
 	}
 
-	// fmt.Printf("checkpoint: walPageN %d, PageMap size %d\n", db.walPageN, db.pageMap.size)
-	for pgno, walID := range pages {
-		page, err = db.readWALPageAt(walID)
-		if err != nil {
-			return fmt.Errorf("reading page %d [page number %d]: %v", walID, pgno, err)
-		}
-		// Write data to the data file.
-		if err = db.writeDBPage(pgno, page); err != nil {
-			return fmt.Errorf("writing page %d: %v", pgno, err)
-		}
-	}
-	// Ensure database file is synced and then truncate the WAL file.
-	if err = db.fsync(db.file); err != nil {
-		return fmt.Errorf("db file sync: %w", err)
-	}
 	// now we've updated the file. There are existing transactions that are still
 	// using the WAL, though. So we wait for them to terminate before we unlock
 	// the rwmu and update the metadata about the WAL.
 	releaseLock = false
-	// fmt.Printf("checkpoint mostly done, waiting for Tx cleanup...\n")
 	db.walPageN = 0
 	db.pageMap = NewPageMap()
+
 	db.afterCurrentTx(func() {
-		// fmt.Printf("truncating WAL\n")
 		defer db.rwmu.Unlock()
+
 		if err = db.walFile.Truncate(0); err != nil {
 			db.logger.Errorf("truncate wal file: %w", err)
 		} else if err = db.fsync(db.walFile); err != nil {
@@ -373,8 +388,10 @@ func (db *DB) checkpoint() (err error) {
 		} else if _, err = db.walFile.Seek(0, io.SeekStart); err != nil {
 			db.logger.Errorf("seek wal file: %w", err)
 		}
-		// fmt.Printf("checkpoint actually done\n")
+
+		db.baseWALID = readMetaWALID(db.data)
 	})
+
 	return nil
 }
 
@@ -764,17 +781,9 @@ func (db *DB) readDBPage(pgno uint32) ([]byte, error) {
 	return db.data[offset : offset+PageSize], nil
 }
 
-// baseWALID returns the WAL ID stored in the database file meta page.
-func (db *DB) baseWALID() int64 {
-	return readMetaWALID(db.data)
-}
-
 // readWALPageByID reads a WAL page by WAL ID.
 func (db *DB) readWALPageByID(id int64) ([]byte, error) {
-	if id == db.baseWALID() {
-		fmt.Printf("id %d oops\n", id)
-	}
-	return db.readWALPageAt(int(id - db.baseWALID() - 1))
+	return db.readWALPageAt(int(id - db.baseWALID - 1))
 }
 
 // readWALPageAt reads the i-th page in the WAL file.

From 1fd872b126356c07cac635888590067bf7f0682f Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Fri, 17 Dec 2021 12:55:32 -0600
Subject: [PATCH 36/51] less write locks in fragment.importRoaring/row

---
 ctl/server.go    |  2 +-
 fragment.go      | 75 ++++++++++++++++++++++++------------------------
 server.go        |  1 -
 server/config.go |  6 ++--
 server/server.go |  2 +-
 5 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/ctl/server.go b/ctl/server.go
index 83edb5456..c5da42847 100644
--- a/ctl/server.go
+++ b/ctl/server.go
@@ -85,7 +85,7 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) {
 	flags.BoolVar(&srv.Config.Storage.FsyncEnabled, "storage.fsync", true, "enable fsync fully safe flush-to-disk")
 
 	// RowcacheOn
-	flags.BoolVar((&srv.Config.RowcacheOn), "rowcache-on", srv.Config.RowcacheOn, "turn on the rowcache for all backends (may speed some queries)")
+	flags.BoolVar((&srv.Config.RowcacheOn), "rowcache-on", srv.Config.RowcacheOn, "Do not use, permanently disabled. Flag exists for backwards compatibility and will be removed.")
 
 	// RBF specific flags. See pilosa/rbf/cfg/cfg.go for definitions.
 	srv.Config.RBFConfig.DefineFlags(flags)
diff --git a/fragment.go b/fragment.go
index 83514edad..9b77f90f7 100644
--- a/fragment.go
+++ b/fragment.go
@@ -593,8 +593,8 @@ func (f *fragment) mutexCheck(tx Tx, details bool, limit int) (map[uint64][]uint
 
 // row returns a row by ID.
 func (f *fragment) row(tx Tx, rowID uint64) (*Row, error) {
-	f.mu.Lock()
-	defer f.mu.Unlock()
+	f.mu.RLock()
+	defer f.mu.RUnlock()
 	return f.unprotectedRow(tx, rowID)
 }
 
@@ -937,9 +937,12 @@ func (f *fragment) unprotectedClearRow(tx Tx, rowID uint64) (changed bool, err e
 	return changed, nil
 }
 
-// unprotectedClearBlock clears all rows for a given block.
+// clearBlock clears all rows for a given block.
 // This updates both the on-disk storage and the in-cache bitmap.
-func (f *fragment) unprotectedClearBlock(tx Tx, block int) (changed bool, err error) {
+func (f *fragment) clearBlock(tx Tx, block int) (changed bool, err error) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
 	firstRow := uint64(block * HashBlockSize)
 	var wp *io.Writer
 	if f.storage != nil {
@@ -2708,20 +2711,24 @@ func (f *fragment) importValue(tx Tx, columnIDs []uint64, values []int64, bitDep
 func (f *fragment) importRoaring(ctx context.Context, tx Tx, data []byte, clear bool) error {
 	span, ctx := tracing.StartSpanFromContext(ctx, "fragment.importRoaring")
 	defer span.Finish()
-	span, ctx = tracing.StartSpanFromContext(ctx, "importRoaring.AcquireFragmentLock")
-	f.mu.Lock()
-	defer f.mu.Unlock()
-	span.Finish()
 
-	return f.unprotectedImportRoaring(ctx, tx, data, clear)
+	rowSet, updateCache, err := f.doImportRoaring(ctx, tx, data, clear)
+	if err != nil {
+		return errors.Wrap(err, "doImportRoaring")
+	}
+	if updateCache {
+		return f.updateCachePostImport(ctx, rowSet)
+	}
+	return nil
 }
 
-func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []byte, clear bool) error {
+func (f *fragment) doImportRoaring(ctx context.Context, tx Tx, data []byte, clear bool) (map[uint64]int, bool, error) {
+	f.mu.RLock()
+	defer f.mu.RUnlock()
 	rowSize := uint64(1 << shardVsContainerExponent)
 	span, ctx := tracing.StartSpanFromContext(ctx, "importRoaring.ImportRoaringBits")
+	defer span.Finish()
 
-	useRowCache := storage.RowCacheEnabled()
-	var changed int
 	var rowSet map[uint64]int
 	var wp *io.Writer
 	if f.storage != nil {
@@ -2734,37 +2741,37 @@ func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []b
 			return err
 		}
 
-		changed, rowSet, err = tx.ImportRoaringBits(f.index(), f.field(), f.view(), f.shard, rit, clear, true, rowSize)
+		_, rowSet, err = tx.ImportRoaringBits(f.index(), f.field(), f.view(), f.shard, rit, clear, true, rowSize)
 		return err
 	})
 
-	span.Finish()
 	if err != nil {
-		return err
+		return nil, false, err
 	}
 
 	updateCache := f.CacheType != CacheTypeNone
+	return rowSet, updateCache, err
+}
+
+func (f *fragment) updateCachePostImport(ctx context.Context, rowSet map[uint64]int) error {
+	f.mu.Lock()
+	defer f.mu.Unlock()
 	anyChanged := false
 
 	for rowID, changes := range rowSet {
 		if changes == 0 {
 			continue
 		}
-		if useRowCache && f.rowCache != nil {
-			f.rowCache.Add(rowID, nil)
-		}
-		if updateCache {
-			anyChanged = true
-			if changes < 0 {
-				absChanges := uint64(-1 * changes)
-				if absChanges <= f.cache.Get(rowID) {
-					f.cache.BulkAdd(rowID, f.cache.Get(rowID)-absChanges)
-				} else {
-					f.cache.BulkAdd(rowID, 0)
-				}
+		anyChanged = true
+		if changes < 0 {
+			absChanges := uint64(-1 * changes)
+			if absChanges <= f.cache.Get(rowID) {
+				f.cache.BulkAdd(rowID, f.cache.Get(rowID)-absChanges)
 			} else {
-				f.cache.BulkAdd(rowID, f.cache.Get(rowID)+uint64(changes))
+				f.cache.BulkAdd(rowID, 0)
 			}
+		} else {
+			f.cache.BulkAdd(rowID, f.cache.Get(rowID)+uint64(changes))
 		}
 	}
 	// we only set this if we need to update the cache
@@ -2772,26 +2779,18 @@ func (f *fragment) unprotectedImportRoaring(ctx context.Context, tx Tx, data []b
 		f.cache.Invalidate()
 	}
 
-	span, _ = tracing.StartSpanFromContext(ctx, "importRoaring.incrementOpN")
-
-	f.incrementOpN(changed)
-
-	span.Finish()
 	return nil
 }
 
 // importRoaringOverwrite overwrites the specified block with the provided data.
 func (f *fragment) importRoaringOverwrite(ctx context.Context, tx Tx, data []byte, block int) error {
-	f.mu.Lock()
-	defer f.mu.Unlock()
-
 	// Clear the existing data from fragment block.
-	if _, err := f.unprotectedClearBlock(tx, block); err != nil {
+	if _, err := f.clearBlock(tx, block); err != nil {
 		return errors.Wrapf(err, "clearing block: %d", block)
 	}
 
 	// Union the new block data with the fragment data.
-	return f.unprotectedImportRoaring(ctx, tx, data, false)
+	return f.importRoaring(ctx, tx, data, false)
 }
 
 // incrementOpN increase the operation count by one.
diff --git a/server.go b/server.go
index 8858ab122..0c74a2fa5 100644
--- a/server.go
+++ b/server.go
@@ -476,7 +476,6 @@ func NewServer(opts ...ServerOption) (*Server, error) {
 	}
 	s.holder = NewHolder(path, s.holderConfig)
 	s.holder.Stats.SetLogger(s.logger)
-	s.holder.Logger.Infof("RowCacheOn: %v", s.holderConfig.RowcacheOn)
 	cwd, err := os.Getwd()
 	if err != nil {
 		return nil, err
diff --git a/server/config.go b/server/config.go
index c215d1596..09d82bfdb 100644
--- a/server/config.go
+++ b/server/config.go
@@ -200,9 +200,9 @@ type Config struct {
 	// "rbf".
 	Storage *storage.Config `toml:"storage"`
 
-	// RowcacheOn, if true, turns on the row cache for all storage backends.
-	// The default is now off because it makes rbf queries faster and uses
-	// much less memory.
+	// RowcacheOn permanently disabled. No longer useful w/ RBF. Left
+	// for backward compatibility but will be removed in a future
+	// version.
 	RowcacheOn bool `toml:"rowcache-on"`
 
 	// RBFConfig defines all externally configurable RBF flags.
diff --git a/server/server.go b/server/server.go
index a6d0049ae..b373d9e35 100644
--- a/server/server.go
+++ b/server/server.go
@@ -482,7 +482,7 @@ func (m *Command) SetupServer() error {
 		pilosa.OptServerClusterName(m.Config.Cluster.Name),
 		pilosa.OptServerSerializer(proto.Serializer{}),
 		pilosa.OptServerStorageConfig(m.Config.Storage),
-		pilosa.OptServerRowcacheOn(m.Config.RowcacheOn),
+		pilosa.OptServerRowcacheOn(false),
 		pilosa.OptServerRBFConfig(m.Config.RBFConfig),
 		pilosa.OptServerMaxQueryMemory(m.Config.MaxQueryMemory),
 		pilosa.OptServerQueryHistoryLength(m.Config.QueryHistoryLength),

From 07998622667818811715d993c1f220820a751435 Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Fri, 17 Dec 2021 14:50:08 -0600
Subject: [PATCH 37/51] move some locks, nbd

---
 rbf/db.go | 4 +++-
 rbf/tx.go | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index e5a81690e..f6b9ce5bc 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -380,6 +380,9 @@ func (db *DB) checkpoint() (err error) {
 
 	db.afterCurrentTx(func() {
 		defer db.rwmu.Unlock()
+		db.baseWALID = readMetaWALID(db.data)
+		db.mu.Unlock()
+		defer db.mu.Lock()
 
 		if err = db.walFile.Truncate(0); err != nil {
 			db.logger.Errorf("truncate wal file: %w", err)
@@ -389,7 +392,6 @@ func (db *DB) checkpoint() (err error) {
 			db.logger.Errorf("seek wal file: %w", err)
 		}
 
-		db.baseWALID = readMetaWALID(db.data)
 	})
 
 	return nil
diff --git a/rbf/tx.go b/rbf/tx.go
index bfb8fc00c..bceabd8f1 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -102,8 +102,6 @@ func (tx *Tx) Commit() error {
 
 	// If any pages have been written, ensure we write a new meta page with
 	// the commit flag to mark the end of the transaction.
-	tx.db.mu.Lock()
-	defer tx.db.mu.Unlock()
 	if tx.dirty() {
 		if err := tx.flush(); err != nil {
 			return err
@@ -120,11 +118,15 @@ func (tx *Tx) Commit() error {
 		// the lock, because we need removeTx to grab the lock to
 		// work, but if it wants to checkpoint, it wants to be able to return
 		// to us here and still be holding the lock.
+		tx.db.mu.Lock()
 		tx.db.rootRecords = tx.rootRecords
 		tx.db.pageMap = tx.pageMap
 		tx.db.walPageN = tx.walPageN
+		tx.db.mu.Unlock()
 	}
 
+	tx.db.mu.Lock()
+	defer tx.db.mu.Unlock()
 	// Disconnect transaction from DB.
 	return tx.db.removeTx(tx)
 }

From f05f1d0de27bae969d31d4364b7bedd85e007593 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Fri, 17 Dec 2021 15:51:30 -0600
Subject: [PATCH 38/51] added more authz functionality

---
 authz/{authz.go => authorization.go}          | 31 ++++++-
 .../{authz_test.go => authorization_test.go}  | 93 ++++++++++++++++++-
 server/server.go                              | 31 +++++++
 3 files changed, 151 insertions(+), 4 deletions(-)
 rename authz/{authz.go => authorization.go} (75%)
 rename authz/{authz_test.go => authorization_test.go} (68%)

diff --git a/authz/authz.go b/authz/authorization.go
similarity index 75%
rename from authz/authz.go
rename to authz/authorization.go
index 42c3bba2f..7f311899d 100644
--- a/authz/authz.go
+++ b/authz/authorization.go
@@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 			if perm, ok := p.Permissions[group.GroupID][index]; ok {
 				allPermissions[perm] = true
 			} else {
-				return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index)
+				return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index)
 			}
 		} else {
 			groupsDenied = append(groupsDenied, group.GroupID)
@@ -99,7 +99,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 	}
 
 	if len(groupsDenied) == len(groups) {
-		return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied)
+		return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied)
 	}
 
 	if allPermissions["admin"] {
@@ -112,3 +112,30 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 		return "", fmt.Errorf("no permissions found")
 	}
 }
+
+func (p *GroupPermissions) IsAdmin(groups []Group) bool {
+	for _, group := range groups {
+		if _, ok := p.Permissions[group.GroupID]; ok {
+			for _, permission := range p.Permissions[group.GroupID] {
+				if permission == "admin" {
+					return true
+				}
+			}
+		}
+	}
+	return false
+}
+
+func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) {
+
+	for _, group := range groups {
+		if _, ok := p.Permissions[group.GroupID]; ok {
+			for index, permission := range p.Permissions[group.GroupID] {
+				if permission == desiredPermission {
+					indexList = append(indexList, index)
+				}
+			}
+		}
+	}
+	return indexList
+}
diff --git a/authz/authz_test.go b/authz/authorization_test.go
similarity index 68%
rename from authz/authz_test.go
rename to authz/authorization_test.go
index fcec43185..84db21ab5 100644
--- a/authz/authz_test.go
+++ b/authz/authorization_test.go
@@ -23,6 +23,7 @@ import (
 )
 
 func TestAuth_ReadPermissionsFile(t *testing.T) {
+
 	singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b":
   "test": "read"`
 
@@ -67,6 +68,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) {
 }
 
 func TestAuth_GetPermissions(t *testing.T) {
+
 	// initializes different example of permissions file in yaml
 	permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b":
   "test": "read"`
@@ -112,14 +114,14 @@ func TestAuth_GetPermissions(t *testing.T) {
 			groupsList3,
 			"test1",
 			"",
-			"NOT allowed access to index",
+			"does not have permission to index",
 		},
 		{
 			permissions2,
 			groupsList2,
 			"test",
 			"",
-			"NOT allowed access to FeatureBase",
+			"does not have permission to FeatureBase",
 		},
 		{
 			permissions1,
@@ -176,3 +178,90 @@ func TestAuth_GetPermissions(t *testing.T) {
 		})
 	}
 }
+
+func TestAuth_IsAdmin(t *testing.T) {
+
+	group := []authz.Group{
+		{"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
+	}
+
+	groupPermissions1 := map[string]map[string]string{
+		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"},
+	}
+
+	groupPermissions2 := map[string]map[string]string{
+		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
+	}
+
+	tests := []struct {
+		groups           []authz.Group
+		groupPermissions map[string]map[string]string
+		output           bool
+	}{
+		{
+			group, groupPermissions1, true,
+		},
+		{
+			group, groupPermissions2, false,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			p := authz.GroupPermissions{test.groupPermissions}
+			resp := p.IsAdmin(test.groups)
+			if resp != test.output {
+				t.Errorf("expected %t, but got %t", test.output, resp)
+			}
+		})
+	}
+}
+
+func TestAuth_GetAuthorizedIndexList(t *testing.T) {
+
+	group := []authz.Group{
+		{"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
+	}
+
+	p := authz.GroupPermissions{map[string]map[string]string{
+		"dca35310-ecda-4f23-86cd-876aee55906b": {
+			"test1": "admin",
+			"test2": "read",
+			"test3": "read",
+		},
+	}}
+
+	tests := []struct {
+		groups     []authz.Group
+		permission string
+		output     []string
+	}{
+		{
+			group,
+			"read",
+			[]string{"test2", "test3"},
+		},
+		{
+			group,
+			"admin",
+			[]string{"test1"},
+		},
+		{
+			group,
+			"write",
+			nil,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+
+			indexList := p.GetAuthorizedIndexList(test.groups, test.permission)
+
+			if !reflect.DeepEqual(indexList, test.output) {
+				t.Errorf("expected %s, but got %s", test.output, indexList)
+			}
+		})
+	}
+
+}
diff --git a/server/server.go b/server/server.go
index c2418630b..17ea28933 100644
--- a/server/server.go
+++ b/server/server.go
@@ -237,6 +237,37 @@ func (m *Command) Start() (err error) {
 		if err = p.ReadPermissionsFile(permsFile); err != nil {
 			return err
 		}
+
+		groups := []authz.Group{
+			{
+				UserID:    "user-id",
+				GroupID:   "dca35310-ecda-4f23-86cd-876aee55906b",
+				GroupName: "group-name",
+			},
+			// {
+			// 	UserID:    "user-id",
+			// 	GroupID:   "dca35310-ecda-4f23-86cd-876aee559900",
+			// 	GroupName: "group-name",
+			// },
+		}
+
+		index := "test"
+
+		perm, err := p.GetPermissions(groups, index)
+		fmt.Printf("\nuser has %s access to index %s\n", perm, index)
+		if err != nil {
+			fmt.Printf("\np: %s, err: %s\n", perm, err.Error())
+		}
+
+		adminAccess := p.IsAdmin(groups)
+		fmt.Printf("\nAdminAccess: %t\n", adminAccess)
+
+		accessList := []string{"read", "write", "admin"}
+		for _, a := range accessList {
+			indexList := p.GetAuthorizedIndexList(groups, a)
+			fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList)
+		}
+
 	}
 
 	// Initialize server.

From a606bd030a09ac65cbc8cedba80533650f710260 Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Fri, 17 Dec 2021 16:46:07 -0600
Subject: [PATCH 39/51] addressed reviewer's feedback

---
 authz/authorization.go      |  8 ++++++--
 authz/authorization_test.go |  8 +++++---
 server/config.go            | 16 ++++------------
 server/server.go            | 31 -------------------------------
 4 files changed, 15 insertions(+), 48 deletions(-)

diff --git a/authz/authorization.go b/authz/authorization.go
index 7f311899d..f89a52ea6 100644
--- a/authz/authorization.go
+++ b/authz/authorization.go
@@ -70,7 +70,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error)
 		return fmt.Errorf("unmarshalling permissions failed with error: %s", err)
 	}
 
-	return nil
+	return
 }
 
 func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) {
@@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 			if perm, ok := p.Permissions[group.GroupID][index]; ok {
 				allPermissions[perm] = true
 			} else {
-				return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index)
+				return "", fmt.Errorf("user %s does not have permission to index %s", group.UserID, index)
 			}
 		} else {
 			groupsDenied = append(groupsDenied, group.GroupID)
@@ -133,6 +133,10 @@ func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermiss
 			for index, permission := range p.Permissions[group.GroupID] {
 				if permission == desiredPermission {
 					indexList = append(indexList, index)
+				} else if permission == "admin" {
+					indexList = append(indexList, index)
+				} else if permission == "write" && desiredPermission == "read" {
+					indexList = append(indexList, index)
 				}
 			}
 		}
diff --git a/authz/authorization_test.go b/authz/authorization_test.go
index 84db21ab5..cd985f973 100644
--- a/authz/authorization_test.go
+++ b/authz/authorization_test.go
@@ -16,6 +16,7 @@ package authz_test
 import (
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 	"testing"
 
@@ -227,7 +228,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 		"dca35310-ecda-4f23-86cd-876aee55906b": {
 			"test1": "admin",
 			"test2": "read",
-			"test3": "read",
+			"test3": "write",
 		},
 	}}
 
@@ -239,7 +240,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 		{
 			group,
 			"read",
-			[]string{"test2", "test3"},
+			[]string{"test1", "test2", "test3"},
 		},
 		{
 			group,
@@ -249,7 +250,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 		{
 			group,
 			"write",
-			nil,
+			[]string{"test1", "test3"},
 		},
 	}
 
@@ -257,6 +258,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 
 			indexList := p.GetAuthorizedIndexList(test.groups, test.permission)
+			sort.Strings(indexList)
 
 			if !reflect.DeepEqual(indexList, test.output) {
 				t.Errorf("expected %s, but got %s", test.output, indexList)
diff --git a/server/config.go b/server/config.go
index 866ffb3ff..5f4f0573c 100644
--- a/server/config.go
+++ b/server/config.go
@@ -601,7 +601,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin
 
 func (c *Config) ValidateAuth() (errors []error) {
 	if !c.Auth.Enable {
-		return errors
+		return
 	}
 	authConfig := map[string]string{
 		"ClientId":         c.Auth.ClientId,
@@ -626,11 +626,7 @@ func (c *Config) ValidateAuth() (errors []error) {
 			}
 		}
 	}
-
-	if len(errors) > 0 {
-		return errors
-	}
-	return nil
+	return errors
 }
 
 func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) {
@@ -667,11 +663,7 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) {
 			}
 		}
 	}
-	if len(errors) > 0 {
-		return errors
-	}
-
-	return nil
+	return errors
 }
 
 func (c *Config) ValidatePermissionsFile() (err error) {
@@ -684,7 +676,7 @@ func (c *Config) ValidatePermissionsFile() (err error) {
 	if (fileExt != ".yaml") && (fileExt != ".yml") {
 		return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile)
 	}
-	return nil
+	return
 }
 
 func (c *Config) MustValidateAuth() {
diff --git a/server/server.go b/server/server.go
index 17ea28933..c2418630b 100644
--- a/server/server.go
+++ b/server/server.go
@@ -237,37 +237,6 @@ func (m *Command) Start() (err error) {
 		if err = p.ReadPermissionsFile(permsFile); err != nil {
 			return err
 		}
-
-		groups := []authz.Group{
-			{
-				UserID:    "user-id",
-				GroupID:   "dca35310-ecda-4f23-86cd-876aee55906b",
-				GroupName: "group-name",
-			},
-			// {
-			// 	UserID:    "user-id",
-			// 	GroupID:   "dca35310-ecda-4f23-86cd-876aee559900",
-			// 	GroupName: "group-name",
-			// },
-		}
-
-		index := "test"
-
-		perm, err := p.GetPermissions(groups, index)
-		fmt.Printf("\nuser has %s access to index %s\n", perm, index)
-		if err != nil {
-			fmt.Printf("\np: %s, err: %s\n", perm, err.Error())
-		}
-
-		adminAccess := p.IsAdmin(groups)
-		fmt.Printf("\nAdminAccess: %t\n", adminAccess)
-
-		accessList := []string{"read", "write", "admin"}
-		for _, a := range accessList {
-			indexList := p.GetAuthorizedIndexList(groups, a)
-			fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList)
-		}
-
 	}
 
 	// Initialize server.

From 41f6156bda7e7e21e37e2ca65f1675c03560e688 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 17:57:33 -0600
Subject: [PATCH 40/51] don't use write Tx even when we're using the expensive
 logic for write Tx

---
 txfactory.go | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/txfactory.go b/txfactory.go
index 62f527653..12fa12981 100644
--- a/txfactory.go
+++ b/txfactory.go
@@ -242,10 +242,15 @@ func (qcx *Qcx) GetTx(o Txo) (tx Tx, finisher func(perr *error), err error) {
 	}
 
 	// qcx.write reflects the top executor determination
-	// if a write will be done at the end, so we upgrade
-	// the "local" read Tx to be writes, so that they
-	// don't deadlock against themselves.
-	o.Write = o.Write || qcx.write
+	// if a write will be happen at some point, in which case, to avoid
+	// locking problems with multi-shard things, we (probably incorrectly)
+	// treat every Tx as its own individual separate Tx.
+	//
+	// But we still want to open non-write transactions individually, we
+	// just can't recycle them (because write operations will come in and
+	// we want them to work and commit right away so we're not holding a write
+	// lock for long).
+	writeLogic := o.Write || qcx.write
 
 	// In general, we make ALL write transactions local, and never reuse them
 	// below. Previously this was to help lmdb.
@@ -273,7 +278,7 @@ func (qcx *Qcx) GetTx(o Txo) (tx Tx, finisher func(perr *error), err error) {
 		return *qcx.RequiredForAtomicWriteTx, NoopFinisher, nil
 	}
 
-	if !o.Write && qcx.Grp != nil {
+	if !writeLogic && qcx.Grp != nil {
 		// read, with a group in place.
 		finisher = func(perr *error) {} // finisher is a returned value
 

From 9945575bf111f3193ed02212dd4ddc109999a0c0 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 21:21:03 -0600
Subject: [PATCH 41/51] create a new worker every so often if progress isn't
 happening

this is very approximate and may be a mess and may be unbounded, but
in practice i think it should be okay. if it's not we'll have an
adventure.
---
 executor.go | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/executor.go b/executor.go
index 8b0409eb7..899ac0e19 100644
--- a/executor.go
+++ b/executor.go
@@ -51,6 +51,9 @@ type executor struct {
 	Node    *topology.Node
 	Cluster *cluster
 
+	// how many jobs the work queue has seen
+	workCounter uint64
+
 	// Client used for remote requests.
 	client InternalQueryClient
 
@@ -128,15 +131,47 @@ func newExecutor(opts ...executorOption) *executor {
 	e.work = make(chan job, e.workerPoolSize)
 	_ = testhook.Opened(NewAuditor(), e, nil)
 	for i := 0; i < e.workerPoolSize; i++ {
-		e.workersWG.Add(1)
-		go func() {
-			defer e.workersWG.Done()
-			worker(e.work)
-		}()
+		e.addWorker()
 	}
+	go func() {
+		// background task: every so often, check to see whether we have
+		// work in the queue but none has been taken for a while. if so, we
+		// need more workers.
+		prev := atomic.LoadUint64(&e.workCounter)
+		periodic := time.NewTicker(50 * time.Millisecond)
+		defer periodic.Stop()
+		running := true
+		for running {
+			<-periodic.C
+			func() {
+				e.workMu.Lock()
+				defer e.workMu.Unlock()
+				if e.shutdown {
+					running = false
+					return
+				}
+				if len(e.work) == 0 {
+					return
+				}
+				next := atomic.LoadUint64(&e.workCounter)
+				if next == prev {
+					e.addWorker()
+					prev = next
+				}
+			}()
+		}
+	}()
 	return e
 }
 
+func (e *executor) addWorker() {
+	e.workersWG.Add(1)
+	go func() {
+		defer e.workersWG.Done()
+		e.worker(e.work)
+	}()
+}
+
 func (e *executor) Close() error {
 	e.workMu.Lock()
 	defer e.workMu.Unlock()
@@ -5935,8 +5970,9 @@ type job struct {
 	resultChan      chan mapResponse
 }
 
-func worker(work chan job) {
+func (e *executor) worker(work chan job) {
 	for j := range work {
+		atomic.AddUint64(&e.workCounter, 1)
 		// Skip out early if the context is done, but still send
 		// an ack so mapperLocal can be sure we aren't about to
 		// work on something it sent us.

From 8f217ab099f4a6a3952b15ba190c6311133c6f39 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 22:05:41 -0600
Subject: [PATCH 42/51] scale down worker pool when it's large

if we have more than twice our starting worker pool, and have had no
tasks when checking the queue for multiple rounds, send a job telling
the system to retire a worker. eventually we'll get down to about 2x
the starting pool size if we stay idle.
---
 executor.go | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/executor.go b/executor.go
index 899ac0e19..be4def11a 100644
--- a/executor.go
+++ b/executor.go
@@ -64,6 +64,7 @@ type executor struct {
 	workMu         sync.RWMutex
 	workersWG      sync.WaitGroup
 	workerPoolSize int
+	currentWorkers int
 	work           chan job
 
 	// Maximum per-request memory usage (Extract() only)
@@ -141,6 +142,7 @@ func newExecutor(opts ...executorOption) *executor {
 		periodic := time.NewTicker(50 * time.Millisecond)
 		defer periodic.Stop()
 		running := true
+		idle := 0
 		for running {
 			<-periodic.C
 			func() {
@@ -151,6 +153,17 @@ func newExecutor(opts ...executorOption) *executor {
 					return
 				}
 				if len(e.work) == 0 {
+					idle++
+					if idle > 10 && e.currentWorkers > (e.workerPoolSize*2) {
+						select {
+						case e.work <- job{idleHands: true}:
+							// we closed an excess worker
+						default:
+							// somehow between our test above and now the work
+							// queue FILLED UP and we stoically accept this
+						}
+					}
+					idle = 0
 					return
 				}
 				next := atomic.LoadUint64(&e.workCounter)
@@ -166,9 +179,11 @@ func newExecutor(opts ...executorOption) *executor {
 
 func (e *executor) addWorker() {
 	e.workersWG.Add(1)
+	e.currentWorkers++
 	go func() {
 		defer e.workersWG.Done()
 		e.worker(e.work)
+		e.currentWorkers--
 	}()
 }
 
@@ -5968,11 +5983,15 @@ type job struct {
 	ctx             context.Context
 	memoryAvailable *int64 // shared, atomic value
 	resultChan      chan mapResponse
+	idleHands       bool
 }
 
 func (e *executor) worker(work chan job) {
 	for j := range work {
 		atomic.AddUint64(&e.workCounter, 1)
+		if j.idleHands {
+			return
+		}
 		// Skip out early if the context is done, but still send
 		// an ack so mapperLocal can be sure we aren't about to
 		// work on something it sent us.

From 9a2a8f964c99f177697661e9c98dad59eb968ff5 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 22:22:27 -0600
Subject: [PATCH 43/51] fix silly typo in worker pool downscaling

---
 executor.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/executor.go b/executor.go
index be4def11a..69bdbf15d 100644
--- a/executor.go
+++ b/executor.go
@@ -162,8 +162,8 @@ func newExecutor(opts ...executorOption) *executor {
 							// somehow between our test above and now the work
 							// queue FILLED UP and we stoically accept this
 						}
+						idle = 0
 					}
-					idle = 0
 					return
 				}
 				next := atomic.LoadUint64(&e.workCounter)

From 1439c316d348ac43d084e88d414e5899ecdfe9dd Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 22:25:01 -0600
Subject: [PATCH 44/51] read-only lock for check of shutdown

---
 executor.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/executor.go b/executor.go
index 69bdbf15d..45d5169c5 100644
--- a/executor.go
+++ b/executor.go
@@ -146,8 +146,8 @@ func newExecutor(opts ...executorOption) *executor {
 		for running {
 			<-periodic.C
 			func() {
-				e.workMu.Lock()
-				defer e.workMu.Unlock()
+				e.workMu.RLock()
+				defer e.workMu.RUnlock()
 				if e.shutdown {
 					running = false
 					return

From 8974014d5783dc61c6a850c03f00119194e10f1a Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Fri, 17 Dec 2021 22:48:40 -0600
Subject: [PATCH 45/51] too tired to be writing code

---
 executor.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/executor.go b/executor.go
index 45d5169c5..fb5ef68ce 100644
--- a/executor.go
+++ b/executor.go
@@ -169,8 +169,8 @@ func newExecutor(opts ...executorOption) *executor {
 				next := atomic.LoadUint64(&e.workCounter)
 				if next == prev {
 					e.addWorker()
-					prev = next
 				}
+				prev = next
 			}()
 		}
 	}()

From 7826c06eee6014cc1ec5b9079f94ed86c53e1dee Mon Sep 17 00:00:00 2001
From: Matthew Jaffee <jaffee@pilosa.com>
Date: Sat, 18 Dec 2021 08:58:19 -0600
Subject: [PATCH 46/51] use atomics for currentWorker to avoid race

---
 executor.go | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/executor.go b/executor.go
index fb5ef68ce..5e03e1a72 100644
--- a/executor.go
+++ b/executor.go
@@ -64,7 +64,7 @@ type executor struct {
 	workMu         sync.RWMutex
 	workersWG      sync.WaitGroup
 	workerPoolSize int
-	currentWorkers int
+	currentWorkers int64
 	work           chan job
 
 	// Maximum per-request memory usage (Extract() only)
@@ -154,7 +154,7 @@ func newExecutor(opts ...executorOption) *executor {
 				}
 				if len(e.work) == 0 {
 					idle++
-					if idle > 10 && e.currentWorkers > (e.workerPoolSize*2) {
+					if idle > 10 && atomic.LoadInt64(&e.currentWorkers) > int64(e.workerPoolSize*2) {
 						select {
 						case e.work <- job{idleHands: true}:
 							// we closed an excess worker
@@ -179,11 +179,11 @@ func newExecutor(opts ...executorOption) *executor {
 
 func (e *executor) addWorker() {
 	e.workersWG.Add(1)
-	e.currentWorkers++
+	atomic.AddInt64(&e.currentWorkers, 1)
 	go func() {
 		defer e.workersWG.Done()
 		e.worker(e.work)
-		e.currentWorkers--
+		atomic.AddInt64(&e.currentWorkers, -1)
 	}()
 }
 

From c14bd08213477c3fc6bc355b24774c39be0bd4cc Mon Sep 17 00:00:00 2001
From: Souhaila Noor <souhaila.noor@molecula.com>
Date: Sun, 19 Dec 2021 11:37:41 -0600
Subject: [PATCH 47/51] updated admin to be at the cluster level

---
 authz/authorization.go         |  33 ++++---
 authz/authorization_test.go    | 153 +++++++++++++++++++++------------
 server/config.go               |   9 +-
 server/config_internal_test.go |  32 +++++--
 4 files changed, 149 insertions(+), 78 deletions(-)

diff --git a/authz/authorization.go b/authz/authorization.go
index f89a52ea6..77bd67ade 100644
--- a/authz/authorization.go
+++ b/authz/authorization.go
@@ -49,7 +49,8 @@ type Auth struct {
 }
 
 type GroupPermissions struct {
-	Permissions map[string]map[string]string
+	Permissions map[string]map[string]string `yaml:"user-groups"`
+	Admin       string                       `yaml:"admin"`
 }
 
 type Group struct {
@@ -65,7 +66,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error)
 		return fmt.Errorf("reading permissions failed with error: %s", err)
 	}
 
-	err = yaml.UnmarshalStrict(permsData, &p.Permissions)
+	err = yaml.UnmarshalStrict(permsData, &p)
 	if err != nil {
 		return fmt.Errorf("unmarshalling permissions failed with error: %s", err)
 	}
@@ -75,8 +76,11 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error)
 
 func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) {
 
+	if admin := p.IsAdmin(groups); admin {
+		return "admin", nil
+	}
+
 	allPermissions := map[string]bool{
-		"admin": false,
 		"write": false,
 		"read":  false,
 	}
@@ -102,9 +106,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 		return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied)
 	}
 
-	if allPermissions["admin"] {
-		return "admin", nil
-	} else if allPermissions["write"] {
+	if allPermissions["write"] {
 		return "write", nil
 	} else if allPermissions["read"] {
 		return "read", nil
@@ -115,26 +117,29 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss
 
 func (p *GroupPermissions) IsAdmin(groups []Group) bool {
 	for _, group := range groups {
-		if _, ok := p.Permissions[group.GroupID]; ok {
-			for _, permission := range p.Permissions[group.GroupID] {
-				if permission == "admin" {
-					return true
-				}
-			}
+		if p.Admin == group.GroupID {
+			return true
 		}
 	}
 	return false
 }
 
 func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) {
+	// if user is admin, find all indexes in permissions file and return them
+	if admin := p.IsAdmin(groups); admin {
+		for groupId := range p.Permissions {
+			for index := range p.Permissions[groupId] {
+				indexList = append(indexList, index)
+			}
+		}
+		return indexList
+	}
 
 	for _, group := range groups {
 		if _, ok := p.Permissions[group.GroupID]; ok {
 			for index, permission := range p.Permissions[group.GroupID] {
 				if permission == desiredPermission {
 					indexList = append(indexList, index)
-				} else if permission == "admin" {
-					indexList = append(indexList, index)
 				} else if permission == "write" && desiredPermission == "read" {
 					indexList = append(indexList, index)
 				}
diff --git a/authz/authorization_test.go b/authz/authorization_test.go
index cd985f973..dab33dbe1 100644
--- a/authz/authorization_test.go
+++ b/authz/authorization_test.go
@@ -25,29 +25,39 @@ import (
 
 func TestAuth_ReadPermissionsFile(t *testing.T) {
 
-	singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b":
-  "test": "read"`
+	singleInput := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee55906b":
+    "test": "read"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b":
-  "test": "read"
-"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": "admin"`
+	multiInput := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee55906b":
+    "test": "read"
+    "test2": "write"
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": "write"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	singleStruct := map[string]map[string]string{
-		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
+	singlePermission := authz.GroupPermissions{
+		Permissions: map[string]map[string]string{
+			"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
+		},
+		Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe",
 	}
 
-	multiStruct := map[string]map[string]string{
-		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
-		"dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"},
+	multiPermission := authz.GroupPermissions{
+		Permissions: map[string]map[string]string{
+			"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read", "test2": "write"},
+			"dca35310-ecda-4f23-86cd-876aee559900": {"test": "write"}},
+		Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe",
 	}
 
 	tests := []struct {
 		input  string
-		output map[string]map[string]string
+		output authz.GroupPermissions
 	}{
-		{singleInput, singleStruct},
-		{multiInput, multiStruct},
+		{singleInput, singlePermission},
+		{multiInput, multiPermission},
 	}
 
 	for i, test := range tests {
@@ -60,8 +70,8 @@ func TestAuth_ReadPermissionsFile(t *testing.T) {
 				t.Fatalf("readPermissionsFile error: %s", err)
 			}
 
-			if !reflect.DeepEqual(p.Permissions, test.output) {
-				t.Fatalf("expected output %s, but got %s", test.output, p.Permissions)
+			if !reflect.DeepEqual(p, test.output) {
+				t.Fatalf("expected output %s, but got %s", test.output, p)
 			}
 		},
 		)
@@ -71,20 +81,28 @@ func TestAuth_ReadPermissionsFile(t *testing.T) {
 func TestAuth_GetPermissions(t *testing.T) {
 
 	// initializes different example of permissions file in yaml
-	permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b":
-  "test": "read"`
+	permissions1 := `"user-groups":
+  "dca35310-ecda-4f23-86cd-876aee55906b":
+    "test": "read"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": "write"`
+	permissions2 := `"user-groups":
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": "write"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b":
-  "test": "write"
-  "test2": "read"
-"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": "admin"`
+	permissions3 := `"user-groups":
+  "dca35310-ecda-4f23-86cd-876aee55906b":
+    "test": "write"
+    "test2": "read"
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": "read"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": ""`
+	permissions4 := `"user-groups":
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": ""
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
 	// initializes groups that are returned from identity provider
 	groupName := "name"
@@ -95,6 +113,7 @@ func TestAuth_GetPermissions(t *testing.T) {
 		{userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName},
 		{userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName},
 	}
+	groupsList4 := []authz.Group{{userId, "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", groupName}}
 
 	tests := []struct {
 		yamlData   string
@@ -140,7 +159,7 @@ func TestAuth_GetPermissions(t *testing.T) {
 		},
 		{
 			permissions3,
-			groupsList3,
+			groupsList4,
 			"test",
 			"admin",
 			"",
@@ -182,34 +201,37 @@ func TestAuth_GetPermissions(t *testing.T) {
 
 func TestAuth_IsAdmin(t *testing.T) {
 
-	group := []authz.Group{
-		{"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
+	group1 := []authz.Group{
+		{"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"},
 	}
 
-	groupPermissions1 := map[string]map[string]string{
-		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"},
+	group2 := []authz.Group{
+		{"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
 	}
 
-	groupPermissions2 := map[string]map[string]string{
-		"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"},
+	groupPermissions := authz.GroupPermissions{
+		Permissions: map[string]map[string]string{
+			"dca35310-ecda-4f23-86cd-876aee55906b": {"test": "write"},
+		},
+		Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe",
 	}
 
 	tests := []struct {
 		groups           []authz.Group
-		groupPermissions map[string]map[string]string
+		groupPermissions authz.GroupPermissions
 		output           bool
 	}{
 		{
-			group, groupPermissions1, true,
+			group1, groupPermissions, true,
 		},
 		{
-			group, groupPermissions2, false,
+			group2, groupPermissions, false,
 		},
 	}
 
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
-			p := authz.GroupPermissions{test.groupPermissions}
+			p := test.groupPermissions
 			resp := p.IsAdmin(test.groups)
 			if resp != test.output {
 				t.Errorf("expected %t, but got %t", test.output, resp)
@@ -220,17 +242,30 @@ func TestAuth_IsAdmin(t *testing.T) {
 
 func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 
-	group := []authz.Group{
-		{"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
+	group1 := []authz.Group{
+		{"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"},
 	}
 
-	p := authz.GroupPermissions{map[string]map[string]string{
-		"dca35310-ecda-4f23-86cd-876aee55906b": {
-			"test1": "admin",
-			"test2": "read",
-			"test3": "write",
+	group2 := []authz.Group{
+		{"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"},
+	}
+
+	group3 := []authz.Group{
+		{"user-id", "dca35310-ecda-4f23-86cd-876aee559900", "group-name"},
+	}
+
+	p := authz.GroupPermissions{
+		Permissions: map[string]map[string]string{
+			"dca35310-ecda-4f23-86cd-876aee55906b": {
+				"test1": "read",
+				"test2": "write",
+			},
+			"dca35310-ecda-4f23-86cd-876aee559900": {
+				"test3": "read",
+			},
 		},
-	}}
+		Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe",
+	}
 
 	tests := []struct {
 		groups     []authz.Group
@@ -238,19 +273,29 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) {
 		output     []string
 	}{
 		{
-			group,
+			group1,
+			"read",
+			[]string{"test1", "test2"},
+		},
+		{
+			group1,
+			"write",
+			[]string{"test2"},
+		},
+		{
+			group3,
+			"write",
+			nil,
+		},
+		{
+			group2,
 			"read",
 			[]string{"test1", "test2", "test3"},
 		},
 		{
-			group,
-			"admin",
-			[]string{"test1"},
-		},
-		{
-			group,
+			group2,
 			"write",
-			[]string{"test1", "test3"},
+			[]string{"test1", "test2", "test3"},
 		},
 	}
 
diff --git a/server/config.go b/server/config.go
index 5f4f0573c..807e19b7f 100644
--- a/server/config.go
+++ b/server/config.go
@@ -657,12 +657,17 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) {
 				continue
 			}
 
-			if !((perm == "admin") || (perm == "write") || (perm == "read")) {
-				errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile))
+			if !((perm == "write") || (perm == "read")) {
+				errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s; expected permissions are read or write", perm, groupId, index, c.Auth.PermissionsFile))
 				continue
 			}
 		}
 	}
+
+	if p.Admin == "" {
+		errors = append(errors, fmt.Errorf("empty string for admin in permissions file: %s", c.Auth.PermissionsFile))
+	}
+
 	return errors
 }
 
diff --git a/server/config_internal_test.go b/server/config_internal_test.go
index 48ba081db..11148dfad 100644
--- a/server/config_internal_test.go
+++ b/server/config_internal_test.go
@@ -386,17 +386,29 @@ func TestConfig_validateAuth(t *testing.T) {
 func TestConfig_validatePermissions(t *testing.T) {
 	permissions0 := ``
 
-	permissions1 := `"":
-  "test": "read"`
+	permissions1 := `user-groups:
+  "":
+    "test": "read"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900":
-  "": "write"`
+	permissions2 := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "": "write"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": ""`
+	permissions3 := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": ""
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
 
-	permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900":
-  "test": "readwrite"`
+	permissions4 := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": "readwrite"
+admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"`
+
+	permissions5 := `user-groups:
+  "dca35310-ecda-4f23-86cd-876aee559900":
+    "test": "read"`
 
 	tests := []struct {
 		err   string
@@ -422,6 +434,10 @@ func TestConfig_validatePermissions(t *testing.T) {
 			"not a valid permission",
 			permissions4,
 		},
+		{
+			"empty string for admin in permissions file",
+			permissions5,
+		},
 	}
 
 	for i, test := range tests {

From 977a699a98c141a65de5f407d17f03d793cddfcf Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Mon, 20 Dec 2021 10:58:41 -0600
Subject: [PATCH 48/51] don't panic on invalid page type

debugging tools shouldn't panic when they encounter bugs. insert
"you had one job" meme.
---
 ctl/rbf_pages.go | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ctl/rbf_pages.go b/ctl/rbf_pages.go
index 2e831892a..ca3484f31 100644
--- a/ctl/rbf_pages.go
+++ b/ctl/rbf_pages.go
@@ -69,9 +69,9 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 
 	// Print one line for each page.
 	for pgno, info := range infos {
+		fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 		switch info := info.(type) {
 		case *rbf.MetaPageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "meta")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", "")
@@ -79,7 +79,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "pageN=%d,walid=%d,rootrec=%d,freelist=%d\n", info.PageN, info.WALID, info.RootRecordPageNo, info.FreelistPageNo)
 
 		case *rbf.RootRecordPageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "rootrec")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", "")
@@ -87,7 +86,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "next=%d\n", info.Next)
 
 		case *rbf.LeafPageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "leaf")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree))
@@ -95,7 +93,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "flags=x%x,celln=%d\n", info.Flags, info.CellN)
 
 		case *rbf.BranchPageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "branch")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree))
@@ -103,7 +100,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "flags=x%x,celln=%d\n", info.Flags, info.CellN)
 
 		case *rbf.BitmapPageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "bitmap")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", prefixToString(info.Tree))
@@ -111,7 +107,6 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "-\n")
 
 		case *rbf.FreePageInfo:
-			fmt.Fprintf(cmd.Stdout, "%-8d ", pgno)
 			fmt.Fprintf(cmd.Stdout, "%-10s ", "free")
 			if cmd.WithTree {
 				fmt.Fprintf(cmd.Stdout, "%-30q ", "")
@@ -119,7 +114,7 @@ func (cmd *RBFPagesCommand) Run(ctx context.Context) error {
 			fmt.Fprintf(cmd.Stdout, "-\n")
 
 		default:
-			panic(fmt.Sprintf("unexpected page info type %T", info))
+			fmt.Fprintf(cmd.Stdout, "unknown [%T]\n", info)
 		}
 	}
 

From 5f8a2819187aee3afb6b46b534d6699549792801 Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Mon, 20 Dec 2021 09:38:16 -0700
Subject: [PATCH 49/51] Fix RBF multi-level branch delete

This commit fixes a bug in RBF where deleting all the elements in
a bitmap that has a depth greater than 2 will cause the root bitmap
to be a branch page with a cell count of zero. This breaks an
assertion in `readBranchCell()` which causes a panic post-commit.

A new assertion has been added to prevent a branch page from being
written with a zero count in the future.
---
 rbf/cursor.go  | 22 ++++++++++++++++++++++
 rbf/tx_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/rbf/cursor.go b/rbf/cursor.go
index 68c165d94..eda6bec1d 100644
--- a/rbf/cursor.go
+++ b/rbf/cursor.go
@@ -774,6 +774,25 @@ func (c *Cursor) deleteBranchCell(stackIndex int, key uint64) (err error) {
 	cells[len(cells)-1] = branchCell{}
 	cells = cells[:len(cells)-1]
 
+	// Branches are not allowed to have zero element so we must remove the page
+	// or, in the case of the root page, convert to a leaf page.
+	if len(cells) == 0 {
+		// If this is the root page, convert to leaf page.
+		if stackIndex == 0 {
+			var buf [PageSize]byte
+			writePageNo(buf[:], elem.pgno)
+			writeFlags(buf[:], PageTypeLeaf)
+			writeCellN(buf[:], len(cells))
+			return c.tx.writePage(buf[:])
+		}
+
+		// If this is a non-root page, free and remove from parent.
+		if err := c.tx.freePgno(elem.pgno); err != nil {
+			return err
+		}
+		return c.deleteBranchCell(stackIndex-1, oldPageKey)
+	}
+
 	// If the root only has one node, replace it with its child.
 	if stackIndex == 0 && len(cells) == 1 {
 		target, _, err := c.tx.readPage(cells[0].ChildPgno)
@@ -802,6 +821,9 @@ func (c *Cursor) deleteBranchCell(stackIndex int, key uint64) (err error) {
 		writeBranchCell(buf[:], j, offset, cell)
 		offset += align8(branchCellSize)
 	}
+
+	assert(readCellN(buf[:]) > 0) // must have at least one cell
+
 	if err := c.tx.writePage(buf[:]); err != nil {
 		return err
 	}
diff --git a/rbf/tx_test.go b/rbf/tx_test.go
index 1004437a3..f05db6626 100644
--- a/rbf/tx_test.go
+++ b/rbf/tx_test.go
@@ -433,6 +433,52 @@ func TestTx_DeallocateToFreeList(t *testing.T) {
 	}
 }
 
+func TestTx_Remove(t *testing.T) {
+	t.Parallel()
+
+	db := MustOpenDB(t)
+	defer MustCloseDB(t, db)
+
+	tx := MustBegin(t, db, true)
+	defer tx.Rollback()
+
+	if err := tx.CreateBitmap("x"); err != nil {
+		t.Fatal(err)
+	}
+
+	// Insert large array values.
+	var values []uint64
+	for i := 0; i < 1000; i++ {
+		for j := 0; j < rbf.ArrayMaxSize; j++ {
+			v := uint64((i << 16) + j)
+			values = append(values, v)
+
+			if _, err := tx.Add("x", v); err != nil {
+				t.Fatalf("Add(%d) err=%q", v, err)
+			}
+		}
+	}
+
+	if err := tx.Commit(); err != nil {
+		t.Fatal(err)
+	}
+
+	tx = MustBegin(t, db, true)
+	defer tx.Rollback()
+
+	// Remove all array values.
+	for _, i := range rand.Perm(len(values)) {
+		v := values[i]
+		if _, err := tx.Remove("x", v); err != nil {
+			t.Fatalf("Remove(%d) err=%q", v, err)
+		}
+	}
+
+	if err := tx.Commit(); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestTx_AddRemove_Quick(t *testing.T) {
 	if testing.Short() {
 		t.Skip("-short enabled, skipping")

From 6481b4eabe12e3c50ca7c8e735483349484223cb Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Mon, 20 Dec 2021 13:24:34 -0700
Subject: [PATCH 50/51] Add rbf check for empty branch pages

---
 rbf/db.go       |  8 +++++
 rbf/rbf_test.go |  9 +++++-
 rbf/tx.go       | 26 ++++++++++++++--
 rbf/tx_test.go  | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/rbf/db.go b/rbf/db.go
index f6b9ce5bc..65dd4fbea 100644
--- a/rbf/db.go
+++ b/rbf/db.go
@@ -265,6 +265,14 @@ func (db *DB) methodicalWALPageN(pageN int) (lastMeta int, err error) {
 	return lastMeta, nil
 }
 
+// Checkpoint performs a manual checkpoint. This is not necessary except for tests.
+func (db *DB) Checkpoint() error {
+	db.mu.Lock()
+	defer db.mu.Unlock()
+	db.rwmu.Lock()
+	return db.checkpoint()
+}
+
 // checkpoint moves all WAL pages to the main DB file. Must be called
 // while holding both db.mu and db.rwmu. Should release db.rwmu, but not
 // db.mu.
diff --git a/rbf/rbf_test.go b/rbf/rbf_test.go
index be3461e37..4071a1a95 100644
--- a/rbf/rbf_test.go
+++ b/rbf/rbf_test.go
@@ -86,7 +86,14 @@ func MustCloseDB(tb testing.TB, db *rbf.DB) {
 	tb.Helper()
 	if err := db.Check(); err != nil && err != rbf.ErrClosed {
 		tb.Fatal(err)
-	} else if n := db.TxN(); n != 0 {
+	}
+	MustCloseDBNoCheck(tb, db)
+}
+
+// MustCloseDBNoCheck closes db. On error, fail test.
+func MustCloseDBNoCheck(tb testing.TB, db *rbf.DB) {
+	tb.Helper()
+	if n := db.TxN(); n != 0 {
 		tb.Fatalf("db still has %d active transactions; must closed before closing db", n)
 	} else if err := db.Close(); err != nil && err != rbf.ErrClosed {
 		tb.Fatal(err)
diff --git a/rbf/tx.go b/rbf/tx.go
index bceabd8f1..5c6c7f7c6 100644
--- a/rbf/tx.go
+++ b/rbf/tx.go
@@ -741,6 +741,27 @@ func (tx *Tx) Check() error {
 	return nil
 }
 
+func (tx *Tx) checkPage(pgno, parent, typ uint32) error {
+	switch typ {
+	case PageTypeBranch:
+		return tx.checkBranchPage(pgno, parent, typ)
+	default:
+		return nil
+	}
+}
+
+func (tx *Tx) checkBranchPage(pgno, parent, typ uint32) error {
+	page, _, err := tx.readPage(pgno)
+	if err != nil {
+		return err
+	}
+
+	if readCellN(page) == 0 {
+		return fmt.Errorf("branch page %d is empty", pgno)
+	}
+	return nil
+}
+
 // checkPageAllocations ensures that all pages are either in-use or on the freelist.
 func (tx *Tx) checkPageAllocations() error {
 	freePageSet, err := tx.freePageSet()
@@ -830,7 +851,7 @@ func (tx *Tx) inusePageSet() (map[uint32]struct{}, error) {
 	// Traverse freelist and mark pages as in-use.
 	if err := tx.walkTree(readMetaFreelistPageNo(tx.meta[:]), 0, func(pgno, parent, typ uint32) error {
 		m[pgno] = struct{}{}
-		return nil
+		return tx.checkPage(pgno, parent, typ)
 	}); err != nil {
 		return m, err
 	}
@@ -846,7 +867,8 @@ func (tx *Tx) inusePageSet() (map[uint32]struct{}, error) {
 
 		if err := tx.walkTree(pgno.(uint32), 0, func(pgno, parent, typ uint32) error {
 			m[pgno] = struct{}{}
-			return nil
+
+			return tx.checkPage(pgno, parent, typ)
 		}); err != nil {
 			return m, err
 		}
diff --git a/rbf/tx_test.go b/rbf/tx_test.go
index 1004437a3..345031417 100644
--- a/rbf/tx_test.go
+++ b/rbf/tx_test.go
@@ -2,8 +2,11 @@
 package rbf_test
 
 import (
+	"encoding/binary"
 	"fmt"
 	"math/rand"
+	"os"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -770,3 +773,83 @@ func TestTx_DeleteBitmapsWithPrefix(t *testing.T) {
 	checkInfos()
 
 }
+
+func TestTx_Check(t *testing.T) {
+	t.Run("EmptyBranchPage", func(t *testing.T) {
+		t.Parallel()
+
+		db := MustOpenDB(t)
+		defer MustCloseDBNoCheck(t, db)
+		tx := MustBegin(t, db, true)
+		defer tx.Rollback()
+
+		if err := tx.CreateBitmap("x"); err != nil {
+			t.Fatal(err)
+		}
+
+		// Insert enough array containers to split page.
+		for i := 0; i < 1000; i++ {
+			if _, err := tx.Add("x", uint64(i<<16)); err != nil {
+				t.Fatalf("Add(%d) err=%q", i<<16, err)
+			}
+		}
+
+		// Read page types for all pages.
+		infos, err := tx.PageInfos()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Commit & checkpoint to flush to the data file.
+		if err := tx.Commit(); err != nil {
+			t.Fatal(err)
+		} else if err := db.Checkpoint(); err != nil {
+			t.Fatal(err)
+		}
+
+		// Corrupt first branch page found by zeroing out the cell count.
+		var pgno uint32
+		for _, info := range infos {
+			if info, ok := info.(*rbf.BranchPageInfo); ok {
+				pgno = info.Pgno
+				page := mustReadPage(t, db.DataPath(), pgno)
+				binary.BigEndian.PutUint16(page[8:10], 0) // zero cell count
+				mustWritePage(t, db.DataPath(), pgno, page)
+				break
+			}
+		}
+
+		// Verify that check now returns an error.
+		if err := db.Check(); err == nil || !strings.Contains(err.Error(), fmt.Sprintf("branch page %d is empty", pgno)) {
+			t.Fatalf("unexpected error: %#v", err)
+		}
+	})
+}
+
+func mustReadPage(tb testing.TB, path string, pgno uint32) []byte {
+	tb.Helper()
+	f, err := os.Open(path)
+	if err != nil {
+		tb.Fatal(err)
+	}
+	defer f.Close()
+
+	buf := make([]byte, rbf.PageSize)
+	if _, err := f.ReadAt(buf, int64(pgno)*rbf.PageSize); err != nil {
+		tb.Fatal(err)
+	}
+	return buf
+}
+
+func mustWritePage(tb testing.TB, path string, pgno uint32, buf []byte) {
+	tb.Helper()
+	f, err := os.OpenFile(path, os.O_WRONLY, 0666)
+	if err != nil {
+		tb.Fatal(err)
+	}
+	defer f.Close()
+
+	if _, err := f.WriteAt(buf, int64(pgno)*rbf.PageSize); err != nil {
+		tb.Fatal(err)
+	}
+}

From ddb5020aa66afa04b9080aa169f4a9fea59be194 Mon Sep 17 00:00:00 2001
From: Seebs <seebs@molecula.com>
Date: Mon, 20 Dec 2021 12:22:24 -0600
Subject: [PATCH 51/51] slightly better lock protection around bitDepth in view

There's a number of deeper issues here (the fragment is conjuring
up a Tx, for instance) but this helps.

Also use field.view() to get the view rather than accessing viewMap
directly without a lock. Also change field.cacheBitDepth to ratchet
upwards -- if we have multiple shards and some shards have lower
depths than others, we should use the highest as the cached value,
not the most recent.
---
 api.go      | 4 ++--
 field.go    | 7 +++++--
 fragment.go | 2 ++
 view.go     | 2 ++
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/api.go b/api.go
index 70a27da94..47558678d 100644
--- a/api.go
+++ b/api.go
@@ -2753,8 +2753,8 @@ func (api *API) RestoreShard(ctx context.Context, indexName string, shard uint64
 
 	for _, flv := range flvs {
 		fld := idx.field(flv.Field)
-		view, ok := fld.viewMap[flv.View]
-		if !ok {
+		view := fld.view(flv.View)
+		if view == nil {
 			view, err = fld.createViewIfNotExists(flv.View)
 			if err != nil {
 				return err
diff --git a/field.go b/field.go
index 2aa865a1f..d325c477e 100644
--- a/field.go
+++ b/field.go
@@ -705,8 +705,11 @@ func (f *Field) cacheBitDepth(bd uint64) error {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 
-	f.options.BitDepth = bd
-	if bsig != nil {
+	if f.options.BitDepth < bd {
+		f.options.BitDepth = bd
+	}
+
+	if bsig != nil && bsig.BitDepth < bd {
 		bsig.BitDepth = bd
 	}
 
diff --git a/fragment.go b/fragment.go
index 9b77f90f7..8dfa6749c 100644
--- a/fragment.go
+++ b/fragment.go
@@ -218,6 +218,8 @@ func newFragment(holder *Holder, spec fragSpec, shard uint64, flags byte) *fragm
 func (f *fragment) cachePath() string { return f.path() + cacheExt }
 
 func (f *fragment) bitDepth() (uint64, error) {
+	f.mu.RLock()
+	defer f.mu.RUnlock()
 	tx, err := f.holder.BeginTx(false, f.idx, f.shard)
 	if err != nil {
 		return 0, errors.Wrapf(err, "beginning new tx(false, %s, %d)", f.index(), f.shard)
diff --git a/view.go b/view.go
index f3bd27b3a..5a8e23ae1 100644
--- a/view.go
+++ b/view.go
@@ -619,7 +619,9 @@ func (v *view) bitDepth(shards []uint64) (uint64, error) {
 	var maxBitDepth uint64
 
 	for _, shard := range shards {
+		v.mu.RLock()
 		frag, ok := v.fragments[shard]
+		v.mu.RUnlock()
 		if !ok || frag == nil {
 			continue
 		}