sql: allow imprecise pgwire type hints

[jordanlewis]

This commit permits the use of imprecise pgwire or sql type hints by
recording any mismatch between the client-provided type hint and the
type-system inferred type, and inserting a runtime cast from the
placeholder's original type to its inferred type.

Before this patch, type hints that were sent via pgwire or SQL (e.g. in
`PREPARE x(int) as ...` the `int` is the type hint) were taken perfectly
literally. Placeholder type hints were required to match the type
system's inferred type for that placeholder. This simple behavior
unfortunately leaves us with compatibility issues with some drivers,
such as JDBC, which send "imprecise type hints" - type hints that don't
exactly match the correct type for the placeholder.

The main motivating example is the following query:

```
PREPARE x(INT) AS SELECT * FROM pg_type WHERE oid=$1
```

In this query, the type hint is `INT` but the inferred type is `OID`,
since we don't support heterogeneous comparisons between integer and
oid. Previously, this would not type check - but JDBC expects it to.

After this patch, if the type hint does not match the inferred type, we
insert a runtime cast from the original type to the inferred type. This
produces the query `SELECT * FROM pg_type WHERE oid=$1::oid` for our
example.

We accomplish this by separately storing the type hints from the
inferred types. Now, type hints inform the typing of the placeholder
during prepare, if present. As soon as a placeholder is typed, the final
placeholder type is set and cannot be changed. If the type hint was
missing for that placeholder, it is set to the final placeholder type -
this is required by pgwire so that the values passed to execute can be
parsed properly.

Previously #14387.
Resolves #14311.
Resolves #14245.
Resolves #14554.
Resolves #17140.

[cockroach-teamcity]

This change is 

[jordanlewis]

Completely reworked. PTAL.

[justinj]

🎉🎉🎉

---

Review status: 0 of 7 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

pkg/acceptance/java_test.go, line 206 at r3 (raw file):

		if (!"pg_catalog".equals(nspName)) {
		    throw new Exception("unexpected: SELECT can't find pg_catalog database: found " + nspName);
		}

Could you also try selecting ARRAY[1,2,3] or something similar? I was running into problems with that before.

---

pkg/sql/prepare.go, line 45 at r3 (raw file):

	// dictates how input parameters for those placeholders will be parsed.
	SQLTypes       parser.PlaceholderTypes
	// OverridenTypes contains the inferred types of the placeholders after

Overridden

---

pkg/sql/parser/placeholders.go, line 35 at r3 (raw file):

	Values QueryArguments
	Types  PlaceholderTypes
	// OverriddenTypes relates placeholder names to their overriden type, if any.

overridden

---

pkg/sql/parser/placeholders.go, line 139 at r3 (raw file):

// case of a placeholder whose type hint does not match the desired type during
// typechecking. It will return an error if the placeholder never received a
// type or if the placeholder's type was already overriden.

overridden

---

pkg/sql/parser/placeholders.go, line 142 at r3 (raw file):

