Merge #142572 #144646

craig[bot] · alectric-tr · jeffswenson · craig[bot] · commit 642081fb8d01 · 2025-04-17T21:15:03.000Z
142572: sql: syntactic for support SET LOGGED, SET UNLOGGED r=rafiss a=xelathan Fixes: #109951 Jira issue: [CRDB-31177](https://cockroachlabs.atlassian.net/browse/CRDB-31177) This PR only adds syntactic support for setting logged and unlogged tables. SET LOGGED/UNLOGGED syntax will be a no-op. 144646: bulk: disable async flush code path r=jeffswenson a=jeffswenson his PR disables the async flush path in the SST batcher due to a correctness bug. When a flush crosses a range boundary, it triggers an async goroutine to handle the RPC. Previously, any error from this goroutine would propagate through the top-level Flush call. However, PR #110218 modified Reset to wait for all async tasks to complete. This change assumed Reset was only used in cleanup, but it is also called by flushIfNeeded after each doFlush. As a result, any error is consumed and silently dropped before Flush is invoked. Disabling async flush won't impact performance because Reset already waits for the async flush, making every flush effectively synchronous. Informs: #143690 Informs: #144650 Release note: Fix rare corruption bug that impacts import and materialized views. Co-authored-by: xelathan <alex.t.tran@gmail.com> Co-authored-by: Jeff Swenson <jeffswenson@betterthannull.com>
diff --git a/docs/generated/sql/bnf/BUILD.bazel b/docs/generated/sql/bnf/BUILD.bazel
@@ -57,6 +57,7 @@ FILES = [
     "alter_table",
     "alter_table_cmds",
     "alter_table_locality_stmt",
+    "alter_table_logged_stmt",
     "alter_table_owner_stmt",
     "alter_table_partition_by",
     "alter_table_reset_storage_param",
diff --git a/docs/generated/sql/bnf/alter_table.bnf b/docs/generated/sql/bnf/alter_table.bnf
@@ -14,5 +14,6 @@ alter_table_stmt ::=
 	| 'ALTER' 'TABLE' 'IF' 'EXISTS' table_name 'SET' 'SCHEMA' schema_name
 	| 'ALTER' 'TABLE' table_name 'SET' locality
 	| 'ALTER' 'TABLE' 'IF' 'EXISTS' table_name 'SET' locality
+	| alter_table_logged_stmt
 	| 'ALTER' 'TABLE' table_name 'OWNER' 'TO' role_spec
 	| 'ALTER' 'TABLE' 'IF' 'EXISTS' table_name 'OWNER' 'TO' role_spec
diff --git a/docs/generated/sql/bnf/alter_table_logged_stmt.bnf b/docs/generated/sql/bnf/alter_table_logged_stmt.bnf
@@ -0,0 +1,5 @@
+alter_table_logged_stmt ::=
+	'ALTER' 'TABLE' relation_expr 'SET' 'LOGGED'
+	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'SET' 'LOGGED'
+	| 'ALTER' 'TABLE' relation_expr 'SET' 'UNLOGGED'
+	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'SET' 'UNLOGGED'
diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -1282,6 +1282,7 @@ unreserved_keyword ::=
 	| 'LOGICALLY'
 	| 'LOGIN'
 	| 'LOCALITY'
+	| 'LOGGED'
 	| 'LOOKUP'
 	| 'LOW'
 	| 'MATCH'
@@ -1702,6 +1703,7 @@ alter_table_stmt ::=
 	| alter_rename_table_stmt
 	| alter_table_set_schema_stmt
 	| alter_table_locality_stmt
+	| alter_table_logged_stmt
 	| alter_table_owner_stmt
 
 alter_index_stmt ::=
@@ -2274,6 +2276,12 @@ alter_table_locality_stmt ::=
 	'ALTER' 'TABLE' relation_expr 'SET' locality
 	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'SET' locality
 
+alter_table_logged_stmt ::=
+	'ALTER' 'TABLE' relation_expr 'SET' 'LOGGED'
+	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'SET' 'LOGGED'
+	| 'ALTER' 'TABLE' relation_expr 'SET' 'UNLOGGED'
+	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'SET' 'UNLOGGED'
+
 alter_table_owner_stmt ::=
 	'ALTER' 'TABLE' relation_expr 'OWNER' 'TO' role_spec
 	| 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'OWNER' 'TO' role_spec
@@ -4088,6 +4096,7 @@ bare_label_keywords ::=
 	| 'LOCALTIME'
 	| 'LOCALTIMESTAMP'
 	| 'LOCKED'
+	| 'LOGGED'
 	| 'LOGICAL'
 	| 'LOGICALLY'
 	| 'LOGIN'
diff --git a/pkg/gen/bnf.bzl b/pkg/gen/bnf.bzl
@@ -57,6 +57,7 @@ BNF_SRCS = [
     "//docs/generated/sql/bnf:alter_table.bnf",
     "//docs/generated/sql/bnf:alter_table_cmds.bnf",
     "//docs/generated/sql/bnf:alter_table_locality_stmt.bnf",
+    "//docs/generated/sql/bnf:alter_table_logged_stmt.bnf",
     "//docs/generated/sql/bnf:alter_table_owner_stmt.bnf",
     "//docs/generated/sql/bnf:alter_table_partition_by.bnf",
     "//docs/generated/sql/bnf:alter_table_reset_storage_param.bnf",
diff --git a/pkg/gen/diagrams.bzl b/pkg/gen/diagrams.bzl
@@ -57,6 +57,7 @@ DIAGRAMS_SRCS = [
     "//docs/generated/sql/bnf:alter_table.html",
     "//docs/generated/sql/bnf:alter_table_cmds.html",
     "//docs/generated/sql/bnf:alter_table_locality.html",
+    "//docs/generated/sql/bnf:alter_table_logged.html",
     "//docs/generated/sql/bnf:alter_table_owner.html",
     "//docs/generated/sql/bnf:alter_table_partition_by.html",
     "//docs/generated/sql/bnf:alter_table_reset_storage_param.html",
diff --git a/pkg/gen/docs.bzl b/pkg/gen/docs.bzl
@@ -67,6 +67,7 @@ DOCS_SRCS = [
     "//docs/generated/sql/bnf:alter_table.bnf",
     "//docs/generated/sql/bnf:alter_table_cmds.bnf",
     "//docs/generated/sql/bnf:alter_table_locality_stmt.bnf",
+    "//docs/generated/sql/bnf:alter_table_logged_stmt.bnf",
     "//docs/generated/sql/bnf:alter_table_owner_stmt.bnf",
     "//docs/generated/sql/bnf:alter_table_partition_by.bnf",
     "//docs/generated/sql/bnf:alter_table_reset_storage_param.bnf",
diff --git a/pkg/kv/bulk/sst_batcher.go b/pkg/kv/bulk/sst_batcher.go
@@ -692,7 +692,17 @@ func (b *SSTBatcher) doFlush(ctx context.Context, reason int) error {
 	// the next one; if memory is not available we'll just block on the send
 	// and then move on to the next send after this SST is no longer being held
 	// in memory.
-	flushAsync := reason == rangeFlush
+	//
+	// TODO(jeffswenson): re-enable flush async after fixing performance and
+	// correctness issues.
+	//
+	// CORRECTNESS: Something has to surface the error from the async flush to
+	// the caller. Right now the error is logged by `Reset`.
+	// PERFORMANCE: The only caller that sets `rangeFlush` calls Reset immediatly
+	// after, which blocks on all in flight requests. So there is no performance
+	// benefit to the async flush.
+	//flushAsync := reason == rangeFlush
+	flushAsync := false
 
 	var reserved int64
 	if flushAsync {
diff --git a/pkg/sql/importer/read_import_pgdump.go b/pkg/sql/importer/read_import_pgdump.go
@@ -757,6 +757,9 @@ func readPostgresStmt(
 			return unsupportedStmtLogger.log(stmt.String(), false /* isParseError */)
 		}
 		return wrapErrorWithUnsupportedHint(errors.Errorf("unsupported statement: %s", stmt))
+	case *tree.AlterTableSetLogged:
+		// No-op: CockroachDB does not support unlogged tables, tables are logged by default
+		return nil
 	case *tree.CreateSequence:
 		schemaQualifiedTableName, err := getSchemaAndTableName(&stmt.Name)
 		if err != nil {
diff --git a/pkg/sql/opaque.go b/pkg/sql/opaque.go
@@ -126,6 +126,8 @@ func planOpaque(ctx context.Context, p *planner, stmt tree.Statement) (planNode,
 		return p.AlterTableLocality(ctx, n)
 	case *tree.AlterTableOwner:
 		return p.AlterTableOwner(ctx, n)
+	case *tree.AlterTableSetLogged:
+		return p.AlterTableSetLogged(ctx, n)
 	case *tree.AlterTableSetSchema:
 		return p.AlterTableSetSchema(ctx, n)
 	case *tree.AlterTenantCapability:
@@ -346,6 +348,7 @@ func init() {
 		&tree.AlterTable{},
 		&tree.AlterTableLocality{},
 		&tree.AlterTableOwner{},
+		&tree.AlterTableSetLogged{},
 		&tree.AlterTableSetSchema{},
 		&tree.AlterTenantCapability{},
 		&tree.AlterTenantRename{},
diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y
@@ -1025,7 +1025,7 @@ func (u *sqlSymUnion) doBlockOption() tree.DoBlockOption {
 %token <str> LABEL LANGUAGE LAST LATERAL LATEST LC_CTYPE LC_COLLATE
 %token <str> LEADING LEASE LEAST LEAKPROOF LEFT LESS LEVEL LIKE LIMIT
 %token <str> LINESTRING LINESTRINGM LINESTRINGZ LINESTRINGZM
-%token <str> LIST LOCAL LOCALITY LOCALTIME LOCALTIMESTAMP LOCKED LOGICAL LOGICALLY LOGIN LOOKUP LOW LSHIFT
+%token <str> LIST LOCAL LOCALITY LOCALTIME LOCALTIMESTAMP LOCKED LOGGED LOGICAL LOGICALLY LOGIN LOOKUP LOW LSHIFT
 
 %token <str> MATCH MATERIALIZED MERGE MINVALUE MAXVALUE METHOD MINUTE MODIFYCLUSTERSETTING MODIFYSQLCLUSTERSETTING MODE MONTH MOVE
 %token <str> MULTILINESTRING MULTILINESTRINGM MULTILINESTRINGZ MULTILINESTRINGZM
@@ -1150,6 +1150,7 @@ func (u *sqlSymUnion) doBlockOption() tree.DoBlockOption {
 %type <tree.Statement> alter_zone_table_stmt
 %type <tree.Statement> alter_table_set_schema_stmt
 %type <tree.Statement> alter_table_locality_stmt
+%type <tree.Statement> alter_table_logged_stmt
 %type <tree.Statement> alter_table_owner_stmt
 
 // ALTER VIRTUAL CLUSTER
@@ -2012,6 +2013,7 @@ alter_table_stmt:
 | alter_rename_table_stmt
 | alter_table_set_schema_stmt
 | alter_table_locality_stmt
+| alter_table_logged_stmt
 | alter_table_owner_stmt
 // ALTER TABLE has its error help token here because the ALTER TABLE
 // prefix is spread over multiple non-terminals.
@@ -12646,6 +12648,40 @@ locality:
     }
   }
 
+alter_table_logged_stmt:
+  ALTER TABLE relation_expr SET LOGGED
+  {
+    $$.val = &tree.AlterTableSetLogged{
+      Name: $3.unresolvedObjectName(),
+      IsLogged: true,
+      IfExists: false,
+    }
+  }
+| ALTER TABLE IF EXISTS relation_expr SET LOGGED
+  {
+    $$.val = &tree.AlterTableSetLogged{
+      Name: $5.unresolvedObjectName(),
+      IsLogged: true,
+      IfExists: true,
+    }
+  }
+| ALTER TABLE relation_expr SET UNLOGGED
+  {
+    $$.val = &tree.AlterTableSetLogged{
+      Name: $3.unresolvedObjectName(),
+      IsLogged: false,
+      IfExists: false,
+    }
+  }
+| ALTER TABLE IF EXISTS relation_expr SET UNLOGGED
+  {
+    $$.val = &tree.AlterTableSetLogged{
+      Name: $5.unresolvedObjectName(),
+      IsLogged: false,
+      IfExists: true,
+    }
+  }
+
 alter_table_owner_stmt:
   ALTER TABLE relation_expr OWNER TO role_spec
   {
@@ -18301,6 +18337,7 @@ unreserved_keyword:
 | LOGICALLY
 | LOGIN
 | LOCALITY
+| LOGGED
 | LOOKUP
 | LOW
 | MATCH
@@ -18869,6 +18906,7 @@ bare_label_keywords:
 | LOCALTIME
 | LOCALTIMESTAMP
 | LOCKED
+| LOGGED
 | LOGICAL
 | LOGICALLY
 | LOGIN
diff --git a/pkg/sql/parser/testdata/alter_table b/pkg/sql/parser/testdata/alter_table
@@ -160,6 +160,22 @@ ALTER TABLE a SET LOCALITY REGIONAL BY ROW AS bobby -- fully parenthesized
 ALTER TABLE a SET LOCALITY REGIONAL BY ROW AS bobby -- literals removed
 ALTER TABLE _ SET LOCALITY REGIONAL BY ROW AS _ -- identifiers removed
 
+parse
+ALTER TABLE a SET LOGGED
+----
+ALTER TABLE a SET LOGGED
+ALTER TABLE a SET LOGGED -- fully parenthesized
+ALTER TABLE a SET LOGGED -- literals removed
+ALTER TABLE _ SET LOGGED -- identifiers removed
+
+parse
+ALTER TABLE a SET UNLOGGED
+----
+ALTER TABLE a SET UNLOGGED
+ALTER TABLE a SET UNLOGGED -- fully parenthesized
+ALTER TABLE a SET UNLOGGED -- literals removed
+ALTER TABLE _ SET UNLOGGED -- identifiers removed
+
 parse
 ALTER TABLE a ADD COLUMN b INT8, ADD CONSTRAINT a_idx UNIQUE (a)
 ----
diff --git a/pkg/sql/sem/tree/alter_table.go b/pkg/sql/sem/tree/alter_table.go
@@ -714,6 +714,27 @@ func (node *AlterTableSetSchema) TelemetryName() string {
 	return "set_schema"
 }
 
+// AlterTableSetLogged represents an ALTER TABLE SET {LOGGED | UNLOGGED}
+type AlterTableSetLogged struct {
+	Name     *UnresolvedObjectName
+	IfExists bool
+	IsLogged bool
+}
+
+// Format implements the NodeFormatter interface.
+func (node *AlterTableSetLogged) Format(ctx *FmtCtx) {
+	ctx.WriteString("ALTER TABLE ")
+	if node.IfExists {
+		ctx.WriteString("IF EXISTS ")
+	}
+	ctx.FormatNode(node.Name)
+	if node.IsLogged {
+		ctx.WriteString(" SET LOGGED")
+	} else {
+		ctx.WriteString(" SET UNLOGGED")
+	}
+}
+
 // AlterTableOwner represents an ALTER TABLE OWNER TO command.
 type AlterTableOwner struct {
 	Name           *UnresolvedObjectName
diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go
@@ -505,6 +505,17 @@ func (*AlterTableOwner) StatementTag() string { return "ALTER TABLE" }
 
 func (*AlterTableOwner) hiddenFromShowQueries() {}
 
+// StatementType implements the Statement interface.
+func (*AlterTableSetLogged) StatementReturnType() StatementReturnType { return DDL }
+
+// StatementType implements the Statement interface.
+func (*AlterTableSetLogged) StatementType() StatementType { return TypeDDL }
+
+// StatementTag returns a short string identifying the type of statement.
+func (*AlterTableSetLogged) StatementTag() string { return "ALTER TABLE" }
+
+func (*AlterTableSetLogged) hiddenFromShowQueries() {}
+
 // StatementReturnType implements the Statement interface.
 func (*AlterTableSetSchema) StatementReturnType() StatementReturnType { return DDL }
 
@@ -2491,6 +2502,7 @@ func (n *AlterTableSetDefault) String() string                { return AsString(
 func (n *AlterTableSetVisible) String() string                { return AsString(n) }
 func (n *AlterTableSetNotNull) String() string                { return AsString(n) }
 func (n *AlterTableOwner) String() string                     { return AsString(n) }
+func (n *AlterTableSetLogged) String() string                 { return AsString(n) }
 func (n *AlterTableSetSchema) String() string                 { return AsString(n) }
 func (n *AlterTenantCapability) String() string               { return AsString(n) }
 func (n *AlterTenantSetClusterSetting) String() string        { return AsString(n) }
diff --git a/pkg/sql/unimplemented.go b/pkg/sql/unimplemented.go
@@ -11,6 +11,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/docs"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
@@ -74,3 +75,21 @@ func makeUnimplementedLegacyError(stmtSyntax redact.SafeString) error {
 		dscDocDetail,
 	)
 }
+
+// AlterTableSetLogged set table as unlogged or logged.
+// No-op since unlogged tables are not supported.
+func (p *planner) AlterTableSetLogged(
+	ctx context.Context, n *tree.AlterTableSetLogged,
+) (planNode, error) {
+	operation := redact.SafeString("LOGGED")
+	if !n.IsLogged {
+		operation = redact.SafeString("UNLOGGED")
+	}
+	p.BufferClientNotice(
+		ctx, pgnotice.Newf(
+			"SET %s is not supported and has no effect",
+			operation,
+		),
+	)
+	return nil, nil
+}
