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

sql/pgwire: "char" datatype is not used correctly if selecting both a constant and from a table #48563

Closed
rafiss opened this issue May 8, 2020 · 0 comments · Fixed by #48619
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@rafiss
Copy link
Collaborator

rafiss commented May 8, 2020

This issue breaks class generation functionality in the .NET EntityFramework Core ORM.

Consider this query: select 'c'::"char";

Using Wireshark, I found that in CockroachDB, the result comes back with a type OID of 18 -- the OID for the "char" datatype. This is correct.

However, consider the following:

root@:26257/defaultdb> create table  tab (a int primary key, b "char");
CREATE TABLE

root@:26257/defaultdb> select a,b, ''::"char" from tab;
  a | b | char
----+---+-------
(0 rows)

Now the ''::"char" value comes back with a type OID of 25 -- the OID for the text datatype. This is incorrect. It should still have a type OID of 18. The b column has the correct type OID of 18.

Also, in CockroachDB, the wrong typlen is returned for the "char" type. It should be 1, but it is -1 right now. Moved this to a separate issue: #48565

As a possible aside: it looks like the pg_catalog tables all use CHAR types when instead they should be using "char".

@rafiss rafiss added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. labels May 8, 2020
@rafiss rafiss changed the title sql/pgwire: "char" datatype should not be the same as the text datatype sql/pgwire: "char" datatype is not used correctly May 8, 2020
@rafiss rafiss changed the title sql/pgwire: "char" datatype is not used correctly sql/pgwire: "char" datatype is not used correctly if selecting both a constant and from a table May 8, 2020
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue May 9, 2020
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue May 10, 2020
The `Const` operator infers its type using `Datum.ResolvedType()`. The Datum's
type loses information in some cases (e.g. see cockroachdb#48563). We now pass the type
explicitly to `ConstructConst`, similar to the `Null` operator.

Since Const already had a private member, optgen needed to be modified to
tolerate a Typ field in this case (only allowed for Scalars). Note that this
field was already special - for any Scalar that doesn't have a `Typ` field, one
is added automatically and `InferType` is used to populate it.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue May 10, 2020
The `Const` operator infers its type using `Datum.ResolvedType()`. The Datum's
type loses information in some cases (e.g. see cockroachdb#48563). We now pass the type
explicitly to `ConstructConst`, similar to the `Null` operator.

Since Const already had a private member, optgen needed to be modified to
tolerate a Typ field in this case (only allowed for Scalars). Note that this
field was already special - for any Scalar that doesn't have a `Typ` field, one
is added automatically and `InferType` is used to populate it.

Release note: None
craig bot pushed a commit that referenced this issue May 13, 2020
48529: builtins: add Geography builtins r=sumeerbhola a=otan

Fruits of labour be released to the public!

Added some more helpers for InfoBuilder to include the source library in
which our calculations are done.

Resolves #48367
Resolves #48393
Resolves #48394
Resolves #48395
Resolves #48396
Resolves #48399
Resolves #48400
Resolves #48401

Release note (sql change): We introduce the following newly implemented
functions that work on GEOGRAPHY types:
* ST_Covers
* ST_CoveredBy
* ST_Intersects
* ST_Distance
* ST_DWithin
* ST_Perimeter
* ST_Area
* ST_Length



48651: opt: show diff in optfmt linter r=RaduBerinde a=RaduBerinde

The optfmt linter only shows the problematic files. This can be inconvenient for
anyone who doesn't regularly touch .opt files and has optfmt set up in their
editor (or has no idea what it even is). This change improves `optfmt -l` to
include a diff which can be applied to fix the files. Sample output:

```
--- FAIL: TestLint (0.00s)
    --- FAIL: TestLint/TestOptfmt (0.20s)
        lint_test.go:340:
            --- /go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/norm/rules/agg.opt
            +++ /go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/norm/rules/agg.opt
            @@ -5,9 +5,8 @@

             # EliminateAggDistinct removes AggDistinct for aggregations where DISTINCT
             # never modifies the result; for example: min(DISTINCT x).
             [EliminateAggDistinct, Normalize]
            -(AggDistinct
            -$input:(Min | Max | BoolAnd | BoolOr))
            +(AggDistinct $input:(Min | Max | BoolAnd | BoolOr))
             =>
             $input

            --- /go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/norm/rules/bool.opt
            +++ /go/src/github.com/cockroachdb/cockroach/pkg/sql/opt/norm/rules/bool.opt
            @@ -152,9 +152,10 @@
             # This transformation is useful for finding a conjunct that can be pushed down
             # in the query tree. For example, if the redundant conjunct A is fully bound by
             # one side of a join, it can be pushed through the join, even if B AND C cannot.
             [ExtractRedundantConjunct, Normalize]
            -(Or $left:^(Or)
            +(Or
            +    $left:^(Or)
                 $right:^(Or) &
                     (Succeeded
                         $conjunct:(FindRedundantConjunct $left $right)
                     )
FAIL
FAIL	github.com/cockroachdb/cockroach/pkg/testutils/lint	0.227s
```

Release note: None

48652: opt: pass Const type explicitly r=RaduBerinde a=RaduBerinde

The `Const` operator infers its type using `Datum.ResolvedType()`. The Datum's
type loses information in some cases (e.g. see #48563). We now pass the type
explicitly to `ConstructConst`, similar to the `Null` operator.

Since Const already had a private member, optgen needed to be modified to
tolerate a Typ field in this case (only allowed for Scalars). Note that this
field was already special - for any Scalar that doesn't have a `Typ` field, one
is added automatically and `InferType` is used to populate it.

Release note: None

48758: settings: fix RegisterPublicNonNegativeDurationSettingWithMaximum r=rohany a=otan

`RegisterPublicNonNegativeDurationSettingWithMaximum` should include the
maximum. Seems broken by `ac3c72339b82d9f85fd8112977a542b87fb5e712`.

Resolves #48337.

Release note (bug fix): Re-allow
`diagnostics.forced_sql_stat_reset.interval`,
`diagnostics.sql_stat_reset.interval` and `external.graphite.interval`
to set to their maximum values (24hr, 24hr and 15min respectively). This
previously only excluded these values to be allowed.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue May 15, 2020
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue May 16, 2020
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
jordanlewis added a commit to jordanlewis/cockroach that referenced this issue May 17, 2020
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
craig bot pushed a commit that referenced this issue May 17, 2020
48619: sql: propagate opt types through renders r=jordanlewis a=jordanlewis

Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes #48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig craig bot closed this as completed in bf975d4 May 17, 2020
rafiss pushed a commit to rafiss/cockroach that referenced this issue May 20, 2020
The `Const` operator infers its type using `Datum.ResolvedType()`. The Datum's
type loses information in some cases (e.g. see cockroachdb#48563). We now pass the type
explicitly to `ConstructConst`, similar to the `Null` operator.

Since Const already had a private member, optgen needed to be modified to
tolerate a Typ field in this case (only allowed for Scalars). Note that this
field was already special - for any Scalar that doesn't have a `Typ` field, one
is added automatically and `InferType` is used to populate it.

Release note: None
rafiss pushed a commit to rafiss/cockroach that referenced this issue May 21, 2020
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-pgwire pgwire protocol issues. 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.

1 participant