sql: properly support CTEs inside views

[knz]

Fixes #23833.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/logictest/testdata/logic_test/dependencies, line 140 at r4 (raw file):

----
descriptor_id  descriptor_name    index_id  dependedonby_id  dependedonby_type  dependedonby_index_id  dependedonby_name  dependedonby_details
63             blog_posts_id_seq  NULL      64               sequence           0                      NULL               Columns: [0]

I don't understand these test changes. Why are the descriptor ids changing?

---

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 284 at r4 (raw file):

4084598993  primary       393119649     0        NULL      NULL   0            0
4084598994  t3_a_b_idx    393119649     0        NULL      NULL   0            0
4252432642  v1            393119649     0        NULL      NULL   0            0

likewise, what caused these changes?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/logictest/testdata/logic_test/dependencies, line 140 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I don't understand these test changes. Why are the descriptor ids changing?

The previous code was performing name resolution of the table names on the AST during the semantic analysis, thereby acquiring a lease. This caused the subsequent transaction that was creating the view and update the leased table desc to abort with a retry error. However since the ID generation is not transactional, we'd "lose" a desc ID this way.

The new code skips this name resolution with lease (the name resolution is performed using uncached/unleased table descs instead) and so the retry error does not occur, so the first generated ID can be used.

---

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 284 at r4 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

likewise, what caused these changes?

ditto

[knz]

rebased, RFAL

[knz]

rebased, RFAL

[knz]

ping?

[knz]

ping?

[knz]

(just a rubber stamp since the same commits were approved in #31051)

[knz]

(just a rubber stamp since the same commits were approved in #31051)

[jordanlewis]

Oops, I didn't realize that I hadn't reviewed this before letting the backport go in... this all looks good to me but I wonder if @vivekmenezes should take a look with regards to the simpler lease stuff, which I don't quite understand.

[knz]

I think it's fair that Vivek and I look into this together again, because in truth I do not understand all of it myself.

However since this commit went into 2.1 already I will merge the PR as-is. If there are additional fixes needed I'll make a new PR.

Thanks

bors r+

[knz]

I think it's fair that Vivek and I look into this together again, because in truth I do not understand all of it myself.

However since this commit went into 2.1 already I will merge the PR as-is. If there are additional fixes needed I'll make a new PR.

Thanks

bors r+

[jordanlewis]

Sounds good

[jordanlewis]

Sounds good

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)