This change is 

Last time I looked at this, scanNode did mutate the descriptor. Let me check again.

LGTM. I don't see any place that's modifying it.

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

I'm not sure where scanNode.desc is being modified, but it sure looks like it is. This is how I discovered that before. Perhaps the way I'm checking this isn't valid.

~ make test PKG=./sql 2>&1 | tee out
...
F170730 16:28:53.135339 203814 sql/scan.go:138  [client=127.0.0.1:58039,user=root,n1] scanNode.desc unexpectedly modified:
[UpVersion: true != false DependedOnBy: []sqlbase.TableDescriptor_Reference[1] != []sqlbase.TableDescriptor_Reference[0]]

diff --git a/pkg/sql/scan.go b/pkg/sql/scan.go
index c68fdbbce..2a88dab8b 100644
--- a/pkg/sql/scan.go
+++ b/pkg/sql/scan.go
@@ -19,8 +19,10 @@ package sql
 import (
 	"bytes"
 	"fmt"
+	"reflect"
 	"sync"
 
+	"github.com/kr/pretty"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 
@@ -29,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/util"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 )
 
@@ -41,9 +44,10 @@ var scanNodePool = sync.Pool{
 // A scanNode handles scanning over the key/value pairs for a table and
 // reconstructing them into rows.
 type scanNode struct {
-	p     *planner
-	desc  sqlbase.TableDescriptor
-	index *sqlbase.IndexDescriptor
+	p       *planner
+	desc    sqlbase.TableDescriptor
+	descPtr *sqlbase.TableDescriptor
+	index   *sqlbase.IndexDescriptor
 
 	// Set if an index was explicitly specified.
 	specifiedIndex *sqlbase.IndexDescriptor
@@ -129,7 +133,12 @@ func (n *scanNode) Start(runParams) error {
 		n.valNeededForCol, false /* returnRangeInfo */, &n.p.alloc)
 }
 
-func (n *scanNode) Close(context.Context) {
+func (n *scanNode) Close(ctx context.Context) {
+	if n.descPtr != nil && !reflect.DeepEqual(n.desc, *n.descPtr) {
+		log.Fatalf(ctx, "scanNode.desc unexpectedly modified:\n%s",
+			pretty.Diff(n.desc, *n.descPtr))
+	}
+
 	*n = scanNode{}
 	scanNodePool.Put(n)
 }
@@ -198,6 +207,7 @@ func (n *scanNode) initTable(
 	wantedColumns []parser.ColumnID,
 ) error {
 	n.desc = *desc
+	n.descPtr = desc
 
 	if !p.skipSelectPrivilegeChecks {
 		if err := p.CheckPrivilege(&n.desc, privilege.SELECT); err != nil {

Very interesting.. Can you also store another copy of *desc and compare against that? It would be useful to distinguish between the scanNode modifying n.desc or something else modifying the real descriptor.

Very interesting.. Can you also store another copy of *desc and compare against that? It would be useful to distinguish between the scanNode modifying n.desc or something else modifying the real descriptor.

It is definitely something modifying the scanNode version. Storing a descCopy instead of *descPtr shows that same badness. Shouldn't be hard to track this down with a bit of elbow grease, but we definitely don't want to merge this until we do.

Oh, but then this should go in after that is fixed, no?

So thanks to Peter's suggestion I am going to do this in two steps. I will first amend the fix in this PR to ensure the leased descriptor is not modified in-place by CREATE VIEW (and check with Peter's patch this indeed is sufficient). Then in my separate PR #17310 I will ensure this is obsolete.

All right, PTAL. The first commit includes both my original change and the assertion; the code is extended to ensure CREATE VIEW does not trip the assertion. The second commit removes the assertion. You can check the 1st commit separately to see that all tests pass.

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

Reviewed 6 of 6 files at r1.
Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

pkg/sql/scan.go, line 137 at r1 (raw file):

func (n *scanNode) Close(ctx context.Context) {
	if n.desc != nil && n.desc.ID != 0 && n.descCopy.ID != 0 && !reflect.DeepEqual(n.descCopy, *n.desc) {

Are the n.desc.ID != 0 && n.descCopy.ID != 0 conditional necessary? Mildly worried they are hiding something.

Comments from Reviewable

Yes they hiding tests that hand scanNode with mostly-null descriptors (hand-crafted).

All right for the sake of double-checking I removed the comparison to zero in the assertion and fixed the tests that were tripping it.