sql: do not evaluate AOST timestamp in session migrations

[rafiss]

fixes https://github.com/cockroachlabs/support/issues/2510
refs #108305
Release note (bug fix): Fixed a bug where a session migration performed by SHOW TRANSFER STATE would not handle prepared statements that used the AS OF SYSTEM TIME clause. Users who encountered this bug would see errors such as expected 1 or 0 for number of format codes, got N. This bug was present since v22.2.0.

[cockroach-teamcity]

This change is 

[rafiss]

NB: I'm only backporting this to v23.1 and not 22.2 since we should prioritize fixing this for Serverless clusters, which are all on 23.1 now.

[jaylim-crl]

Can we also log the error here instead of swallowing it during session migration?

cockroach/pkg/sql/conn_executor_prepare.go

Lines 291 to 293 in 7fe93d7

	if err := prepare(ctx, ex.state.mu.txn); err != nil && origin != PreparedStatementOriginSessionMigration {
	return nil, err
	}

As an aside, @jeffswenson thinks that we should have errored out if we fail to prepare any statements during deserialization instead of having it error when the user uses them.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

[rafiss]

Added the warning log.

As an aside, @jeffswenson thinks that we should have errored out if we fail to prepare any statements during deserialization instead of having it error when the user uses them.

I disagree with this - even without session migrations, it is possible for a prepared statement to become invalid. One example is if a schema change occurs on a table referenced by a prepared statement. That was the motivation for this commit: dda2fa9. If we error out eagerly, then that would be contrary to the goal of making session migrations transparent to the user.

IMO the missing piece here is that we weren't logging the reason why a statement couldn't be prepared.

[jeffswenson]

Added the warning log.

As an aside, @jeffswenson thinks that we should have errored out if we fail to prepare any statements during deserialization instead of having it error when the user uses them.

I disagree with this - even without session migrations, it is possible for a prepared statement to become invalid. One example is if a schema change occurs on a table referenced by a prepared statement. That was the motivation for this commit: dda2fa9. If we error out eagerly, then that would be contrary to the goal of making session migrations transparent to the user.

IMO the missing piece here is that we weren't logging the reason why a statement couldn't be prepared.

The current behavior only makes sense to me IFF we are confident that all of the errors are the result of schema changes. If there is a bug in the prepare logic that impacts transferred sessions, it would be much better for the transfer to fail than to have the session limp along with corrupted state. The warning log is an improvement, but I'm still worried this will hide hard to track down bugs.

Thinking through how applications are developed, I'm skeptical that prepared statements broken by schema changes is actually a common scenario. Usually schema changes are backwards compatible with the running application.

[rafiss]

The current behavior only makes sense to me IFF we are confident that all of the errors are the result of schema changes.

I have confidence. This bug being fixed seems to be the first issue we've heard about in a over year of this feature being used, so I believe we can get to a place where unexpected errors during session transfers do not happen unless something is majorly wrong.

Given how commonly people ask about invalidated prepared statements in bug trackers, I think it is probably more common than you predict, and failing eagerly would be more disruptive. (The user's experience would be that their connection was dropped for no clear reason.)

• React to invaildated prepared statements (0A000: cached plan must not change result type) npgsql/npgsql#4015
• handling "cached plan must not change result type" errors jackc/pgx#927
• https://stackoverflow.com/questions/2783813/postgres-error-cached-plan-must-not-change-result-type
• PG::FeatureNotSupported: ERROR: cached plan must not change result type rails/rails#30507
• The ConnectorError cached plan must not change result type periodically appears (Postgres). prisma/prisma#7678

[jeffswenson]

Given how commonly people ask about invalidated prepared statements in bug trackers, I think it is probably more common than you predict, and failing eagerly would be more disruptive. (The user's experience would be that their connection was dropped for no clear reason.)

I'm open to keeping the current implementation as long as we periodically check for this log warning.

That said, if there is a broken plan, closing the connection is likely the least disruptive thing we can do. We can't migrate sessions in the middle of a transaction and as long as they are using a connection pool, the connection will be reopened for them.

[rafiss]

I'm open to keeping the current implementation as long as we periodically check for this log warning.

for sure. i will add it to our SLI dashboard

[yuzefovich]

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rafiss)

[rafiss]

tftr!

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 5befaa7 to blathers/backport-release-23.1-108503: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

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