Do not parse trigger bodies in LoadOnly context

enginetest/engine_only_test.go

@@ -647,7 +647,9 @@ func TestTriggerViewWarning(t *testing.T) {
 
 	baseDb := db.(*memory.HistoryDatabase).BaseDatabase
 	err = baseDb.CreateTrigger(nil, sql.TriggerDefinition{
-		Name:            "view_trig",
+		Name: "view_trig",
+		// TODO: this trigger statement still wouldn't parse correctly in MySQL if it was created on a table. Update to
+		// a trigger that would otherwise be valid.
 		CreateStatement: "CREATE TRIGGER view_trig BEFORE INSERT ON myview FOR EACH ROW SET i=i+2",
 	})
 	assert.NoError(t, err)
@@ -657,7 +659,8 @@ func TestTriggerViewWarning(t *testing.T) {
 	enginetest.CreateNewConnectionForServerEngine(ctx, e)
 
 	enginetest.TestQueryWithContext(t, ctx, e, harness, "insert into mytable values (4, 'fourth row')", []sql.Row{{types.NewOkResult(1)}}, nil, nil, nil)
-	enginetest.TestQueryWithContext(t, ctx, e, harness, "SHOW WARNINGS", []sql.Row{{"Warning", 0, "trigger on view is not supported; 'DROP TRIGGER  view_trig' to fix"}}, nil, nil, nil)
+	enginetest.TestQueryWithContext(t, ctx, e, harness, "SHOW WARNINGS", []sql.Row{{"Warning", 0, "Trigger on view is not supported. Please run 'DROP TRIGGER view_trig;'"}}, nil, nil, nil)
+	// TODO: this is not correct. MySQL allows inserting into views. https://github.com/dolthub/dolt/issues/10292
 	enginetest.AssertErrWithCtx(t, e, harness, ctx, "insert into myview values (5, 'fifth row')", nil, nil, "expected insert destination to be resolved or unresolved table")
 }
 

enginetest/queries/function_queries.go

@@ -2325,6 +2325,7 @@ var FunctionQueryTests = []QueryTest{
 		Expected: []sql.Row{{0}},
 	},
 	{
+		// https://github.com/dolthub/dolt/issues/10087
 		Skip:     true,
 		Query:    "select extract(day_microsecond from true)",
 		Expected: []sql.Row{{1000000}},

enginetest/queries/trigger_queries.go

@@ -3918,6 +3918,117 @@ end;
 			},
 		},
 	},
