cli/dump: fails with column-less tables

[lopezator]

To Reproduce

Given an empty database:

CREATE DATABASE foo;
CREATE TABLE bar();

$> cockroach dump --insecure foo

Results in:

CREATE TABLE bar (,
	FAMILY "primary" (rowid)
);
Error: pq: syntax error at or near "from"
Failed running "dump"

Quite evident in the example but can very frustating in a large database.

CC \ @knz

[knz]

Note for the reader: the problem is not create table accepting an empty column list.
(We could discuss that but it's not in scope.)

The problem is really in cockroach dump and it's possible to reproduce by creating a regular table, and using alter table drop column to remove all the visible columns.

(Such usage of alter is supported by postgres so we can't just disallow it)

[knz]

cc #28948.

[knz]

Added clearer error in code in #38725, to remove the UX-surprise label.

[jseldess]

Known limitation documented in cockroachdb/docs#5671.

[C0rWin]

Hey,

I would like to take a look and address the issue described here. However, currently looking into this ticket I have few reservations with what should be expected outcome or behavior in such a case. Moreover, I do not really understand the use case or what is the rational behind columns less table.

Anyway, would it be correct to expect that given

CREATE DATABASE foo;
CREATE TABLE bar();

the output should be

CREATE TABLE bar (FAMILY "primary" (rowid)
);

INSERT INTO bar (rowid) VALUES
        (XXXXXXX),
        (YYYYYYY);
….
….
        (ZZZZZZZZ);

did I get it right?

[knz]

I do not really understand the use case or what is the rational behind columns less table.

I think it's useful to think about "Column-less tables" as "tables with no visible columns" (but where they may be non-visible columns). Also, these are a regular feature of most SQL engines, so regardless of whether you can discern a purpose to them they're just a fact of life that crdb has to support well.

the output should be [...]

The INSERT part I agree with. In fact, I would even strongly recommend to make the hidden columns (incl rowid) explicit for all tables, not just column-less tables.

As to the CREATE part, it's definitely odd to see the rowid column mentioned in the FAMILY but not being defined explicitly.
On the other hand, it would be a mistake to emit CREATE TABLE ...(rowid INTEGER DEFAULT ... because then the column would flip from non-visible to visible when restoring a dump.

I think there are two steps here:

1. make it possible to use the word HIDDEN in CREATE, to mark a column as non-visible. In essence this would be reviving PR sql: add proper support for hidden columns #26644

2. use the new HIDDEN word when generating the CREATE statement in cockroach dump

What do you think?

[C0rWin]

I think there are two steps here:

1. make it possible to use the word `HIDDEN` in CREATE, to mark a column as non-visible. In essence this would be reviving PR #26644

2. use the new HIDDEN word when generating the CREATE statement in `cockroach dump`

What do you think?

Isn't it "too intrusive" change? Just to make sure I've got it correctly, you suggestions is to explicitly mention non-visible columns within CREATE statement, hence it will appear during the dump. E.g.

CREATE TABLE foo(rowid int HIDDEN,
FAMILY "primary" (rowid))

I'm not an expert but it seems quite intrusive change relatively to changing behavior of showFamilyClause to handle non-visible columns. For instance I've played a bit with code and got following change https://gist.github.com/C0rWin/90424d770de16cb0ec3c7ad8bc180a26.

Probably it might make sense to divide the effort and introduce a change where we change showFamiliyClause, then adding support to enhance CREATE statement with ability to specify hidden columns explicitly. Will something like this make sense?

[knz]

I agree these two changes should be done in separate commits.
Also you can probably reuse some code from #26644.

(nit: perhaps NOT VISIBLE is the better syntax choice, rather than HIDDEN).

[C0rWin]

@knz create a PR#46406 which achieves functionality claimed in this ticket. Would something like this make sense?

[lopezator]

Thanks for addressing the issue @C0rWin !!