release-20.1: sql: propagate opt types through renders

[rafiss]

Backport 1/1 commits from #48619.

/cc @cockroachdb/release

---

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.

[cockroach-teamcity]

This change is 

[RaduBerinde]

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

[rafiss]

hmmm i think the test this was supposed to fix is not working:

Query {"String": "SELECT 'a'::\"char\" FROM a"}
----
            expected:
            {"Type":"RowDescription","Fields":[{"Name":"char","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":18,"DataTypeSize":1,"TypeModifier":-1,"Format":0}]}

            found:
            {"Type":"RowDescription","Fields":[{"Name":"char","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":25,"DataTypeSize":1,"TypeModifier":-1,"Format":0}]}

[rafiss]

@RaduBerinde & @jordanlewis can you think of anything that might be missing here?

[jordanlewis]

You would also need to backport @RaduBerinde's patch #48652

[rafiss]

That one has been backported here: #49331

[RaduBerinde]

I don't get it either.. Without this PR we have

[email protected]:40541/defaultdb> explain (types) select a,b, ''::"char" from ab;
    tree    |    field    | description  |             columns              | ordering
------------+-------------+--------------+----------------------------------+-----------
            | distributed | true         |                                  |
            | vectorized  | false        |                                  |
  render    |             |              | (a int, b string, "char" string) |
   │        | render 0    | (a)[int]     |                                  |
   │        | render 1    | (b)[string]  |                                  |
   │        | render 2    | ('')[string] |                                  |
   └── scan |             |              | (a int, b string)                |
            | table       | ab@primary   |                                  |
            | spans       | FULL SCAN    |                                  |

And with this PR we have

[email protected]:39549/defaultdb> explain (types) select a,b, ''::"char" from ab;
    tree    |    field    | description  |             columns              | ordering
------------+-------------+--------------+----------------------------------+-----------
            | distributed | true         |                                  |
            | vectorized  | false        |                                  |
  render    |             |              | (a int, b string, "char" "char") |
   │        | render 0    | (a)[int]     |                                  |
   │        | render 1    | (b)[string]  |                                  |
   │        | render 2    | ('')[string] |                                  |
   └── scan |             |              | (a int, b string)                |
            | table       | ab@primary   |                                  |
            | spans       | FULL SCAN    |                                  |

So it looks like the top planNode has the correct type..

[RaduBerinde]

Ah, lol, I think it's this map:

cockroach/pkg/sql/pgwire/types.go

Line 67 in c83f61f

var resultOidMap = map[oid.Oid]oid.Oid{

[RaduBerinde]

I think we need this: #44471

[RaduBerinde]

I fell less and less comfortable making these changes in 20.1.x..

[rafiss]

Ah right I remember seeing that before... Thanks for the find. I'm fine abandoning this backport, if that's the vibe people are having.

[andy-kimball]

Agreed that we shouldn't make a backwards-compat breaking change in a point release, unless we had an extraordinary reason to do so, agreement from all stakeholders, thorough documentation and education, etc.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis)

[rafiss]

we shouldn't make a backwards-compat breaking change in a point release...

Agreed. This was initially intended to be a small bugfix (filling in missing data), but now it's become something more than that.

Closing this.