{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22371","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22371/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22371/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22371/events","html_url":"https://github.com/cockroachdb/cockroach/pull/22371","id":294280421,"number":22371,"title":"sql, *: make CockroachDB understand schema names","user":{"login":"knz","id":642886,"avatar_url":"https://avatars3.githubusercontent.com/u/642886?v=4","gravatar_id":"","url":"https://api.github.com/users/knz","html_url":"https://github.com/knz","followers_url":"https://api.github.com/users/knz/followers","following_url":"https://api.github.com/users/knz/following{/other_user}","gists_url":"https://api.github.com/users/knz/gists{/gist_id}","starred_url":"https://api.github.com/users/knz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/knz/subscriptions","organizations_url":"https://api.github.com/users/knz/orgs","repos_url":"https://api.github.com/users/knz/repos","events_url":"https://api.github.com/users/knz/events{/privacy}","received_events_url":"https://api.github.com/users/knz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":19,"created_at":"2018-02-05T05:46:35Z","updated_at":"2018-02-12T15:58:50Z","closed_at":null,"author_association":"OWNER","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/22371","html_url":"https://github.com/cockroachdb/cockroach/pull/22371","diff_url":"https://github.com/cockroachdb/cockroach/pull/22371.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/22371.patch"},"body":"\"Why\":\r\n\r\nPrior to this patch, CockroachDB only recognized names of the form\r\n\"db.tbl\", like MySQL, whereas PostgreSQL compatibility mandates that\r\n\"db.public.tbl\" be also valid. This needed a change.\r\n\r\n### \"What\"\r\n\r\nThe visible tip of this iceberg patch is as follows:\r\n\r\n- new name resolution functions which are more correct and more easy\r\n  to use than the previous code!\r\n\r\n  Use `Normalize()` on a `TableName`, then `ResolveExistingObject()`\r\n  or `ResolveTargetObject()`.\r\n\r\n- this in turn relies on generic, well-abstracted name resolution\r\n  algorithms in `sem/tree/name_resolution.go`, the definition\r\n  of which closely follows the specs in the accompanying RFC (#21456).\r\n\r\n  This handles the pg syntax for catalog and schema names, together\r\n  with compatibility rules with CockroachDB v1.x.\r\n\r\n- a new schema accessor interface that truly encapsulates schema access!\r\n  Check out the gorgeous documentation in `sql/schema_accessors.go`.\r\n\r\n  In particular:\r\n\r\n  - `planner` now implements a new interface `SchemaResolver`. Use it.\r\n  - if you are not happy with `SchemaResolver` directly, use\r\n    `SchemaAccessor` or its various consistuent interfaces.\r\n\r\n    Depending on whether virtual schemas are to be considered, consider\r\n    alternatively `planner.LogicalSchemaAccessor()` and\r\n    `planner.PhysicalSchemaAccessor()`.\r\n\r\nOne may be surprised to see this schema accessor work happen in the\r\nsame patch. This was, unfortunately, a requirement to successfully\r\nplug catalog and schema name resolution in all the right places.\r\nAlso, it incidentally answers a long-standing demand for a better\r\ninterface to access descriptors.\r\n\r\n### \"How\"\r\n\r\nThe itinerary to achieve this brought me through the following steps:\r\n\r\n- the various name resolution algorithms are concentrated in a new\r\n  file `sql/sem/tree/name_resolution.go`. They use abstract\r\n  interfaces to interface with the \"name provider\": database, table,\r\n  column things coming from the database.\r\n\r\n- in the process of writing the name resolution algorithms, I found\r\n  that our previous understanding of name resolution was problematic:\r\n\r\n  - the previous code assumed it was always possible to \"qualify a\r\n    name using the current database\" by just looking at the name\r\n    itself and the current database, without any additional\r\n    information.\r\n\r\n    In contrast, the correct qualification algorithms requires both\r\n    the current database, the search path, and descriptor\r\n    lookups.\r\n\r\n    This is why this patch fuses all occurrences of separate\r\n    \"qualification\" and \"resolution\" phases together.\r\n\r\n    Before: `tn = tn.QualifyWithDatabase(tcurDb); desc = MustGetDesc(tn)`\r\n\r\n    After: `desc = ResolveExistingObject(p, tn)`\r\n\r\n  - the resolution code was pushing a `VirtualTabler` very deep in the\r\n    lookup routines, with the mistaken understanding that\r\n    VirtualTabler is able to respond to requests for database names.\r\n\r\n    In contrast, VirtualTabler really has nothing to do with database\r\n    names, and the corresponding code had to get rid of it.\r\n\r\n    This was the first motivation for implementing new schema accessor\r\n    interfaces.\r\n\r\n  - the path to decide whether to use cached or non-cached descriptors\r\n    was very hard to read; in many instances the code was using\r\n    uncached descriptors where cached descriptors would be needed\r\n    instead. The various APIs were a mess, and information needed to\r\n    decide whether a lookup was possible or not was not available at\r\n    the right level(s) of abstraction.\r\n\r\n    This was the second motivation for implementing new schema accessor\r\n    interfaces.\r\n\r\n- Once this all was said done, the various consumers of name\r\n  resolution had to implement the interfaces. They are:\r\n\r\n  - resolution of zone specifiers;\r\n  - resolution of target names for CCL statements;\r\n  - resolution of names (both existing and targets) in the `sql` package;\r\n  - some testing code in `sql/opt` and `sql/opt/build`.\r\n\r\n### Details and suggestions to the reviewers\r\n\r\nThe following items are fairly self-contained and can be reviewed in\r\nparallel, and probably should be reviewed first:\r\n\r\n1. the changes in `ccl/sqlccl`. This achieves the following two things:\r\n   - the search path is plumbed where needed.\r\n   - the check for valid table names does not use\r\n     `QualifyWithDatabase` any more (because that was removed);\r\n     instead they use the new name resolution interface with a shim\r\n     that just checks the schema name is valid.\r\n\r\n2. the changes in `sql/opt` and `sql/opt/build`. This merely\r\n   interfaces with the name resolution algorithm for column items.  I\r\n   noticed that the underlying data structures there only know about\r\n   unqualified table names, so I did not attempt to plumb\r\n   catalog/schema name resolution logic in there. No functionality is\r\n   lost.\r\n\r\n3. the changes in `pkg/config` for zone specifiers. These are\r\n   straightforward and merely deal with the correspondance between the\r\n   schema part in the CLI zone specifier syntax and the catalog part of\r\n   the internal `ZoneSpecifier` struct.\r\n\r\n4. the new file `resolver.go`, which contains the entry-point\r\n   functions for name resolution using a `SchemaResolver`.\r\n\r\n5. the changes to `server/admin.go`. These simply get rid of the\r\n   virtual database checks (we have virtual schemas, not databases,\r\n   now.)\r\n\r\n6. the name resolution algorithms in `sem/tree/name_resolution.go`\r\n   and the corresponding tests in `sem/tree/name_resolution_test.go`.\r\n\r\n   This is where it all started and should be compared to the\r\n   specifications in the accompanying RFC (#21456).\r\n\r\n7. the interface definitions in `schema_accessors.go` and their\r\n   implementations in `phy_schema_accessors.go` and\r\n   `logical_schema_accessors.go`.\r\n\r\nOnce the last two items in the list above have been reviewed,\r\nit becomes possible to proceed as follows:\r\n\r\n8. check that the various planNode constructors access the new APIs\r\n   consistently, with no surprises.\r\n\r\n9. check that no harm was done in `table.go` (table cache) and\r\n   `database.go` (database cache).\r\n\r\n10. check that the construction of the main accessors in\r\n    `session.go` make sense.\r\n\r\nSuggested review attributions:\r\n\r\n- @vivekmenezes: items 7 and 9.\r\n- @petermattis: items 2 and 6.\r\n- @jordanlewis: items 4 and 7.\r\n- @benesch: items 1 and 3.\r\n- @m-schneider: item 8.\r\n- @justinj: item 6.\r\n- @rytaft, @andy-kimball: items 2 and 6.\r\n- @BramGruneir, @a-robinson: item 5.\r\n- @andreimatei: item 10.\r\n\r\nThe following items remain to be done before this PR can be reviewed fully:\r\n\r\n- equipping `planner` with the proper interface to become a\r\n  `ColumnItemResolver`.\r\n\r\n  - consequently, fix star expansion.\r\n\r\n- upgrading the virtual table definitions as per the RFC.\r\n\r\nRelease note (sql change): CockroachDB now recognizes the syntax\r\n`db.public.tbl` in addition to `db.tbl`, for better compatibility with\r\nPostgreSQL."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/364967532","html_url":"https://github.com/cockroachdb/cockroach/pull/22371#issuecomment-364967532","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22371","id":364967532,"user":{"login":"knz","id":642886,"avatar_url":"https://avatars3.githubusercontent.com/u/642886?v=4","gravatar_id":"","url":"https://api.github.com/users/knz","html_url":"https://github.com/knz","followers_url":"https://api.github.com/users/knz/followers","following_url":"https://api.github.com/users/knz/following{/other_user}","gists_url":"https://api.github.com/users/knz/gists{/gist_id}","starred_url":"https://api.github.com/users/knz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/knz/subscriptions","organizations_url":"https://api.github.com/users/knz/orgs","repos_url":"https://api.github.com/users/knz/repos","events_url":"https://api.github.com/users/knz/events{/privacy}","received_events_url":"https://api.github.com/users/knz/received_events","type":"User","site_admin":false},"created_at":"2018-02-12T15:58:50Z","updated_at":"2018-02-12T15:58:50Z","author_association":"OWNER","body":"vivek: well understood\n\n---\n\nReview status: 2 of 116 files reviewed at latest revision, 20 unresolved discussions, some commit checks pending.\n\n---\n\n*[pkg/ccl/sqlccl/targets_test.go, line 89 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-L4kjmMx4KRDIevxWU1u:-L559ySVGifnBFNETBD1:b-84xon2) ([raw file](https://github.com/cockroachdb/cockroach/blob/3e201f736af3630e6cf24aa1564e49afb70a42a9/pkg/ccl/sqlccl/targets_test.go#L89)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary><blockquote>\n\nIs there a reason that you changed `SyStEm` to `system`?\n</blockquote></details>\n\noversight; reverted.\n\n---\n\n*[pkg/sql/create_index.go, line 42 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-L4aPBWSN2D0pcdjQIiV:-L59imWmChA8N05GXBFa:b-3ejdcu) ([raw file](https://github.com/cockroachdb/cockroach/blob/aaaba1e8a8f531c55c965da70ff06420381077a6/pkg/sql/create_index.go#L42)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary><blockquote>\n\nGot it. It may be less error prone to include this as an argument to `ResolveExistingObject`. That way the caller can't forget to think about whether to allow the use of non-committed descriptors.\n\nPS I think you might have left a file out of this PR. I can't find `MustGetTableDesc` or `MustGetTableOrViewDesc`.\n</blockquote></details>\n\nyes correct.\n\n---\n\n*[pkg/sql/data_source.go, line 384 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-L4aPc0gNh91Uk8rfGE_:-L55OpYsCZxk41uS43oY:bss4a2b) ([raw file](https://github.com/cockroachdb/cockroach/blob/aaaba1e8a8f531c55c965da70ff06420381077a6/pkg/sql/data_source.go#L384)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary><blockquote>\n\nPublish before push? (The `log.VEventf` calls are still there).\n</blockquote></details>\n\nRemoved.\n\n---\n\n*[pkg/sql/database.go, line 107 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-L4aQFY9VnlY7CtMl8bG:-L55OzLA1zk9FHy1bT9f:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/aaaba1e8a8f531c55c965da70ff06420381077a6/pkg/sql/database.go#L107)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary><blockquote>\n\nLooks like we can drop this interface completely. It doesn't appear to be used anymore.\n</blockquote></details>\n\nDone.\n\n---\n\n*[pkg/sql/sem/tree/name_resolution.go, line 124 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-L4beEAuX3RbgBD_j0eB:-L55P5YHkmscb_zs2UGa:bgv4lm1) ([raw file](https://github.com/cockroachdb/cockroach/blob/aaaba1e8a8f531c55c965da70ff06420381077a6/pkg/sql/sem/tree/name_resolution.go#L124)):*\n<details><summary><i>Previously, rytaft wrote…</i></summary><blockquote>\n\nPerhaps something like:\n\n```\nconst (\n  columnPart = iota\n  tablePart\n  schemaPart\n  catalogPart\n  \n  numParts\n)\n```\n\nI see that in other places in the code these positions may be shifted. But is there a good reason for that? NameParts always has 4 parts. Why not make it clear which part is in which position?\n</blockquote></details>\n\nI think there was a misunderstanding here. The 1-4 parts in the syntax of unresolved names cannot be attributed fixed meaning before name resolution starts.\nIn particular:\n- In a scalar context the first part is the column name\n- in a relational context the first part is the table name\n\nAlso we can't make two separate types because there's at least one place where we need a single parser rule (and thus 1 type) to represent *either* something that looks like a table name (specifically: a function name) or something that looks like a column name.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/22371#-:-L59j81HrI7v2d_XBx6S:bcx73cj)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"knz"}}