Skip to content

Commit ed733ad

Browse files
RaduBerindeRaduBerinde
committed
opt: add opaque operator flavors for mutations and DDL
The opaque operator assumes the worst and is tagged with Mutation and DDL. This prevents all opaque statements from running in read-only transactions. This change splits `OpaqueRel` into three flavors: `OpaqueRel`, `OpaqueMutation`, and `OpaqueDDL`. The difference is in the operator tags, which causes different behavior in the execbuilder. In addition, the opaque metadata `String()` now returns the `StatementTag` and is used in error messages. This was tested manually as all opaque operators are read-only so far. Fixes #39204. Release note: None
1 parent 13c802c commit ed733ad

9 files changed

Lines changed: 114 additions & 27 deletions

File tree

‎pkg/sql/logictest/testdata/logic_test/show_fingerprints‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,3 +110,15 @@ query TT
110110
SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE blocks
111111
----
112112
primary 590700560494856555
113+
114+
# Verify that we can show fingerprints from a read-only transaction (#39204).
115+
statement ok
116+
BEGIN TRANSACTION AS OF SYSTEM TIME '-1us'
117+
118+
query TT
119+
SHOW EXPERIMENTAL_FINGERPRINTS FROM TABLE t
120+
----
121+
primary 1205834892498753533
122+
123+
statement ok
124+
COMMIT

‎pkg/sql/opaque.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ package sql
1313
import (
1414
"context"
1515
"reflect"
16-
"strings"
1716

1817
"github.com/cockroachdb/cockroach/pkg/sql/opt"
1918
"github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder"
@@ -65,22 +64,22 @@ func buildOpaque(
6564
return nil, nil, err
6665
}
6766
res := &opaqueMetadata{
68-
info: strings.TrimPrefix(reflect.TypeOf(stmt).String(), "*tree."),
67+
info: stmt.StatementTag(),
6968
plan: plan,
7069
}
7170
return res, planColumns(plan), nil
7271
}
7372

