sql: add a view dep on sequence string arguments

[RichardJCai]

Add a dependency on sequences that are used via string argument when creating a view.

Also fixed minor bug where column dependencies were not being added when a sequence is referred to in a currval call.

Fixes (part of) #24556 and #50033

Release note (sql change): Sequences passed as a string argument into views
are now tracked as a dependency. Example: CREATE VIEW v AS SELECT nextval('s')
will now add a dependency on sequence s.

[cockroach-teamcity]

This change is 

[RaduBerinde]

So.. if nextval('s') is inside a CASE and it wouldn't necessarily get executed, do we want to error out if the source doesn't exist? E.g. this works in postgres:

radu=> CREATE VIEW v AS SELECT CASE WHEN EXISTS (SELECT * FROM t WHERE x=1) THEN nextval('s') ELSE 0 END;
CREATE VIEW
radu=> SELECT * FROM v;
ERROR:  "s" is not a sequence
radu=> DELETE FROM t WHERE true;
DELETE 1
radu=> SELECT * FROM v;
 case 
------
    0
(1 row)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

[RichardJCai]

So.. if nextval('s') is inside a CASE and it wouldn't necessarily get executed, do we want to error out if the source doesn't exist? E.g. this works in postgres:

radu=> CREATE VIEW v AS SELECT CASE WHEN EXISTS (SELECT * FROM t WHERE x=1) THEN nextval('s') ELSE 0 END;
CREATE VIEW
radu=> SELECT * FROM v;
ERROR:  "s" is not a sequence
radu=> DELETE FROM t WHERE true;
DELETE 1
radu=> SELECT * FROM v;
 case 
------
    0
(1 row)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

So would it be correct if we didn't error out if the source didn't exist?

This case is really strange to me, is this the only one you can think of that results in this behaviour? Where it's valid to have a call referring to a sequence where the sequence may not exist.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)

---

pkg/sql/opt/optbuilder/scalar.go, line 512 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

What if resolve datasource returns an error? I think you only want to ignore it if it is a "UnknownDescriptor" error (or something like that)

The second arg is a name, not an error. Errors are returned via panics that are caught at the optbuilder entrpoint.

That said, I agree that we probably want to tolerate "UnknownDescriptor" here.. Postgres definitely allows you to create a view with a nextval call for a bogus sequence.

[RichardJCai]

So.. if nextval('s') is inside a CASE and it wouldn't necessarily get executed, do we want to error out if the source doesn't exist? E.g. this works in postgres:

radu=> CREATE VIEW v AS SELECT CASE WHEN EXISTS (SELECT * FROM t WHERE x=1) THEN nextval('s') ELSE 0 END;
CREATE VIEW
radu=> SELECT * FROM v;
ERROR:  "s" is not a sequence
radu=> DELETE FROM t WHERE true;
DELETE 1
radu=> SELECT * FROM v;
 case 
------
    0
(1 row)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

Wait what version of PG are you using @RaduBerinde , I'm using PG 12 and this does raise an error.

postgres=# create table t(x int);
CREATE TABLE
postgres=# CREATE VIEW v AS SELECT CASE WHEN EXISTS (SELECT * FROM t WHERE x=1) THEN nextval('s') ELSE 0 END;
ERROR:  relation "s" does not exist
LINE 1: ...N EXISTS (SELECT * FROM t WHERE x=1) THEN nextval('s') ELSE ...
                                                             ^
postgres=# 

Or maybe your error comes from s existing but being a table instead of a sequence?
ERROR: "s" is not a sequence

[RaduBerinde]

Oh, wow, good catch, yeah it looks like I had a table 's' that I forgot about. I retract my comments then, erroring out seems like the correct thing to do.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)

[RichardJCai]

@RaduBerinde Do you want to take another look before I merge this?

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde, @RichardJCai, and @rohany)

[RichardJCai]

Thanks for the reviews!

[RichardJCai]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[RichardJCai]

I think TestDistinct is flakey, although I noticed one of my comments is out of date so I'm gonna update that and retry.

[RichardJCai]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)