Skip to content

Commit 1c7a7d9

Browse files
craig[bot]DrewKimball
craig[bot]
and
DrewKimball
committed
Merge #144217
144217: sql,optbuilder: route descriptor lookups for triggers through cache r=mgartner,yuzefovich a=DrewKimball #### rttanalysis: add test case with a trigger This commit adds an rttanalysis test case that creates a trigger on a table and then inserts into the table. This will be useful in the following commit, which seeks to ensure the schema lookup during planning of a trigger goes through the cache. Informs #144211 Release note: None #### sql: add logic test for trigger arguments This commit adds a test to ensure that trigger function arguments are provided correctly. Epic: None Release note: None #### sql,optbuilder: route descriptor lookups for triggers through cache Triggers have a `TG_TABLE_SCHEMA` argument which provides the schema name. Getting the schema name requires doing a descriptor lookup when the trigger is planned, which previously avoided the cache and always performed an expensive KV lookup. This commit plumbs `ResolveSchemaByID`, which allows the lookup to hit the descriptor cache. Fixes #144211 Release note (performance improvement): Triggers now perform the descriptor lookup for `TG_TABLE_SCHEMA` against a cache. This can significantly reduce trigger planning latency in multi-region databases. Co-authored-by: Drew Kimball <[email protected]>
2 parents 6d1fb75 + 04e44c8 commit 1c7a7d9

File tree

12 files changed

+227
-11
lines changed

12 files changed

+227
-11
lines changed

