Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2.0-beta.20180319: Regression in star queries #24169

Closed
charsleysa opened this issue Mar 23, 2018 · 5 comments · Fixed by #24811
Closed

v2.0-beta.20180319: Regression in star queries #24169

charsleysa opened this issue Mar 23, 2018 · 5 comments · Fixed by #24811
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@charsleysa
Copy link

BUG REPORT

  • What did you do?
    Executed the following query
select
    "myblueprint"."study_industry".*
from "myblueprint"."study_industry"
  • What did you expect to see?
    All rows with all columns from the table.

  • What did you see instead?
    The following error [42703] ERROR: no data source named "myblueprint.study_industry"

This worked fine in v1.1.6 but is now broken in v2.0-beta.20180319

@jordanlewis
Copy link
Member

Hi @charsleysa,

For better Postgres compatibility, we've changed CockroachDB's name resolution to understand the concept of database schemas. This means that there's now another level in the name hierarchy, between database and table: the schema.

The default schema in CockroachDB (and the only one with user data, since we don't yet support custom schemas) is called public, like it is in Postgres. We insert public between the database and table in data sources, if one isn't specified, but it seems we don't do the same for render expressions.

To work around this, you may insert public between the database name and table name in your render expression:

select "myblueprint"."public"."study_industry".* from ...

We didn't quite realize that anyone was using fully qualified names in render expressions, otherwise we would have caught this. Why do you need to qualify the table by the database name, though? If you're connected to the database myblueprint, you do not need to qualify your table names with the database name.

cc @knz

@charsleysa
Copy link
Author

charsleysa commented Mar 23, 2018

Hi @jordanlewis,

Thanks for getting back quickly.

I am aware some of those changes were made but I was using the "myblueprint" part as the schema qualifier as a SQL generation library I was using kept placing "public" into queries.

I think I may have missed some information when initially creating the issue, the following query works in v1.1.6 AND the v2.0-beta:

select
    "myblueprint"."study_industry"."id",
    "myblueprint"."study_industry"."title"
from "myblueprint"."study_industry"

@jordanlewis
Copy link
Member

Oh, I see. That seems like a bug. Here's a simple reproduction:

root@:26257/> use test;
SET

Time: 313.263µs

root@:26257/test> create table a (a int primary key);
CREATE TABLE

Time: 5.299591ms

root@:26257/test> select test.a.a from a;
+---+
| a |
+---+
+---+
(0 rows)

Time: 4.133979ms

root@:26257/test> select test.a.* from a;
pq: no data source named "test.a"

@knz knz self-assigned this Mar 23, 2018
@knz
Copy link
Contributor

knz commented Mar 23, 2018

I'll look into it when I am back.

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Mar 23, 2018
@knz knz added this to the 2.0 milestone Mar 23, 2018
@knz
Copy link
Contributor

knz commented Mar 27, 2018

(technical information ahead - not relevant to understand the issue, but necessary to fix):

The proximate cause is that FindColumnInSource / FindSourceProvidingColumn assume that the given prefix (if any) must match one of the names provided by the source(s). Meanwhile, the source name undergoes qualification before its name is made available to column name resolution, so from the perspective of ColumnResolver there are only fully qualified sources with "public" in them.

To address this properly requires adding to FindColumnInSource / FindSourceProvidingColumn the same crdb 1.x compatibility fallback that is also present in table name resolution. However, achieving this change probably requires extra care (need to investigate what are the subtleties associated with dbs called "public") and what happens when both "db.public.blah" and "public.public.blah" are in scope for a given column resolution.

cc @rytaft - Becca, maybe this doesn't change anything for you, but you'll want to watch the story as it unfolds. The ambiguity problem will need attention + docs when we extend the fix to this issue to work with correlated naming resolution.

@petermattis petermattis modified the milestones: 2.0, 2.0.x Apr 5, 2018
craig bot pushed a commit that referenced this issue Apr 16, 2018
24811: sql: use a reusable name resolution algo for star expansions r=knz a=knz

Fixes #24169.

A previous patch has added a reusable name resolution mechanism which
encapsulates all the special cases supported CockroachDB into a single
code path. This covers both the resolution of object names and column
names.

Prior to this patch however, the expansion of qualified stars was
using a separate code path. This proved to be incorrect/insufficient
however, because one of the special cases of name resolution should
apply to star expansion too and the separate code path did not contain
the special case.

To solve this problem, this patch abstracts the resolution of
qualified stars in a common algorithm alongside the others (in
`sem/name_resolution.go`) and ensures it is used where applicable.

Release note (bug fix): CockroachDB now again allows to use a simply
qualified table name in qualified stars (e.g., `SELECT mydb.kv.* FROM
kv`), for compatibility with CockroachDB v1.x.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig craig bot closed this as completed in #24811 Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants