Mistakenly assumes Long value as INT4 on update

[jorgeacetozi]

Describe the problem

After updating CockroachDB from version 20.2.10 to 21.1.5, we started getting this exception when issuing an update:

Unexpected Exception during request processing - return 500 Internal Server Error org.jdbi.v3.core.statement.UnableToExecuteStatementException: org.postgresql.util.PSQLException: ERROR: integer out of range for type int4

After some debugging, we were able to figured out that the issue happens when:

1. Insert a row with a null value for a column where the type is INT8 in the table, but set as a INT4 (JDBI defaults to INT4 when a null value is provided);
2. Issue an update to this row, also with a null value;
3. Issue another update to the same row, but now with a Long value;

To Reproduce

We set up a simple unit test that spins up a CockroachDB container (version 21.1.5) and uses Postgres JDBC Driver (version 42.2.23) to reproduce the steps described:

    @Test
    @DisplayName("cockroachdb should respect column type")
    public void testCockroachWrongType() throws IOException, InterruptedException, SQLException {
        Long expected = 1626256150696L;

        try(CockroachContainer cockroachContainer = new CockroachContainer(
                "dbName",
                "cockroachdb/cockroach:v21.1.5")){
            cockroachContainer.start();

            cockroachContainer.executeSql("CREATE TABLE dbName.someNumber (theNumber INT, someId INT NOT NULL);");

            Connection con = getConnection(cockroachContainer.getUsername(), cockroachContainer.getPassword(), cockroachContainer.getJdbcUrl());
            con.setAutoCommit(true);

            int integerType = Types.INTEGER; // some libraries defaults to int when value being passed is null, i.e.: jdbi

            try (PreparedStatement firstInsert = con.prepareStatement("INSERT INTO dbName.someNumber (theNumber, someId) VALUES(?, ?);")) {
                firstInsert.setNull(1, integerType);
                firstInsert.setLong(2, 1);
                firstInsert.execute();
            }

            try (PreparedStatement firstUpdate = con.prepareStatement("UPDATE dbName.someNumber SET theNumber = ? WHERE someId = ?;")) {
                firstUpdate.setNull(1, integerType);
                firstUpdate.setLong(2, 1);
                firstUpdate.execute();
            }

            try (PreparedStatement secondUpdate = con.prepareStatement("UPDATE dbName.someNumber SET theNumber = ? WHERE someId = ?;")) {
                secondUpdate.setLong(1, expected);
                secondUpdate.setLong(2, 1);
                secondUpdate.execute(); // org.postgresql.util.PSQLException: ERROR: integer out of range for type int4
            }

            Statement stmt = con.createStatement();
            ResultSet rs = stmt.executeQuery("SELECT theNumber FROM dbName.someNumber WHERE someId = 1;");
            rs.next();
            long actual = rs.getLong("theNumber");

            assertEquals(expected, actual);
        }
    }

If the CockroachDB container version in this test is set to 20.2.10, then the test work as expected and the issue does not happen.

Expected behavior
The expectation is that the last update works properly since the value being sent is a Long and the column type is a INT8.

Environment:

• CockroachDB version: 21.1.5
• Server OS: Linux 4.15.0-1021-aws
• Client app: JDBI -> Postgres JDBC

Additional context
Unexpected behavior in update queries.

[blathers-crl]

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I was unable to automatically find someone to ping.

If we have not gotten back to your issue within a few business days, you can try the following:

• Join our community slack channel and ask on #cockroachdb.
• Try find someone from here if you know they worked closely on the area and CC them.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[rafiss]

Thanks for this report @jorgeacetozi! The example you gave is really helpful.

It looks like this is related to the fact that in v21.1 we started using our new vectorized execution engine in more places. Specifically, in this case, the vectorized cast operator seems to have a bug.

This is confirmed by adding the following lines to your example before inserting data:

            Statement vecOff = con.createStatement();
            vecOff.execute("SET vectorize = off;");

I used a debugger and tracked down the source of the error to this line of code:

cockroach/pkg/sql/colexec/colexecbase/cast.eg.go

Line 3119 in 1fd13c9

colexecerror.ExpectedError(tree.ErrInt4OutOfRange)

We'll work more to see if it can be fixed.

cc @jordanlewis

[yuzefovich]

@rafiss on a quick glance I don't think that the vectorized cast is to blame - more likely the typing information obtained during the planning of the query says that the value must be INT4, and the vectorized engine simply follows that.

[rafiss]

@yuzefovich the one extra detail that is relevant here is that there is no error if you remove this part of the code:

            try (PreparedStatement firstUpdate = con.prepareStatement("UPDATE dbName.someNumber SET theNumber = ? WHERE someId = ?;")) {
                firstUpdate.setNull(1, integerType);
                firstUpdate.setLong(2, 1);
                firstUpdate.execute();
            }

