Skip to content

Commit

Permalink
sql: fix show_create_all_tables in explicit transactions
Browse files Browse the repository at this point in the history
Related to cockroachdb#68216.

Previously, running `crdb_internal.show_create_all_tables` in an
existing explicit transaction would crash for the reasons discussed
in cockroachdb#68216. This should not have crashed, but this also revealed that
the internal executor in the `showCreateAllTablesGenerator` was a little
confused. It was issuing AS OF SYSTEM TIME queries, but also supplying
its parent transaction. So it wanted to execute in the context of its
parent transaction, but also to run at a very specific timestamp -
contradictory desires.

This commit fixes this by removing the transactions from these uses of
the internal executor and letting the internal executor queries run at
their fixed timestamp.

An alternative to this is to do away with the non-transactional nature
of `showCreateAllTablesGenerator`. I don't understand why it is using
its own timestamp and issuing AS OF SYSTEM TIME queries at all, instead
of just using its parent transaction. But this all seems so deliberate
that I imagine there was a good reason to build it this way.

Example crash:
```
➜ ./cockroach demo --insecure

> BEGIN;

OPEN> SELECT count(*) FROM rides;
  count
---------
    500

OPEN> SELECT crdb_internal.show_create_all_tables('movr');
ERROR: internal error: crdb_internal.show_create_all_tables: unexpected batch read timestamp: 1627529007.202325000,0. Expected refreshed timestamp: 1627528898.793460000,0. ba: Scan [/Table/3/1,/Table/3/2), [txn: 3565aed1]. txn: "sql txn" meta={id=3565aed1 pri=0.05136598 epo=0 ts=1627529007.202325000,0 min=1627528898.793460000,0 seq=0} lock=false stat=PENDING rts=1627529007.202325000,0 wto=false gul=1627529007.202325000,0
SQLSTATE: XX000
DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go:160: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go:285: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_seq_num_allocator.go:105: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_heartbeater.go:240: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_coord_sender.go:513: Send()
github.com/cockroachdb/cockroach/pkg/kv/db.go:831: sendUsingSender()
github.com/cockroachdb/cockroach/pkg/kv/txn.go:982: Send()
github.com/cockroachdb/cockroach/pkg/kv/db.go:742: sendAndFill()
github.com/cockroachdb/cockroach/pkg/kv/txn.go:635: Run()
github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv/catalogkv.go:281: getAllDescriptorsAndMaybeNamespaceEntriesUnvalidated()
github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv/catalogkv.go:343: GetAllDescriptors()
github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/kv_descriptors.go:265: getAllDescriptors()
github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/collection.go:270: GetAllDescriptors()
github.com/cockroachdb/cockroach/pkg/sql/information_schema.go:2335: forEachTableDescWithTableLookupInternal()
github.com/cockroachdb/cockroach/pkg/sql/information_schema.go:2289: forEachTableDescWithTableLookup()
github.com/cockroachdb/cockroach/pkg/sql/pg_catalog.go:964: func1()
github.com/cockroachdb/cockroach/pkg/sql/virtual_schema.go:533: 1()
github.com/cockroachdb/cockroach/pkg/sql/virtual_table.go:120: func3()
github.com/cockroachdb/cockroach/pkg/util/stop/stopper.go:442: func2()
runtime/asm_amd64.s:1371: goexit()

HINT: You have encountered an unexpected error.

Please check the public issue tracker to check whether this problem is
already tracked. If you cannot find it there, please report the error
with details by creating a new issue.

If you would rather not post publicly, please contact us directly
using the support form.

We appreciate your feedback.
```
  • Loading branch information
nvanbenschoten committed Jul 29, 2021
1 parent 1b60983 commit 4a13b9a
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -369,3 +369,45 @@ query T
SELECT crdb_internal.show_create_all_tables('test_sequence')
----
CREATE SEQUENCE public.s1 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 123 START 1;

# Ensure that the builtin can be run in unused and used transactions.

statement ok
USE test_schema;

query T
BEGIN;
SELECT crdb_internal.show_create_all_tables('test_schema');
COMMIT;
----
CREATE TABLE sc1.t (
x INT8 NULL,
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
CONSTRAINT "primary" PRIMARY KEY (rowid ASC),
FAMILY "primary" (x, rowid)
);
CREATE TABLE sc2.t (
x INT8 NULL,
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
CONSTRAINT "primary" PRIMARY KEY (rowid ASC),
FAMILY "primary" (x, rowid)
);