func (p *PlaceholderInfo) OverrideType(name string, typ Type) error {
	if _, ok := p.Types[name]; !ok {
		return fmt.Errorf("cannot override untyped placeholder %s", name)

Can you use a pgerror here?

---

pkg/sql/parser/placeholders.go, line 147 at r3 (raw file):

		p.OverriddenTypes = make(map[string]Type, 1)
	} else if _, ok := p.OverriddenTypes[name]; ok {
		return fmt.Errorf("placeholder %s already has type %s, cannot assign %s", name, p.OverriddenTypes[name], typ)

also here

---

pkg/sql/parser/placeholders.go, line 224 at r3 (raw file):

// this a no-op and is supported for tests only.
func replacePlaceholders(expr Expr, ctx *SemaContext) Expr {
	if ctx == nil || len(ctx.Placeholders.Values) == 0 {

just to confirm my understanding - this change is purely an optimization, correct?

---

pkg/sql/parser/type_check.go, line 812 at r3 (raw file):

			//
			// Note that we can only override a type once - otherwise we'll permit
			// non-well-typed queries like `SELECT NOT $1, $1 > 10` which tries to

do we have a test of this case?

---

Comments from Reviewable

[jordanlewis]

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

pkg/sql/prepare.go, line 45 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Overridden

Done.

---

pkg/sql/parser/placeholders.go, line 35 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

overridden

Done.

---

pkg/sql/parser/placeholders.go, line 139 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

overridden

Done.

---

pkg/sql/parser/placeholders.go, line 142 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Can you use a pgerror here?

Done.

---

pkg/sql/parser/placeholders.go, line 147 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

also here

Done.

---

pkg/sql/parser/placeholders.go, line 224 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

just to confirm my understanding - this change is purely an optimization, correct?

Yes.

---

pkg/sql/parser/type_check.go, line 812 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

do we have a test of this case?

Yes, in TestPGPrepareFail. It actually failed in CI because I didn't account for the case where no pgwire type hint is set.

While fixing this, I realized that there's a better way to express the "overriding" that's happening in this PR. Instead of having types and overridden types, we now have type hints and types in both the placeholder info struct and the prepared statement struct.

Now it's simple to understand what's going on. Type hints inform the first typing of a placeholder. If there's no type hint for a placeholder, it's set by the first typing of the placeholder. As soon as a placeholder is actually typed, its placeholder type is set. That can only happen once. The end.

---

Comments from Reviewable

[justinj]

LGTM modulo question!

---

Review status: 0 of 9 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

		return t, true
	} else if t, ok := p.TypeHints[name]; ok {
		return t, true

so I don't 100% know what contexts this function will be used in, but is it conceivable that once we've reported a type in this way, we have to "lock in" to it so that if someone attempts to type it differently later on we won't return a different value?

---

pkg/sql/parser/type_check.go, line 812 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Yes, in TestPGPrepareFail. It actually failed in CI because I didn't account for the case where no pgwire type hint is set.

While fixing this, I realized that there's a better way to express the "overriding" that's happening in this PR. Instead of having types and overridden types, we now have type hints and types in both the placeholder info struct and the prepared statement struct.

Now it's simple to understand what's going on. Type hints inform the first typing of a placeholder. If there's no type hint for a placeholder, it's set by the first typing of the placeholder. As soon as a placeholder is actually typed, its placeholder type is set. That can only happen once. The end.

Yes this is much more understandable! 👍

---

Comments from Reviewable

[jordanlewis]

Review status: 0 of 9 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

pkg/acceptance/java_test.go, line 206 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Could you also try selecting ARRAY[1,2,3] or something similar? I was running into problems with that before.

I'm not sure what you mean here can you give a concrete example?

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, justinj (Justin Jaffray) wrote…

so I don't 100% know what contexts this function will be used in, but is it conceivable that once we've reported a type in this way, we have to "lock in" to it so that if someone attempts to type it differently later on we won't return a different value?

We do have to do that, but unfortunately it needs to be up to the caller, since at the time that we get a type hint we might choose to lock in to a different type. This is done in Placeholder.TypeCheck.

---

Comments from Reviewable

[justinj]

Review status: 0 of 9 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

pkg/acceptance/java_test.go, line 206 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I'm not sure what you mean here can you give a concrete example?

I just mean try running a query like SELECT ARRAY[1,2,3] and reading it, when I tried this before I would get a type error from some system query jdbc would issue when running it, for some reason.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

We do have to do that, but unfortunately it needs to be up to the caller, since at the time that we get a type hint we might choose to lock in to a different type. This is done in Placeholder.TypeCheck.

I see, ok!

---

Comments from Reviewable

[knz]

I think this is OK, I have a few minor comments below.
However before this merges I would like you to extend the commit message and make a longer clarifying story about what is going on here, as an overview. The story should contain:

• what was happening before this patch
• what is happening now
• how you made it happen

For the two first points use an example query and show separately for prepare/execute what happens with their placeholders.

---

Reviewed 7 of 7 files at r4, 2 of 2 files at r5, 9 of 9 files at r6.
Review status: all files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

---

pkg/sql/logictest/testdata/logic_test/prepare, line 251 at r6 (raw file):

# Mismatch between expected and hinted types should prepare, but potentially
# fail to execute if the cast is not possible.
statement

nit: say "statement ok" here and below

---

pkg/sql/parser/placeholders.go, line 224 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Yes.

This deserves an additional comment: your optimization and, "in the case placeholders are present in the SQL but none are provided during execute, the further type analysis will detect this and report an unassigned placeholder error". (This case is checked in a test, right?)

---

pkg/sql/parser/placeholders.go, line 34 at r6 (raw file):

// PlaceholderInfo defines the interface to SQL placeholders.
type PlaceholderInfo struct {
	Values    QueryArguments

Please document the difference between the two fields here.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I see, ok!

It seems to me that (*PlaceholderInfo).Type() can never be called until Placeholder.TypeCheck has been closed. So it's not so clear to me why this case needs to be handled here. For the use of the hint made in Placeholder.TypeCheck, we could have a separate method (PlaceholderInfo.TypeHint?)

---

pkg/sql/parser/placeholders.go, line 185 at r6 (raw file):

			if !typed {
				// All placeholders should be typed at this point. If they're not, we
				// can just return their value, though.

Ugh, why not an error?

---

pkg/sql/parser/placeholders.go, line 189 at r6 (raw file):

			}
			if !e.ResolvedType().Equivalent(typ) {
				// This happens when we overrode the placeholder's type during type

override

---

pkg/sql/parser/placeholders.go, line 195 at r6 (raw file):

				// TODO(jordan): introduce a restriction on what casts are allowed here.
				colType, err := DatumTypeToColumnType(typ)
				if err != nil {

I don't think the error should be silently dropped.

---

pkg/sql/parser/type_check.go, line 814 at r6 (raw file):

			// the type system expects. Then, when the value is actually sent to us
			// later, we cast the input value (whose type is the expected type) to the
			// desired type here.

There word "here" in this sentence is unclear. Where is the cast happening?

---

Comments from Reviewable

[jordanlewis]

Review status: all files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

---

pkg/acceptance/java_test.go, line 206 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I just mean try running a query like SELECT ARRAY[1,2,3] and reading it, when I tried this before I would get a type error from some system query jdbc would issue when running it, for some reason.

Hmmm but that has nothing to do with this patch because there's no placeholders there. I don't expect it to change.

I did a test with a placeholder and it has a different problem that is not related to this patch:

//CREATE TABLE test.v (x int[])
stmt = conn.prepareStatement("INSERT INTO test.v VALUES(?)");
Array array = conn.createArrayOf("INT8", new Object[]{1,2,3});

This produces: Exception in thread "main" org.postgresql.util.PSQLException: Unable to find server array type for provided name INT8.

We should open an issue for this but it's not related to this patch.

---

pkg/sql/logictest/testdata/logic_test/prepare, line 251 at r6 (raw file):

Previously, knz (kena) wrote…

nit: say "statement ok" here and below

This whole file is missing ok. I will fix in a separate pr.

---

pkg/sql/parser/placeholders.go, line 224 at r3 (raw file):

Previously, knz (kena) wrote…

This deserves an additional comment: your optimization and, "in the case placeholders are present in the SQL but none are provided during execute, the further type analysis will detect this and report an unassigned placeholder error". (This case is checked in a test, right?)

Done. Detecting missing placeholders is done before this point for both pgwire execute and sql execute.

---

pkg/sql/parser/placeholders.go, line 34 at r6 (raw file):

Previously, knz (kena) wrote…

Please document the difference between the two fields here.

Done.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, knz (kena) wrote…

It seems to me that (*PlaceholderInfo).Type() can never be called until Placeholder.TypeCheck has been closed. So it's not so clear to me why this case needs to be handled here. For the use of the hint made in Placeholder.TypeCheck, we could have a separate method (PlaceholderInfo.TypeHint?)

It's called in two places, during type checking and then later. I think it's more confusing than helpful to segregate the methods, since during type checking we do have to examine both.

---

pkg/sql/parser/placeholders.go, line 185 at r6 (raw file):

Previously, knz (kena) wrote…

Ugh, why not an error?

Done.

---

pkg/sql/parser/placeholders.go, line 189 at r6 (raw file):

Previously, knz (kena) wrote…

override

No, I'm using the past tense here on purpose.

---

pkg/sql/parser/placeholders.go, line 195 at r6 (raw file):

Previously, knz (kena) wrote…

I don't think the error should be silently dropped.

You're right, Done.

---

Comments from Reviewable

[jordanlewis]

TFTRs!

[knz]

Great commit message 💯

There's still an error message to fix so you're up for another round of CI anyway, so let's finish the discussion on placeholderinfo.Type().

---

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

It's called in two places, during type checking and then later. I think it's more confusing than helpful to segregate the methods, since during type checking we do have to examine both.

But that's the thing right? After type checking, we should never be looking at the hints map. Right now if there's a bug and we end up not populating p.Types properly, this method will silently mask that and make PlaceholderInfo.Type respond something because there's still a hint laying around. That's why I would like to find a way to assert the hints are only reachable during type checking and not afterwards.

---

pkg/sql/parser/placeholders.go, line 193 at r7 (raw file):

			if !typed {
				// All placeholders should be typed at this point.
				v.err = pgerror.NewErrorf(pgerror.CodeInternalError, "Missing type for placeholder %s", t.Name)

lowercase

---

Comments from Reviewable

[jordanlewis]

Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

---

pkg/sql/parser/placeholders.go, line 98 at r6 (raw file):

Previously, knz (kena) wrote…

But that's the thing right? After type checking, we should never be looking at the hints map. Right now if there's a bug and we end up not populating p.Types properly, this method will silently mask that and make PlaceholderInfo.Type respond something because there's still a hint laying around. That's why I would like to find a way to assert the hints are only reachable during type checking and not afterwards.

Okay, I added a boolean flag to the function allowTypeHints.

---

pkg/sql/parser/placeholders.go, line 193 at r7 (raw file):

Previously, knz (kena) wrote…

lowercase

Done.

---

Comments from Reviewable

[knz]

LGTM -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[jordanlewis]

Woohoo! TFTRs! This one started in March - psyched to get it in.