sql: default_int_size does not apply to integer literals

[rafiss]

Describe the problem

With the following:

root@:26257/defaultdb> set default_int_size = 4;
SET

root@:26257/defaultdb> select 1;
  ?column?
------------
         1

one would expect that the integer sent back over the wire would be an INT4. However, if you use Wireshark (or any client that cares about integer types) you'll see that it's actually an INT8.

To Reproduce

The following pgwire test:

send crdb_only
Query {"String": "SET default_int_size = 4"}
----

until crdb_only
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"SET"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Query {"String": "SELECT 1"}
----

until
ReadyForQuery
----
{"Type":"RowDescription","Fields":[{"Name":"?column?","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":23,"DataTypeSize":4,"TypeModifier":-1,"Format":0}]}
{"Type":"DataRow","Values":[{"text":"1"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

run with make test PKG=./pkg/sql/pgwire TESTS='TestPGTest'

Expected behavior
The test should pass, but instead it returns a DataTypeOID of 20 (int8)

Additional context

• This affects jasync-sql tests.
• default_int_size is pretty silly anyway. consider this test case/comment:

# The setting doesn't affect explicit casts, only table definitions.
# (So in CockroachdB ::int is still ::int8, whereas it's ::int4 in postgres.)
send crdb_only
Query {"String": "SELECT 1::int, 2::int4, 3::int8"}
----

until crdb_only
ReadyForQuery
----
{"Type":"RowDescription","Fields":[{"Name":"int8","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0},{"Name":"int4","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":23,"DataTypeSize":4,"TypeModifier":-1,"Format":0},{"Name":"int8","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":20,"DataTypeSize":8,"TypeModifier":-1,"Format":0}]}
{"Type":"DataRow","Values":[{"text":"1"},{"text":"2"},{"text":"3"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

Jira issue: CRDB-10964

[otan]

should we just default int size to 4 anyway?

[rafiss]

Apparently that was the plan at one point in history:

#26925 (comment)

[2.2] Test: Ensure that cast operations '1'::INT result in the correct data width.

[otan]

spicy

[otan]

default_int_size is pretty silly anyway. consider this test case/comment:

this is more because of the family stuff (resolution logic relies on families rather than oids). casting anything atm assumes the same family and will default to the "family main type":

[email protected]:56449/defaultdb> select pg_typeof(1::int2);
  pg_typeof
-------------
  bigint
(1 row)


Time: 1ms total (execution 0ms / network 0ms)

[rafiss]

I think that problem is specific to how we've implemented pg_typeof.

In other words, if you do a datadriven pgwire test like

send
Query {"String": "SELECT 1::int2"}
----

(note the cast), then it does correctly show a DataTypeOID of 21.

But the DataTypeOID is not affected by just the default_int_size setting alone.

---

For some context, I filed this issue because the jasync-sql test suite does queries like SELECT 1 all over the place to check that the database is up, but it's very picky and requires that the result is a 32-bit java integer. Setting default_int_size=4 does not fix these tests because of this github issue.

[rafiss]

Ah I see what you're probably getting at. The type is determined in the optimizer here:

cockroach/pkg/sql/opt/optbuilder/scope.go

Lines 266 to 272 in a159b0b

	func (s *scope) addColumn(name scopeColumnName, expr tree.TypedExpr) *scopeColumn {
	s.cols = append(s.cols, scopeColumn{
	name: name,
	typ: expr.ResolvedType(),
	expr: expr,
	})
	return &s.cols[len(s.cols)-1]

Since expr is a DInt the resolved type is always a types.Int.

I wonder if we could enhance this part of the type-checking to be aware of default_int_size...

cockroach/pkg/sql/opt/optbuilder/scope.go

Lines 436 to 443 in a159b0b

	func (s *scope) resolveType(expr tree.Expr, desired *types.T) tree.TypedExpr {
	expr = s.walkExprTree(expr)
	texpr, err := tree.TypeCheck(s.builder.ctx, expr, s.builder.semaCtx, desired)
	if err != nil {
	panic(err)
	}
	return s.ensureNullType(texpr, desired)
	}

An even lower-level would be to make typeCheckConstant be aware of default_int_size but that seems too invasive.

[ajwerner]

An even lower-level would be to make typeCheckConstant be aware of default_int_size but that seems too invasive.

I don't think that seems all that invasive.

[giangpham712]

This affects efcore.pf tests
@fqazi