In other words, remove line 3 of:

CREATE TABLE dbName.someNumber (theNumber INT, someId INT NOT NULL);
INSERT INTO dbName.someNumber (theNumber, someId) VALUES(NULL::INT4, 1);
UPDATE dbName.someNumber SET theNumber = NULL::INT4 WHERE someId = 1;
UPDATE dbName.someNumber SET theNumber = 1626256150696 WHERE someId = 1;
SELECT theNumber FROM dbName.someNumber WHERE someId = 1;

Do you know why any type-checking logic would depend on whether this UPDATE statement ran or not?

By the way, I confirmed that the non-vectorized engine does not attempt to perform this cast. So I don't see how this would be a planning issue.

Could you point me to the code where the vectorized engine does the type-checking you are referring to? Or could you point me to where the vectorized engine decides that it needs to include a castInt64Int32Op operation? I can debug that area, but just need to know where to set the breakpoints.

[rafiss]

OK, I was able to track it down a bit further and find the places to look.

In this part of the optimizer:

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

Line 226 in 31cdc0e

texpr := inScope.resolveType(expr, desiredType)

We have a desiredType of INT8, but expr is a tree.Placeholder with a type annotation of INT4. the type annotation is INT4 because JDBI passed in a placeholder hint for INT4. calling into resolveType goes to this type-checking code:

cockroach/pkg/sql/sem/tree/type_check.go

Lines 1533 to 1552 in 31cdc0e

	if !desired.Equivalent(typ) {
	// This indicates there's a conflict between what the type system thinks
	// the type for this position should be, and the actual type of the
	// placeholder. This actual placeholder type could be either a type hint
	// (from pgwire or from a SQL PREPARE), or the actual value type.
	//
	// To resolve this situation, we *override* the placeholder type with what
	// 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.
	typ = desired
	}
	// We call SetType regardless of the above condition to inform the
	// placeholder struct that this placeholder is locked to its type and cannot
	// be overridden again.
	if err := semaCtx.Placeholders.SetType(expr.Idx, typ); err != nil {
	return nil, err
	}
	expr.typ = typ
	return expr, nil

Again, in that code desired is INT8 but typ is INT4. therefore we don't enter the if !desired.Equivalent(typ) branch, and the output type is set to INT4. and INT4 becomes the required type for the vectorized engine.

I think the problem is that right after this, another UPDATE dbName.someNumber SET theNumber = ? WHERE someId = ?; prepared statement is used. Since this prepared statement was already parsed, CockroachDB does not re-plan or re-type-check anything, so it continues thinking that the type of the placeholder is INT4, and chokes on the large value.

There's a couple things I'd say:

• CockroachDB perhaps should re-prepare the statement if it sees that the type hints for the placeholders have changed. I haven't checked yet to see if that's feasible.
• Ideally JDBI shouldn't send a hint of INT4 when the type of the column is INT8. That seems like a bug to me. Consider filing an issue with the JDBI project.

I think the problem would have existed in 20.2 as well, but the issue just surfaced because 21.1 changed the vectorized engine to on by default.

[rafiss]

Could you share your JDBI code?

I see that JDBI does have an ArgumentFactory that should handle OptionalLong values: https://github.com/jdbi/jdbi/blob/0420cc462f98565062bd577cf5d3a4d92a621060/core/src/main/java/org/jdbi/v3/core/argument/OptionalArgumentFactory.java#L42

But it looks like the long ArgumentFactory is using types.INTEGER for some reason: https://github.com/jdbi/jdbi/blob/0420cc462f98565062bd577cf5d3a4d92a621060/core/src/main/java/org/jdbi/v3/core/argument/PrimitivesArgumentFactory.java#L31 -- I wonder if this was updated to use types.BIGINT if your code would work.

[rafiss]

I figured out my first point about re-preparing the statement: #67651

I tested locally and confirmed that your example works with this change.

[joschi]

I wonder if this was updated to use types.BIGINT if your code would work.

It indeed works for us in the case described (tested against a locally-built SNAPSHOT version of Jdbi with the necessary changes).

The respective PR against Jdbi is here: jdbi/jdbi#1902

[yuzefovich]

Thanks Rafi for the investigation!

By the way, I confirmed that the non-vectorized engine does not attempt to perform this cast. So I don't see how this would be a planning issue.

The row-by-row engine doesn't respect widths of integers and always operates as if on INT8 whereas the vectorized engine does respect the widths and is strict about them.

[stevenschlansker]

Thank you for reporting and sending a PR to fix this bug! Jdbi 3.21.0 was just released with the fix and should be available on Maven Central shortly.