+	{
+		// https://github.com/dolthub/dolt/issues/10287
+		Name: "only throw 'column not found' error when trigger is actually triggered",
+		SetUpScript: []string{
+			"create table A(col0 int, col1 int)",
+			"create table B(col0 int, col1 int)",
+			"create table C(col0 int)",
+			"create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0",
+			"drop table B",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query: "show triggers",
+				// assert trigger is still there
+				Expected: []sql.Row{
+					{
+						"test_trigger", // Trigger
+						"UPDATE",       // Event
+						"A",            // Table
+						"insert into C (col0) select col0 from B where B.col0 = new.col0", // Statement
+						"AFTER",               // Timing
+						time.Unix(0, 0).UTC(), // Created
+						"",                    // sql_mode
+						"",                    // Definer
+						sql.Collation_Default.CharacterSet().String(), // character_set_client
+						sql.Collation_Default.String(),                // collation_connection
+						sql.Collation_Default.String(),                // Database Collation
+					},
+				},
+			},
+			{
+				Query: "create table B(a int, b int)",
+				Expected: []sql.Row{
+					{types.NewOkResult(0)},
+				},
+			},
+			{
+				Query: "show triggers",
+				// assert trigger is still there
+				Expected: []sql.Row{
+					{
+						"test_trigger", // Trigger
+						"UPDATE",       // Event
+						"A",            // Table
+						"insert into C (col0) select col0 from B where B.col0 = new.col0", // Statement
+						"AFTER",               // Timing
+						time.Unix(0, 0).UTC(), // Created
+						"",                    // sql_mode
+						"",                    // Definer
+						sql.Collation_Default.CharacterSet().String(), // character_set_client
+						sql.Collation_Default.String(),                // collation_connection
+						sql.Collation_Default.String(),                // Database Collation
+					},
+				},
+			},
+			{
+				Query: "insert into C values (12)",
+				Expected: []sql.Row{
+					{types.NewOkResult(1)},
+				},
+			},
+			{
+				Query: "insert into A values (1, 2)",
+				Expected: []sql.Row{
+					{types.NewOkResult(1)},
+				},
+			},
+			{
+				Query:       "update A set A.col0=2 where true",
+				ExpectedErr: sql.ErrTableColumnNotFound,
+			},
+			{
+				Query:    "select * from A",
+				Expected: []sql.Row{{1, 2}},
+			},
+			{
+				Query: "show create trigger test_trigger",
+				// assert trigger info is there
+				Expected: []sql.Row{
+					{
+						"test_trigger",
+						"",
+						"create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0",
+						sql.Collation_Default.CharacterSet().String(),
+						sql.Collation_Default.String(),
+						sql.Collation_Default.String(),
+						time.Unix(0, 0).UTC(),
+					},
+				},
+			},
+			{
+				Query: "drop trigger test_trigger",
+				Expected: []sql.Row{
+					{types.NewOkResult(0)},
+				},
+			},
+			{
+				Query: "show triggers",
+				// assert trigger has been dropped
+				Expected: []sql.Row{},
+			},
+			{
+				// https://github.com/dolthub/dolt/issues/10291
+				Skip:  true,
+				Query: "create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0",
+				Expected: []sql.Row{
+					{types.NewOkResult(0)},
+				},
+			},
+		},
+	},
 }
 
 var TriggerCreateInSubroutineTests = []ScriptTest{

sql/analyzer/load_triggers.go

@@ -108,6 +108,7 @@ func loadTriggersFromDb(ctx *sql.Context, a *Analyzer, db sql.Database, ignorePa
 			// TODO: should perhaps add the auth query handler to the analyzer? does this even use auth?
 			builder := planbuilder.New(ctx, a.Catalog, nil)
 			builder.SetParserOptions(sqlMode.ParserOptions())
+			builder.TriggerCtx().LoadOnly = true
 			parsedTrigger, _, _, _, err = builder.Parse(trigger.CreateStatement, nil, false)
 			if err != nil {
 				// We want to be able to drop invalid triggers, so ignore any parser errors and return the name of the trigger

sql/analyzer/triggers.go

@@ -212,42 +212,21 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope,
 
 		b := planbuilder.New(ctx, a.Catalog, nil)
 		b.DisableAuth()
-		prevActive := b.TriggerCtx().Active
-		b.TriggerCtx().Active = true
-		defer func() {
-			b.TriggerCtx().Active = prevActive
-		}()
 
 		for _, trigger := range triggers {
 			var parsedTrigger sql.Node
 			sqlMode := sql.NewSqlModeFromString(trigger.SqlMode)
 			b.SetParserOptions(sqlMode.ParserOptions())
+			b.TriggerCtx().LoadOnly = true
 			parsedTrigger, _, _, _, err = b.Parse(trigger.CreateStatement, nil, false)
 			b.Reset()
 			if err != nil {
-				return nil, transform.SameTree, err
+				continue
 			}
 
 			ct, ok := parsedTrigger.(*plan.CreateTrigger)
 			if !ok {
-				return nil, transform.SameTree, sql.ErrTriggerCreateStatementInvalid.New(trigger.CreateStatement)
-			}
-			transform.Inspect(ct.Body, func(n sql.Node) bool {
-				call, isCall := n.(*plan.Call)
-				if !isCall {
-					return true
-				}
-				if call.Procedure == nil {
-					return true
-				}
-				if call.Procedure.ValidationError == nil {
-					return true
-				}
-				err = call.Procedure.ValidationError
-				return false
-			})
-			if err != nil {
-				return nil, transform.SameTree, err
+				continue
 			}
 
 			var triggerTable string
@@ -257,11 +236,11 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope,
 			default:
 			}
 			if stringContains(affectedTables, triggerTable) && triggerEventsMatch(triggerEvent, ct.TriggerEvent) {
-				// first pass allows unresolved before we know whether trigger is relevant
-				// TODO store destination table name with trigger, so we don't have to do parse twice
+				// first pass does not parse the trigger body and is only so we know whether trigger is relevant
 				b.TriggerCtx().Call = true
+				// TODO: We only need to parse the body here since the other info from the create trigger statement have
+				// already been parsed.
 				parsedTrigger, _, _, _, err = b.Parse(trigger.CreateStatement, nil, false)
-				b.TriggerCtx().Call = false
 				b.Reset()
 				if err != nil {
 					return nil, transform.SameTree, err
@@ -272,6 +251,24 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope,
 					return nil, transform.SameTree, sql.ErrTriggerCreateStatementInvalid.New(trigger.CreateStatement)
 				}
 
+				transform.Inspect(ct.Body, func(n sql.Node) bool {
+					call, isCall := n.(*plan.Call)
+					if !isCall {
+						return true
+					}
+					if call.Procedure == nil {
+						return true
+					}
+					if call.Procedure.ValidationError == nil {
+						return true
+					}
+					err = call.Procedure.ValidationError
+					return false
+				})
+				if err != nil {
+					return nil, transform.SameTree, err
+				}
+
 				if block, ok := ct.Body.(*plan.BeginEndBlock); ok {
 					ct.Body = plan.NewTriggerBeginEndBlock(block)
 				}

sql/planbuilder/builder.go

@@ -99,6 +99,7 @@ type TriggerContext struct {
 	UnresolvedTables []string
 	Active           bool
 	Call             bool
+	LoadOnly         bool
 }
 
 // ProcContext allows nested CALLs to use the same database for resolving

sql/planbuilder/create_ddl.go

@@ -53,84 +53,88 @@ func (b *Builder) buildCreateTrigger(inScope *scope, subQuery string, fullQuery
 		}
 	}
 
-	// resolve table -> create initial scope
-	prevTriggerCtxActive := b.TriggerCtx().Active
-	b.TriggerCtx().Active = true
-	defer func() {
-		b.TriggerCtx().Active = prevTriggerCtxActive
-	}()
+	db, ok := b.resolveDbForTable(c.Table)
+	if !ok {
+		b.handleErr(sql.ErrDatabaseSchemaNotFound.New(c.Table.SchemaQualifier.String()))
+	}
 
-	tableName := strings.ToLower(c.Table.Name.String())
 	tableScope, ok := b.buildResolvedTableForTablename(inScope, c.Table, nil)
 	if !ok {
-		b.handleErr(sql.ErrTableNotFound.New(tableName))
+		b.handleErr(sql.ErrTableNotFound.New(c.Table.Name.String()))
 	}
 	if _, ok := tableScope.node.(*plan.UnresolvedTable); ok {
 		// unknown table in trigger body is OK, but the target table must exist
-		b.handleErr(sql.ErrTableNotFound.New(tableName))
-	}
-
-	// todo scope with new and old columns provided
-	// insert/update have "new"
-	// update/delete have "old"
-	newScope := tableScope.replace()
-	oldScope := tableScope.replace()
-	for _, col := range tableScope.cols {
-		switch c.TriggerSpec.Event {
-		case ast.InsertStr:
-			newScope.newColumn(col)
-		case ast.UpdateStr:
-			newScope.newColumn(col)
-			oldScope.newColumn(col)
-		case ast.DeleteStr:
-			oldScope.newColumn(col)
-		}
+		b.handleErr(sql.ErrTableNotFound.New(c.Table.Name.String()))
 	}
-	newScope.setTableAlias("new")
-	oldScope.setTableAlias("old")
-	triggerScope := tableScope.replace()
-
-	triggerScope.addColumns(newScope.cols)
-	triggerScope.addColumns(oldScope.cols)
-
-	triggerScope.addExpressions(newScope.exprs)
-	triggerScope.addExpressions(oldScope.exprs)
 
-	bodyStr := strings.TrimSpace(fullQuery[c.SubStatementPositionStart:c.SubStatementPositionEnd])
-	bodyScope := b.buildSubquery(triggerScope, c.TriggerSpec.Body, bodyStr, fullQuery)
-	definer := getCurrentUserForDefiner(b.ctx, c.TriggerSpec.Definer)
-
-	db, ok := b.resolveDbForTable(c.Table)
-	if !ok {
-		b.handleErr(sql.ErrDatabaseSchemaNotFound.New(c.Table.SchemaQualifier.String()))
-	}
+	triggerCtx := b.TriggerCtx()
 
 	if _, ok := tableScope.node.(*plan.ResolvedTable); !ok {
-		if prevTriggerCtxActive {
-			// previous ctx set means this is an INSERT or SHOW
-			// old version of Dolt permitted a bad trigger on VIEW
-			// warn and noop
-			b.ctx.Warn(0, "trigger on view is not supported; 'DROP TRIGGER  %s' to fix", c.TriggerSpec.TrigName.Name.String())
-			bodyScope.node = plan.NewResolvedDualTable()
+		// Old versions of GMS/Dolt permitted creating an invalid trigger on VIEW
+		if triggerCtx.LoadOnly {
+			// LoadOnly means a CreateTrigger statement was parsed while loading triggers for SHOW, INSERT, UPDATE, or
+			// DELETE. Warn and no-op. Since tableScope.node here is not a ResolvedTable, it won't be matched to any
+			// table during applyTriggers.
+			b.ctx.Warn(0, "Trigger on view is not supported. Please run 'DROP TRIGGER %s;'", c.TriggerSpec.TrigName.Name.String())
 		} else {
-			// top-level call is DDL
-			err := sql.ErrExpectedTableFoundView.New(tableName)
+			// Top-level call is DDL, and we should not allow a trigger on a VIEW to be created here. Or somehow, a
+			// trigger on a VIEW has been matched during applyTriggers, and we should not run that trigger.
+			err := sql.ErrExpectedTableFoundView.New(c.Table.Name.String())
 			b.handleErr(err)
 		}
 	}
 
+	var bodyNode sql.Node
+	bodyStr := strings.TrimSpace(fullQuery[c.SubStatementPositionStart:c.SubStatementPositionEnd])
+	if !triggerCtx.LoadOnly {
+		prevTriggerCtxActive := triggerCtx.Active
+		triggerCtx.Active = true
+		defer func() {
+			triggerCtx.Active = prevTriggerCtxActive
+		}()
+
+		// todo scope with new and old columns provided
+		// insert/update have "new"
+		// update/delete have "old"
+		newScope := tableScope.replace()
+		oldScope := tableScope.replace()
+		for _, col := range tableScope.cols {
+			switch c.TriggerSpec.Event {
+			case ast.InsertStr:
+				newScope.newColumn(col)
+			case ast.UpdateStr:
+				newScope.newColumn(col)
+				oldScope.newColumn(col)
+			case ast.DeleteStr:
+				oldScope.newColumn(col)
+			}
+		}
+		newScope.setTableAlias("new")
+		oldScope.setTableAlias("old")
+		triggerScope := tableScope.replace()
+
+		triggerScope.addColumns(newScope.cols)
+		triggerScope.addColumns(oldScope.cols)
+
+		triggerScope.addExpressions(newScope.exprs)
+		triggerScope.addExpressions(oldScope.exprs)
+
+		bodyScope := b.buildSubquery(triggerScope, c.TriggerSpec.Body, bodyStr, fullQuery)
+		bodyNode = bodyScope.node
+	}
+
 	outScope.node = plan.NewCreateTrigger(
 		db,
 		c.TriggerSpec.TrigName.Name.String(),
 		c.TriggerSpec.Time,
 		c.TriggerSpec.Event,
 		triggerOrder,
 		tableScope.node,
-		bodyScope.node,
+		bodyNode,
 		subQuery,
 		bodyStr,
 		b.ctx.QueryTime(),
-		definer,
+		getCurrentUserForDefiner(b.ctx, c.TriggerSpec.Definer),
 	)
 	return outScope
 }