‎pkg/bench/rttanalysis/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ go_test(
4747
"orm_queries_bench_test.go",
4848
"rtt_analysis_test.go",
4949
"system_bench_test.go",
50+
"trigger_resolution_test.go",
5051
"truncate_bench_test.go",
5152
"udf_resolution_test.go",
5253
"validate_benchmark_data_test.go",

‎pkg/bench/rttanalysis/testdata/benchmark_expectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ exp,benchmark
119119
1,SystemDatabaseQueries/select_system.users_with_empty_database_Name
120120
1,SystemDatabaseQueries/select_system.users_with_schema_Name
121121
1,SystemDatabaseQueries/select_system.users_without_schema_Name
122+
1,TriggerResolution/insert_into_table_with_trigger
122123
15,Truncate/truncate_1_column_0_rows
123124
15,Truncate/truncate_1_column_1_row
124125
15,Truncate/truncate_1_column_2_rows

‎pkg/bench/rttanalysis/trigger_resolution_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright 2025 The Cockroach Authors.
2+
//
3+
// Use of this software is governed by the CockroachDB Software License
4+
// included in the /LICENSE file.
5+
6+
package rttanalysis
7+
8+
import "testing"
9+
10+
func BenchmarkTriggerResolution(b *testing.B) {
11+
reg.Run(b)
12+
}
13+
func init() {
14+
reg.Register("TriggerResolution", []RoundTripBenchTestCase{
15+
{
16+
Name: "insert into table with trigger",
17+
Setup: `
18+
CREATE TABLE trigger_table (a INT);
19+
CREATE FUNCTION trigger_fn() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$ BEGIN RETURN NEW; END $$;
20+
CREATE TRIGGER tr BEFORE INSERT ON trigger_table FOR EACH ROW EXECUTE FUNCTION trigger_fn();`,
21+
Stmt: `INSERT INTO trigger_table VALUES (100);`,
22+
},
23+
})
24+
}

‎pkg/ccl/logictestccl/testdata/logic_test/triggers‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4037,7 +4037,7 @@ statement ok
40374037
CREATE OR REPLACE FUNCTION f() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$ BEGIN RETURN NEW; END $$;
40384038

40394039
# ==============================================================================
4040-
# Test that descriptor backreferences are cleaned up
4040+
# Test that descriptor backreferences are cleaned up.
40414041
# ==============================================================================
40424042

40434043
subtest ensure_back_ref_cleanup
@@ -4086,6 +4086,100 @@ DROP FUNCTION update_listing_balance ;
40864086
statement ok
40874087
drop table listings_balance cascade;
40884088

4089+
# ==============================================================================
4090+
# Test trigger arguments.
4091+
# ==============================================================================
4092+
4093+
statement ok
4094+
CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
4095+
BEGIN
4096+
RAISE NOTICE 'NEW: %', NEW;
4097+
RAISE NOTICE 'OLD: %', OLD;
4098+
RAISE NOTICE 'TG_NAME: %', TG_NAME;
4099+
RAISE NOTICE 'TG_WHEN: %', TG_WHEN;
4100+
RAISE NOTICE 'TG_LEVEL: %', TG_LEVEL;
4101+
RAISE NOTICE 'TG_OP: %', TG_OP;
4102+
RAISE NOTICE 'TG_RELID: %', TG_RELID;
4103+
RAISE NOTICE 'TG_RELNAME: %', TG_RELNAME;
4104+
RAISE NOTICE 'TG_TABLE_NAME: %', TG_TABLE_NAME;
4105+
RAISE NOTICE 'TG_TABLE_SCHEMA: %', TG_TABLE_SCHEMA;
4106+
RAISE NOTICE 'TG_NARGS: %', TG_NARGS;
4107+
-- TODO(#135311): uncomment this when we support TG_ARGV.
4108+
--RAISE NOTICE 'TG_ARGV: %s', TG_ARGV;
4109+
RETURN NEW;
4110+
END
4111+
$$;
4112+
4113+
statement ok
4114+
CREATE TRIGGER foo BEFORE INSERT OR UPDATE ON xy FOR EACH ROW WHEN ((NEW).x = 1) EXECUTE FUNCTION g();
4115+
4116+
statement ok
4117+
CREATE TRIGGER bar AFTER INSERT OR UPDATE ON xy FOR EACH ROW WHEN ((NEW).x = 1) EXECUTE FUNCTION g();
4118+
4119+
query T noticetrace
4120+
INSERT INTO xy VALUES (1, 2);
4121+
----
4122+
NOTICE: NEW: (1,2)
4123+
NOTICE: OLD: <NULL>
4124+
NOTICE: TG_NAME: foo
4125+
NOTICE: TG_WHEN: BEFORE
4126+
NOTICE: TG_LEVEL: ROW
4127+
NOTICE: TG_OP: INSERT
4128+
NOTICE: TG_RELID: 106
4129+
NOTICE: TG_RELNAME: xy
4130+
NOTICE: TG_TABLE_NAME: xy
4131+
NOTICE: TG_TABLE_SCHEMA: public
4132+
NOTICE: TG_NARGS: 0
4133+
NOTICE: NEW: (1,2)
4134+
NOTICE: OLD: <NULL>
4135+
NOTICE: TG_NAME: bar
4136+
NOTICE: TG_WHEN: AFTER
4137+
NOTICE: TG_LEVEL: ROW
4138+
NOTICE: TG_OP: INSERT
4139+
NOTICE: TG_RELID: 106
4140+
NOTICE: TG_RELNAME: xy
4141+
NOTICE: TG_TABLE_NAME: xy
4142+
NOTICE: TG_TABLE_SCHEMA: public
4143+
NOTICE: TG_NARGS: 0
4144+
4145+
query T noticetrace
4146+
UPDATE xy SET y = 3 WHERE x = 1;
4147+
----
4148+
NOTICE: NEW: (1,3)
4149+
NOTICE: OLD: (1,2)
4150+
NOTICE: TG_NAME: foo
4151+
NOTICE: TG_WHEN: BEFORE
4152+
NOTICE: TG_LEVEL: ROW
4153+
NOTICE: TG_OP: UPDATE
4154+
NOTICE: TG_RELID: 106
4155+
NOTICE: TG_RELNAME: xy
4156+
NOTICE: TG_TABLE_NAME: xy
4157+
NOTICE: TG_TABLE_SCHEMA: public
4158+
NOTICE: TG_NARGS: 0
4159+
NOTICE: NEW: (1,3)
4160+
NOTICE: OLD: (1,2)
4161+
NOTICE: TG_NAME: bar
4162+
NOTICE: TG_WHEN: AFTER
4163+
NOTICE: TG_LEVEL: ROW
4164+
NOTICE: TG_OP: UPDATE
4165+
NOTICE: TG_RELID: 106
4166+
NOTICE: TG_RELNAME: xy
4167+
NOTICE: TG_TABLE_NAME: xy
4168+
NOTICE: TG_TABLE_SCHEMA: public
4169+
NOTICE: TG_NARGS: 0
4170+
4171+
statement ok
4172+
DROP TRIGGER foo ON xy;
4173+
4174+
statement ok
4175+
DROP TRIGGER bar ON xy;
4176+
4177+
statement ok
4178+
DROP FUNCTION g;
4179+
4180+
statement ok
4181+
DELETE FROM xy WHERE true;
4182+
40894183
# ==============================================================================
40904184
# Regression tests.
40914185
# ==============================================================================

‎pkg/sql/opt/cat/catalog.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,10 @@ type Catalog interface {
153153
// be safely copied or used across goroutines.
154154
ResolveSchema(ctx context.Context, flags Flags, name *SchemaName) (Schema, SchemaName, error)
155155

156+
// ResolveSchemaByID is similar to ResolveSchema, except that it locates a
157+
// schema by its StableID. See the comment for StableID for more details.
158+
ResolveSchemaByID(ctx context.Context, flags Flags, id StableID) (Schema, error)
159+
156160
// GetAllSchemaNamesForDB Gets all the SchemaNames for a database.
157161
GetAllSchemaNamesForDB(ctx context.Context, dbName string) ([]SchemaName, error)
158162

‎pkg/sql/opt/cat/table.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,10 @@ type Table interface {
175175
// owning database could not be determined.
176176
GetDatabaseID() descpb.ID
177177

178+
// GetSchemaID returns the owning schema id of the table, or zero, if the
179+
// owning schema could not be determined.
180+
GetSchemaID() descpb.ID
181+
178182
// IsHypothetical returns true if this is a hypothetical table (used when
179183
// searching for index recommendations).
180184
IsHypothetical() bool

‎pkg/sql/opt/exec/explain/plan_gist_factory.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,11 @@ func (u *unknownTable) GetDatabaseID() descpb.ID {
650650
return 0
651651
}
652652

653+
// GetSchemaID is part of the cat.Table interface.
654+
func (u *unknownTable) GetSchemaID() descpb.ID {
655+
return 0
656+
}
657+
653658
// IsHypothetical is part of the cat.Table interface.
654659
func (u *unknownTable) IsHypothetical() bool {
655660
return false

‎pkg/sql/opt/optbuilder/trigger.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,11 +220,13 @@ func (mb *mutationBuilder) buildTriggerFunctionArgs(
220220
tgOp := tree.NewDString(eventType.String())
221221
tgRelID := tree.NewDOid(oid.Oid(mb.tab.ID()))
222222
tgTableName := tree.NewDString(string(mb.tab.Name()))
223-
fqName, err := mb.b.catalog.FullyQualifiedName(mb.b.ctx, mb.tab)
223+
schema, err := mb.b.catalog.ResolveSchemaByID(
224+
mb.b.ctx, cat.Flags{}, cat.StableID(mb.tab.GetSchemaID()),
225+
)
224226
if err != nil {
225227
panic(err)
226228
}
227-
tgTableSchema := tree.NewDString(fqName.Schema())
229+
tgTableSchema := tree.NewDString(schema.Name().Schema())
228230
tgNumArgs := tree.NewDInt(tree.DInt(len(trigger.FuncArgs())))
229231
tgArgV := tree.NewDArray(types.String)
230232
for _, arg := range trigger.FuncArgs() {
@@ -240,7 +242,7 @@ func (mb *mutationBuilder) buildTriggerFunctionArgs(
240242
f.ConstructConstVal(tgWhen, types.String), // TG_WHEN
241243
f.ConstructConstVal(tgLevel, types.String), // TG_LEVEL
242244
f.ConstructConstVal(tgOp, types.String), // TG_OP
243-
f.ConstructConstVal(tgRelID, types.Oid), // TG_RELIID
245+
f.ConstructConstVal(tgRelID, types.Oid), // TG_RELID
244246
f.ConstructConstVal(tgTableName, types.String), // TG_RELNAME
245247
f.ConstructConstVal(tgTableName, types.String), // TG_TABLE_NAME
246248
f.ConstructConstVal(tgTableSchema, types.String), // TG_TABLE_SCHEMA
@@ -703,7 +705,7 @@ func (tb *rowLevelAfterTriggerBuilder) Build(
703705
f.ConstructConstVal(tgWhen, types.String), // TG_WHEN
704706
f.ConstructConstVal(tgLevel, types.String), // TG_LEVEL
705707
tgOp, // TG_OP
706-
f.ConstructConstVal(tgRelID, types.Oid), // TG_RELIID
708+
f.ConstructConstVal(tgRelID, types.Oid), // TG_RELID
707709
f.ConstructConstVal(tgTableName, types.String), // TG_RELNAME
708710
f.ConstructConstVal(tgTableName, types.String), // TG_TABLE_NAME
709711
f.ConstructConstVal(tgTableSchema, types.String), // TG_TABLE_SCHEMA

‎pkg/sql/opt/testutils/testcat/create_table.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func (tc *Catalog) CreateTable(stmt *tree.CreateTable) *Table {
8080
isRbr = stmt.Locality.LocalityLevel == tree.LocalityLevelRow
8181
isRbt = stmt.Locality.LocalityLevel == tree.LocalityLevelTable
8282
}
83-
tab := &Table{TabID: tc.nextStableID(), TabName: stmt.Table, Catalog: tc}
83+
tab := &Table{TabID: tc.nextStableID(), SchemaID: testSchemaID, TabName: stmt.Table, Catalog: tc}
8484

8585
if isRbt && stmt.Locality.TableRegion != "" {
8686
tab.multiRegion = true
@@ -490,7 +490,13 @@ func (tc *Catalog) CreateTableAs(name tree.TableName, columns []cat.Column) *Tab
490490
// Update the table name to include catalog and schema if not provided.
491491
tc.qualifyTableName(&name)
492492

493-
tab := &Table{TabID: tc.nextStableID(), TabName: name, Catalog: tc, Columns: columns}
493+
tab := &Table{
494+
TabID: tc.nextStableID(),
495+
SchemaID: testSchemaID,
496+
TabName: name,
497+
Catalog: tc,
498+
Columns: columns,
499+
}
494500

495501
var rowid cat.Column
496502
ordinal := len(columns)

‎pkg/sql/opt/testutils/testcat/test_catalog.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,8 @@ import (
4343

4444
const (
4545
// testDB is the default current database for testing purposes.
46-
testDB = "t"
46+
testDB = "t"
47+
testSchemaID = 1
4748
)
4849

4950
// Catalog implements the cat.Catalog interface for testing purposes.
@@ -78,7 +79,7 @@ func New() *Catalog {
7879

7980
return &Catalog{
8081
testSchema: Schema{
81-
SchemaID: 1,
82+
SchemaID: testSchemaID,
8283
SchemaName: cat.SchemaName{
8384
CatalogName: testDB,
8485
SchemaName: catconstants.PublicSchemaName,
@@ -135,6 +136,17 @@ func (tc *Catalog) ResolveSchema(
135136
return tc.resolveSchema(&toResolve)
136137
}
137138

139+
// ResolveSchemaByID is part of the cat.Catalog interface.
140+
func (tc *Catalog) ResolveSchemaByID(
141+
_ context.Context, _ cat.Flags, id cat.StableID,
142+
) (cat.Schema, error) {
143+
if id != testSchemaID {
144+
return nil, pgerror.Newf(pgcode.InvalidSchemaName,
145+
"target database or schema does not exist")
146+
}
147+
return &tc.testSchema, nil
148+
}
149+
138150
// GetAllSchemaNamesForDB is part of the cat.Catalog interface.
139151
func (tc *Catalog) GetAllSchemaNamesForDB(
140152
ctx context.Context, dbName string,
@@ -844,6 +856,7 @@ func (tv *View) Trigger(i int) cat.Trigger {
844856
type Table struct {
845857
TabID cat.StableID
846858
DatabaseID descpb.ID
859+
SchemaID descpb.ID
847860
TabVersion int
848861
TabName tree.TableName
849862
Columns []cat.Column
@@ -1085,6 +1098,11 @@ func (tt *Table) GetDatabaseID() descpb.ID {
10851098
return tt.DatabaseID
10861099
}
10871100

1101+
// GetSchemaID is part of the cat.Table interface.
1102+
func (tt *Table) GetSchemaID() descpb.ID {
1103+
return tt.SchemaID
1104+
}
1105+
10881106
// IsHypothetical is part of the cat.Table interface.
10891107
func (tt *Table) IsHypothetical() bool {
10901108
return false

0 commit comments

Comments
 (0)