This change is 

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

pkg/sql/executor.go, line 556 at r1 (raw file):

		if result.Rows != nil && !result.Columns.TypesEqual(stmt.Columns) {
			results.Close(session.Ctx())
			return StatementResults{}, pgerror.NewError(pgerror.CodeFeatureNotSupportedError,

is this really the right code? seems surprising.

pkg/sql/executor.go, line 1337 at r1 (raw file):

		}
		results, err := e.ExecutePreparedStatement(session, prepared, &parser.PlaceholderInfo{Values: qArgs, Types: prepared.SQLTypes})
		if err == nil && len(results.ResultList) == 1 {

is it actually possible for len(results.ResultList) > 1? in other words, why can't this be a "normal" error check?

pkg/sql/pgwire/pgwire_test.go, line 1621 at r1 (raw file):

		t.Fatal(err)
	}

random newline

pkg/sql/pgwire/pgwire_test.go, line 1636 at r1 (raw file):

	}
	if _, err := stmt.Exec(); !testutils.IsError(err, "must not change result type") {
		t.Fatal("expected error")

include the error as %v (it might not be nil)

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/sql/executor.go, line 556 at r1 (raw file):

Yup:

postgres=# EXECUTE x;
ERROR:  0A000: cached plan must not change result type
LOCATION:  RevalidateCachedQuery, plancache.c:718

pkg/sql/executor.go, line 1337 at r1 (raw file):

No, there's no way for a prepared statement to return more than one result set.

pkg/sql/pgwire/pgwire_test.go, line 1621 at r1 (raw file):

Done.

pkg/sql/pgwire/pgwire_test.go, line 1636 at r1 (raw file):

done

Comments from Reviewable

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

pkg/sql/executor.go, line 1337 at r1 (raw file):

So can this be a normal error check?

pkg/sql/pgwire/pgwire_test.go, line 1635 at r2 (raw file):

	}
	if _, err := stmt.Exec(); !testutils.IsError(err, "must not change result type") {
		t.Fatalf("expected error: %v", err)

"unexpected"

Comments from Reviewable

mod existing comments

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

Comments from Reviewable

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

pkg/sql/executor.go, line 1337 at r1 (raw file):

No, because it can also return 0 results, so we still have to check the length. Well, I used to have a normal error check, and a return Result{}, nil below, but that seemed redundant. I can change it back to that if you feel strong.

Comments from Reviewable

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

pkg/sql/pgwire/pgwire_test.go, line 1635 at r2 (raw file):

Done.

Comments from Reviewable

I believe @knz had some comments about the correctness of this implementation, though I don't see them here.

Ah, they were made on the issue, rather than this PR. #16062 (comment)

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

@knz I've added a test that addresses your concern, I think, and it does indeed prevent the INSERT from running, so I think we're safe. Can you give a look?

I think one of us is missing something. There is a test that:

1. prepares an insert
2. executes a schema change
3. executes the prepared insert
4. observes that the insert did not take place

Or is the test an logic_test/prepare not doing that?

Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion.

pkg/sql/executor.go, line 1348 at r4 (raw file):

I've made some changes here to be similar to yours. I don't think it's a good idea to return a single result because 1) in the normal case of prepared statements being executed through pgwire, we will have to reform it back into a StatementResults, and 2) zero results is valid if we are executing the empty prepared statement.

Comments from Reviewable

Yes correct, the logic test is insufficient to test this.

The problem here is that the PREPARE and EXECUTE statements are executed via the Executor logic and thus inside the error check that aborts the transaction.

In contrast, a pgwire prepare message is processed outside of the executor logic, and there the Executor will not see the error and not prevent the txn from committing.

Try via pgwire_test.go to see what I mean.

(Note: also, since the code paths are separate, we need both tests. I am not suggesting you replace the logic test by a pgwire test entirely).

Reviewed 1 of 1 files at r5.
Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

pkg/sql/executor.go, line 1348 at r4 (raw file):

Ack. I still think the code should contain an explicit check that there are no more results beyond [0], that would need to be Close()d.

Comments from Reviewable

I've changed this to instead plumb down the expected results into the executor. I don't like calling plan.prepare down there, but I'm not aware of a better way. It is possible calling that is not idempotent, but all the tests pass. Any thoughts about doing it this way? Ugly.

Reviewed 2 of 5 files at r1, 1 of 2 files at r4, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

pkg/sql/executor.go, line 595 at r6 (raw file):

	// for each result in the reply.
	results := e.execParsed(session, stmts, pinfo, copyMsgNone, []sqlbase.ResultColumns{stmt.Columns})
	if len(results.ResultList) > 1 {

Same problem with having this check here again -- if the error occurs, the txn is still committed.
The check needs to be deeper.

pkg/sql/executor.go, line 750 at r6 (raw file):

		var expect []sqlbase.ResultColumns
		if expectResultColumns != nil {
			expect = expectResultColumns[:len(stmtsToExec)]

I'm confused: how is this logic supposed to work since there is at most a single statement to execute?

pkg/sql/executor.go, line 1419 at r6 (raw file):

	if expectResultColumns != nil {
		plan, err := planner.prepare(session.Ctx(), stmt)

This is not the right location to do this. The check must be present inside execStmtInParallel() and execStmt(). I strongly suggest factoring it inside makePlan().

If I were you I would proceed as follows:

• define a new type type Statement struct { ast parser.Statement; expectedTypes ResultColumns }, and use this new Statement struct instead of parser.Statement everywhere in this package; this will take care of removing the extra arguments you just added and that indeed feel "ugly"
• make makePlan() use the expectedTypes field, as you intend

pkg/sql/sqlbase/result_columns.go, line 64 at r6 (raw file):

	}
	for i, c := range r {
		if c.Typ == parser.TypeNull || other[i].Typ == parser.TypeNull {

Can you explain what this special casing with NULL is about. Also, it misses a test.

Comments from Reviewable

I've done a major refactor to create a new sql.Statement type so as to not pass down the result columns as a separate variable, as suggested by knz. RFAL.

This looks much better. So many thanks for introducing sql.Statement. It's great.
I still have two concerns remaining, see reviewable for the 2 comments still open.

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

Comments from Reviewable

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

pkg/sql/executor.go, line 1348 at r4 (raw file):

execPrepared will return an error and close results if there are more than 1. Do you think that logic needs to be duplicated here? I think the comment is clear enough.

pkg/sql/executor.go, line 595 at r6 (raw file):

This is an error that should never occur because all prepared statements can only contain one statement during creation, so it's closer to a debug assertion.

pkg/sql/executor.go, line 750 at r6 (raw file):

expectResultColumns is a slice of ResultColumns, and thus is used to specify either all or none of the statement results. Even though prepared statements are guaranteed to ever only have a single statement, I thought it was better to have this variable be a slice so that it could just mirror stmts. I'm not opposed to changing it, since it is kind of silly.

pkg/sql/executor.go, line 1419 at r6 (raw file):

Done.

pkg/sql/sqlbase/result_columns.go, line 64 at r6 (raw file):

Done.

Comments from Reviewable

sql.Statement is looking 🔥!

Review status: 13 of 15 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

Comments from Reviewable

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

pkg/sql/executor.go, line 1348 at r4 (raw file):

No this comment was outdated.

pkg/sql/executor.go, line 595 at r6 (raw file):

then the pgerror should be InternalError (which we'll soon catch with error reporting to the reg server)

pkg/sql/executor.go, line 750 at r6 (raw file):

This is fine, thanks

pkg/sql/sqlbase/result_columns.go, line 64 at r6 (raw file):

Can you dig this deeper then. I think it is possible that the PREPARE will resolve some non-NULL type, and then EXECUTE will infer TypeNull; however I believe the converse is impossible. This means only one of the two cases should be tested here.

Also I think this still misses a SQL prepare/execute test, not just a test of this function.

Comments from Reviewable

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/sql/executor.go, line 595 at r6 (raw file):

Done.

pkg/sql/sqlbase/result_columns.go, line 64 at r6 (raw file):

Yes, I had a small error and this has been changed so that only the RHS argument can be changed to NULL. There are tests in pgwire with SELECT CASE that cover the INT -> NULL case (which is how I found out I needed to write this block). Good?

Comments from Reviewable

terrific.

Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

FYI: this has not been cherry-picked for 1.0.2 yet.

Yes, working on this today. Real bad merge.