7473
func init() {
75-
opaqueStmts := []reflect.Type{
74+
opaqueReadOnlyStmts := []reflect.Type{
7675
reflect.TypeOf(&tree.ShowClusterSetting{}),
7776
reflect.TypeOf(&tree.ShowHistogram{}),
7877
reflect.TypeOf(&tree.ShowTableStats{}),
7978
reflect.TypeOf(&tree.ShowTraceForSession{}),
8079
reflect.TypeOf(&tree.ShowZoneConfig{}),
8180
reflect.TypeOf(&tree.ShowFingerprints{}),
8281
}
83-
for _, t := range opaqueStmts {
84-
optbuilder.RegisterOpaque(t, buildOpaque)
82+
for _, t := range opaqueReadOnlyStmts {
83+
optbuilder.RegisterOpaque(t, optbuilder.OpaqueReadOnly, buildOpaque)
8584
}
8685
}

‎pkg/sql/opt/exec/execbuilder/relational.go‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,9 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) {
155155
}
156156

157157
// Raise error if mutation op is part of a read-only transaction.
158-
if opt.IsMutationOp(e) {
159-
if b.evalCtx.TxnReadOnly {
160-
return execPlan{}, pgerror.Newf(pgcode.ReadOnlySQLTransaction,
161-
"cannot execute %s in a read-only transaction", e.Op().SyntaxTag())
162-
}
158+
if opt.IsMutationOp(e) && b.evalCtx.TxnReadOnly {
159+
return execPlan{}, pgerror.Newf(pgcode.ReadOnlySQLTransaction,
160+
"cannot execute %s in a read-only transaction", b.statementTag(e))
163161
}
164162

165163
// Collect usage telemetry for relational node, if appropriate.
@@ -261,8 +259,8 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) {
261259
case *memo.ShowTraceForSessionExpr:
262260
ep, err = b.buildShowTrace(t)
263261

264-
case *memo.OpaqueRelExpr:
265-
ep, err = b.buildOpaque(t)
262+
case *memo.OpaqueRelExpr, *memo.OpaqueMutationExpr, *memo.OpaqueDDLExpr:
263+
ep, err = b.buildOpaque(t.Private().(*memo.OpaqueRelPrivate))
266264

267265
case *memo.AlterTableSplitExpr:
268266
ep, err = b.buildAlterTableSplit(t)
@@ -1778,7 +1776,7 @@ func (b *Builder) applySaveTable(
17781776
return input, err
17791777
}
17801778

1781-
func (b *Builder) buildOpaque(opaque *memo.OpaqueRelExpr) (execPlan, error) {
1779+
func (b *Builder) buildOpaque(opaque *memo.OpaqueRelPrivate) (execPlan, error) {
17821780
node, err := b.factory.ConstructOpaque(opaque.Metadata)
17831781
if err != nil {
17841782
return execPlan{}, err
@@ -1902,3 +1900,15 @@ func (b *Builder) buildSortedInput(input execPlan, ordering opt.Ordering) (execP
19021900
}
19031901
return execPlan{root: node, outputCols: input.outputCols}, nil
19041902
}
1903+
1904+
// statementTag returns a string that can be used in an error message regarding
1905+
// the given expression.
1906+
func (b *Builder) statementTag(expr memo.RelExpr) string {
1907+
switch expr.Op() {
1908+
case opt.OpaqueRelOp, opt.OpaqueMutationOp, opt.OpaqueDDLOp:
1909+
return expr.Private().(*memo.OpaqueRelPrivate).Metadata.String()
1910+
1911+
default:
1912+
return expr.Op().SyntaxTag()
1913+
}
1914+
}

‎pkg/sql/opt/memo/expr_format.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,9 +193,10 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) {
193193

194194
case *ScanExpr, *VirtualScanExpr, *IndexJoinExpr, *ShowTraceForSessionExpr,
195195
*InsertExpr, *UpdateExpr, *UpsertExpr, *DeleteExpr, *SequenceSelectExpr,
196-
*WindowExpr, *OpaqueRelExpr, *AlterTableSplitExpr, *AlterTableUnsplitExpr,
197-
*AlterTableUnsplitAllExpr, *AlterTableRelocateExpr, *ControlJobsExpr,
198-
*CancelQueriesExpr, *CancelSessionsExpr:
196+
*WindowExpr, *OpaqueRelExpr, *OpaqueMutationExpr, *OpaqueDDLExpr,
197+
*AlterTableSplitExpr, *AlterTableUnsplitExpr, *AlterTableUnsplitAllExpr,
198+
*AlterTableRelocateExpr, *ControlJobsExpr, *CancelQueriesExpr,
199+
*CancelSessionsExpr:
199200
fmt.Fprintf(f.Buffer, "%v", e.Op())
200201
FormatPrivate(f, e.Private(), required)
201202

‎pkg/sql/opt/memo/logical_props_builder.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,16 @@ func (b *logicalPropsBuilder) buildOpaqueRelProps(op *OpaqueRelExpr, rel *props.
811811
b.buildBasicProps(op, op.Columns, rel)
812812
}
813813

814+
func (b *logicalPropsBuilder) buildOpaqueMutationProps(
815+
op *OpaqueMutationExpr, rel *props.Relational,
816+
) {
817+
b.buildBasicProps(op, op.Columns, rel)
818+
}
819+
820+
func (b *logicalPropsBuilder) buildOpaqueDDLProps(op *OpaqueDDLExpr, rel *props.Relational) {
821+
b.buildBasicProps(op, op.Columns, rel)
822+
}
823+
814824
func (b *logicalPropsBuilder) buildAlterTableSplitProps(
815825
split *AlterTableSplitExpr, rel *props.Relational,
816826
) {

‎pkg/sql/opt/memo/statistics_builder.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,8 @@ func (sb *statisticsBuilder) colStat(colSet opt.ColSet, e RelExpr) *props.Column
342342
case opt.SequenceSelectOp:
343343
return sb.colStatSequenceSelect(colSet, e.(*SequenceSelectExpr))
344344

345-
case opt.ExplainOp, opt.ShowTraceForSessionOp, opt.OpaqueRelOp:
345+
case opt.ExplainOp, opt.ShowTraceForSessionOp,
346+
opt.OpaqueRelOp, opt.OpaqueMutationOp, opt.OpaqueDDLOp:
346347
return sb.colStatUnknown(colSet, e.Relational())
347348

348349
case opt.WithOp:

‎pkg/sql/opt/operator.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,8 +281,8 @@ func AggregateIsNullOnEmpty(op Operator) bool {
281281
type OpaqueMetadata interface {
282282
ImplementsOpaqueMetadata()
283283

284-
// String is used when printing optimizer trees and should contain a short
285-
// description of the statement.
284+
// String is a short description used when printing optimizer trees and when
285+
// forming error messages; it should be the SQL statement tag.
286286
String() string
287287
}
288288

‎pkg/sql/opt/ops/statement.opt‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,25 @@ define ShowTracePrivate {
104104
#
105105
# OpaqueRel can produce data and can be used as a data source as part of a
106106
# larger enclosing query.
107-
[Relational, DDL, Mutation]
107+
[Relational]
108108
define OpaqueRel {
109109
_ OpaqueRelPrivate
110110
}
111111

112+
# OpaqueMutation is a variant of OpaqueRel for operators that can mutate data as
113+
# part of the transaction.
114+
[Relational, Mutation]
115+
define OpaqueMutation {
116+
_ OpaqueRelPrivate
117+
}
118+
119+
# OpaqueMutation is a variant of OpaqueRel for operators that cause a schema
120+
# change and cannot be executed following a mutation in the same transaction.
121+
[Relational, Mutation, DDL]
122+
define OpaqueDDL {
123+
_ OpaqueRelPrivate
124+
}
125+
112126
[Private]
113127
define OpaqueRelPrivate {
114128
Columns ColList

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

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,34 +18,74 @@ import (
1818
"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
1919
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2020
"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
21+
"github.com/cockroachdb/errors"
2122
)
2223

2324
// BuildOpaqueFn is a handler for building the metadata for an opaque statement.
2425
type BuildOpaqueFn func(
2526
context.Context, *tree.SemaContext, *tree.EvalContext, tree.Statement,
2627
) (opt.OpaqueMetadata, sqlbase.ResultColumns, error)
2728

29+
// OpaqueType indicates whether an opaque statement can mutate data or change
30+
// schema.
31+
type OpaqueType int
32+
33+
const (
34+
// OpaqueReadOnly is used for statements that do not mutate state as part of
35+
// the transaction, and can be run in read-only transactions.
36+
OpaqueReadOnly OpaqueType = iota
37+
38+
// OpaqueMutation is used for statements that mutate data and cannot be run as
39+
// part of read-only transactions.
40+
OpaqueMutation
41+
42+
// OpaqueDDL is used for statements that change a schema and cannot be
43+
// executed following a mutation in the same transaction.
44+
OpaqueDDL
45+
)
46+
2847
// RegisterOpaque registers an opaque handler for a specific statement type.
29-
func RegisterOpaque(stmtType reflect.Type, fn BuildOpaqueFn) {
30-
opaqueStatements[stmtType] = fn
48+
func RegisterOpaque(stmtType reflect.Type, opaqueType OpaqueType, fn BuildOpaqueFn) {
49+
if _, ok := opaqueStatements[stmtType]; ok {
50+
panic(errors.AssertionFailedf("opaque statement %s already registered", stmtType))
51+
}
52+
opaqueStatements[stmtType] = opaqueStmtInfo{
53+
typ: opaqueType,
54+
buildFn: fn,
55+
}
56+
}
57+
58+
type opaqueStmtInfo struct {
59+
typ OpaqueType
60+
buildFn BuildOpaqueFn
3161
}
3262

33-
var opaqueStatements = make(map[reflect.Type]BuildOpaqueFn)
63+
var opaqueStatements = make(map[reflect.Type]opaqueStmtInfo)
3464

3565
func (b *Builder) tryBuildOpaque(stmt tree.Statement, inScope *scope) (outScope *scope) {
36-
fn, ok := opaqueStatements[reflect.TypeOf(stmt)]
66+
info, ok := opaqueStatements[reflect.TypeOf(stmt)]
3767
if !ok {
3868
return nil
3969
}
40-
obj, cols, err := fn(b.ctx, b.semaCtx, b.evalCtx, stmt)
70+
obj, cols, err := info.buildFn(b.ctx, b.semaCtx, b.evalCtx, stmt)
4171
if err != nil {
4272
panic(err)
4373
}
4474
outScope = inScope.push()
4575
b.synthesizeResultColumns(outScope, cols)
46-
outScope.expr = b.factory.ConstructOpaqueRel(&memo.OpaqueRelPrivate{
76+
private := &memo.OpaqueRelPrivate{
4777
Columns: colsToColList(outScope.cols),
4878
Metadata: obj,
49-
})
79+
}
80+
switch info.typ {
81+
case OpaqueReadOnly:
82+
outScope.expr = b.factory.ConstructOpaqueRel(private)
83+
case OpaqueMutation:
84+
outScope.expr = b.factory.ConstructOpaqueMutation(private)
85+
case OpaqueDDL:
86+
outScope.expr = b.factory.ConstructOpaqueDDL(private)
87+
default:
88+
panic(errors.AssertionFailedf("invalid opaque statement type %d", info.typ))
89+
}
5090
return outScope
5191
}

0 commit comments

Comments
 (0)