query T
BEGIN;
SELECT * FROM sc1.t;
SELECT crdb_internal.show_create_all_tables('test_schema');
COMMIT;
----
CREATE TABLE sc1.t (
x INT8 NULL,
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
CONSTRAINT "primary" PRIMARY KEY (rowid ASC),
FAMILY "primary" (x, rowid)
);
CREATE TABLE sc2.t (
x INT8 NULL,
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
CONSTRAINT "primary" PRIMARY KEY (rowid ASC),
FAMILY "primary" (x, rowid)
);
10 changes: 4 additions & 6 deletions pkg/sql/sem/builtins/generator_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -1691,7 +1691,6 @@ const (
// crdb_internal.show_create_all_tables(dbName).
type showCreateAllTablesGenerator struct {
ie sqlutil.InternalExecutor
txn *kv.Txn
timestamp string
ids []int64
dbName string
Expand All @@ -1714,7 +1713,7 @@ func (s *showCreateAllTablesGenerator) ResolvedType() *types.T {
}

// Start implements the tree.ValueGenerator interface.
func (s *showCreateAllTablesGenerator) Start(ctx context.Context, txn *kv.Txn) error {
func (s *showCreateAllTablesGenerator) Start(ctx context.Context, _ *kv.Txn) error {
// Note: All the table ids are accumulated in ram before the generator
// starts generating values.
// This is reasonable under the assumption that:
Expand All @@ -1727,15 +1726,14 @@ func (s *showCreateAllTablesGenerator) Start(ctx context.Context, txn *kv.Txn) e
// We also account for the memory in the BoundAccount memory monitor in
// showCreateAllTablesGenerator.
ids, err := getTopologicallySortedTableIDs(
ctx, s.ie, txn, s.dbName, s.timestamp, &s.acc,
ctx, s.ie, s.dbName, s.timestamp, &s.acc,
)
if err != nil {
return err
}

s.ids = ids

s.txn = txn
s.idx = -1
s.phase = create
return nil
Expand All @@ -1753,7 +1751,7 @@ func (s *showCreateAllTablesGenerator) Next(ctx context.Context) (bool, error) {
}

createStmt, err := getCreateStatement(
ctx, s.ie, s.txn, s.ids[s.idx], s.timestamp, s.dbName,
ctx, s.ie, s.ids[s.idx], s.timestamp, s.dbName,
)
if err != nil {
return false, err
Expand Down Expand Up @@ -1799,7 +1797,7 @@ func (s *showCreateAllTablesGenerator) Next(ctx context.Context) (bool, error) {
statementReturnType = alterValidateFKStatements
}
alterStmt, err := getAlterStatements(
ctx, s.ie, s.txn, s.ids[s.idx], s.timestamp, s.dbName, statementReturnType,
ctx, s.ie, s.ids[s.idx], s.timestamp, s.dbName, statementReturnType,
)
if err != nil {
return false, err
Expand Down
28 changes: 8 additions & 20 deletions pkg/sql/sem/builtins/show_create_all_tables_builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"sort"
"unsafe"

"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/sql/memsize"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
Expand Down Expand Up @@ -49,14 +48,9 @@ const foreignKeyValidationWarning = "-- Validate foreign key constraints. These
// the table that uses the sequence).
// The tables are sorted by table id first to guarantee stable ordering.
func getTopologicallySortedTableIDs(
ctx context.Context,
ie sqlutil.InternalExecutor,
txn *kv.Txn,
dbName string,
ts string,
acc *mon.BoundAccount,
ctx context.Context, ie sqlutil.InternalExecutor, dbName string, ts string, acc *mon.BoundAccount,
) ([]int64, error) {
ids, err := getTableIDs(ctx, ie, txn, ts, dbName, acc)
ids, err := getTableIDs(ctx, ie, ts, dbName, acc)
if err != nil {
return nil, err
}
Expand All @@ -80,7 +74,7 @@ func getTopologicallySortedTableIDs(
it, err := ie.QueryIteratorEx(
ctx,
"crdb_internal.show_create_all_tables",
txn,
nil, /* txn */
sessiondata.NoSessionDataOverride,
query,
tid,
Expand Down Expand Up @@ -155,12 +149,7 @@ func getTopologicallySortedTableIDs(
// getTableIDs returns the set of table ids from
// crdb_internal.show_create_all_tables for a specified database.
func getTableIDs(
ctx context.Context,
ie sqlutil.InternalExecutor,
txn *kv.Txn,
ts string,
dbName string,
acc *mon.BoundAccount,
ctx context.Context, ie sqlutil.InternalExecutor, ts string, dbName string, acc *mon.BoundAccount,
) ([]int64, error) {
query := fmt.Sprintf(`
SELECT descriptor_id
Expand All @@ -173,7 +162,7 @@ func getTableIDs(
it, err := ie.QueryIteratorEx(
ctx,
"crdb_internal.show_create_all_tables",
txn,
nil, /* txn */
sessiondata.NoSessionDataOverride,
query,
dbName,
Expand Down Expand Up @@ -246,7 +235,7 @@ func topologicalSort(
// getCreateStatement gets the create statement to recreate a table (ignoring fks)
// for a given table id in a database.
func getCreateStatement(
ctx context.Context, ie sqlutil.InternalExecutor, txn *kv.Txn, id int64, ts string, dbName string,
ctx context.Context, ie sqlutil.InternalExecutor, id int64, ts string, dbName string,
) (tree.Datum, error) {
query := fmt.Sprintf(`
SELECT
Expand All @@ -258,7 +247,7 @@ func getCreateStatement(
row, err := ie.QueryRowEx(
ctx,
"crdb_internal.show_create_all_tables",
txn,
nil, /* txn */
sessiondata.NoSessionDataOverride,
query,
id,
Expand All @@ -275,7 +264,6 @@ func getCreateStatement(
func getAlterStatements(
ctx context.Context,
ie sqlutil.InternalExecutor,
txn *kv.Txn,
id int64,
ts string,
dbName string,
Expand All @@ -291,7 +279,7 @@ func getAlterStatements(
row, err := ie.QueryRowEx(
ctx,
"crdb_internal.show_create_all_tables",
txn,
nil, /* txn */
sessiondata.NoSessionDataOverride,
query,
id,
Expand Down

0 comments on commit 4a13b9a

Please sign in to comment.