sql: introduce preliminary support for savepoints

[knz]

Informs #10735
This implementation corresponds to compatibility level L1 in the RFC.

This PR defines:

• the savepoint name stack-like scoping to implement nesting
• the savepoint lookup and rewind logic
• the separation of logic between restart and "regular" savepoints
• working logic to release and roll back regular savepoints when no errors have occurred

What is not supported:

• defining restart savepoints "under" regular savepoints (out of scope according to RFC)
• rolling back to a regular savepoint over an error (this is planned for a later PR)

[cockroach-teamcity]

This change is 

[knz]

Some experiment about the current DDL behavior:

GOOD:

> begin;
  savepoint foo;
  create table t(x int);
  insert into t(x) values (1);
  rollback to savepoint foo;
  commit;
COMMIT

> select * from t;
ERROR: relation "t" does not exist
SQLSTATE: 42P01

BAD:

> begin; 
  savepoint foo; 
  create table t(x int); 
  rollback to savepoint foo;
ROLLBACK
> insert into t(x) values (1);
INSERT

(Bad because the create is rolled back so insert should fail to see the table)

But then:

> commit;
COMMIT

> select * from t;
ERROR: relation "t" does not exist
SQLSTATE: 42P01

So at least the creation gets eventually rolled back.

[knz]

This PR is ready for initial review.

Here are the pieces missing before it can merge:

• PR engine: Handle put when all writes are ignored and there are no committed values #44697 with fix needs to be merged
• I need to add a check in the code that there was no DDL "under" a savepoint being rolled back.

However I think the general structure of the code is ready for review and I'd appreciate the early feedback.

[knz]

The failed tests show that the restart savepoint is erroneously dropped and becomes unusable after a first RELEASE SAVEPOINT cockroach_restart encounters a retry error.

This is is because COMMIT implicitly drops all savepoints, and RELEASE SAVEPOINT cockroach_restart reuses the COMMIT code.

I'll split the commit code in two to fix.

[andreimatei]

There's a lot of distinction in this PR between "restart" and non-restart savepoints. I wonder if there's a reason for this distinction. The only reason I can think of is that in the case of rolling back to cockroach_restart, we definitely don't want to clean up intents, whereas for everything else we might want to clean them up. Except with the current implementation, we never clean up any intents (right)? So I wonder if we really need the distinction. Because if we don't, it looks to me like a lot of code would be simpler.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @knz, and @nvanbenschoten)

---

pkg/sql/conn_executor_savepoints.go, line 260 at r5 (raw file):

// special "restart" semantics.
func (ex *connExecutor) isRestartSavepoint(savepoint tree.Name) bool {
	if ex.sessionData.ForceSavepointRestart {

Does this ForceSavepointRestart session var still serve a purpose?

---

pkg/sql/conn_executor_savepoints.go, line 261 at r5 (raw file):

func (ex *connExecutor) isRestartSavepoint(savepoint tree.Name) bool {
	if ex.sessionData.ForceSavepointRestart {
		// The session sxetting force_savepoint_restart implies that all

sxetting

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

---

pkg/sql/conn_executor_savepoints.go, line 260 at r5 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Does this ForceSavepointRestart session var still serve a purpose?

Unfortunately, yes. There's some ORM out there (presumably, Spring?) where the app cannot customize the identifiers generated for savepoints. With that ORM the only way to implement a retry loop is to make all savepoints retry savepoints.

I did push back against it with Jordan and andy but they would like to decouple the development of a specific adapter for that ORM from the savepoint work. That sounded reasonable to me, so I want to keep that logic here until the separate AppRel work makes progress.

---

pkg/sql/conn_executor_savepoints.go, line 261 at r5 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

sxetting

Done.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @knz, and @nvanbenschoten)

---

pkg/sql/conn_executor_savepoints.go, line 260 at r5 (raw file):

Previously, knz (kena) wrote…

Unfortunately, yes. There's some ORM out there (presumably, Spring?) where the app cannot customize the identifiers generated for savepoints. With that ORM the only way to implement a retry loop is to make all savepoints retry savepoints.

I did push back against it with Jordan and andy but they would like to decouple the development of a specific adapter for that ORM from the savepoint work. That sounded reasonable to me, so I want to keep that logic here until the separate AppRel work makes progress.

I understand but I'm still hoping that the special treatment of cockroach_restart can also go away :)

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

---

pkg/sql/conn_executor_savepoints.go, line 260 at r5 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I understand but I'm still hoping that the special treatment of cockroach_restart can also go away :)

I hope so too. But baby steps. I want to get to that L1 step in the RFC then also L2 (error recovery) and then we'll talk about this again.

[knz]

The DDL blocking logic is now ready.
I have added numerous tests and transferred a few thing from logic tests to the new datadriven-based TestSavepoints.

RFAL. Note that I plan to implement error recovery in a subsequent PR.

[andreimatei]

Only if you're curious, I've been trying my hand over here: https://github.com/andreimatei/cockroach/commits/pr/43051
I think the code is getting there, but it's not ready - I seem to have broken something in one of the new tests.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @nvanbenschoten)

[knz]

subsumed by #45566