sql: ensure a Prepare referencing a table created in the same txn works

[vivekmenezes]

This change is 

[jordanlewis]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/executor.go, line 457 at r2 (raw file):

	}
	txn.Proto().OrigTimestamp = e.cfg.Clock.Now()

I'll do another review later, but at the very least this set needs to go up in the if txn == nil block, otherwise we're overwriting the OrigTimestamp of the pre-existing txn.

---

Comments from Reviewable

[vivekmenezes]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/executor.go, line 457 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I'll do another review later, but at the very least this set needs to go up in the if txn == nil block, otherwise we're overwriting the OrigTimestamp of the pre-existing txn.

Done.

---

Comments from Reviewable

[vivekmenezes]

I think this fix is not enough we also need to update txnState.txn when we create the new transaction

---

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

---

Comments from Reviewable

[vivekmenezes]

Added another commit for sql: disallow a DROP TABLE followed by a Prepare() in the same txn in this PR

---

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

---

Comments from Reviewable

[benesch]

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

---

pkg/sql/executor.go, line 451 at r5 (raw file):

	// TODO(andrei): is this OK? If we're preparing as part of a SQL txn, how do
	// we check that they're reading descriptors consistent with the txn in which
	// they'll be used?

I'm not sure this PR fully addresses @andreimatei's TODO. As I understand it his TODO also refers to the broader problem of using a prepared statement in a completely different transaction:

BEGIN;
CREATE TABLE t (a int, b int);
COMMIT;

BEGIN;
PREPARE query AS SELECT a + b from t;
COMMIT;

BEGIN;
ALTER TABLE DROP COLUMN a;
ALTER TABLE ADD COLUMN a string;
COMMIT;

BEGIN;
EXECUTE query; -- ?!?
COMMIT;

But I could be totally off base.

---

Comments from Reviewable

[vivekmenezes]

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

---

pkg/sql/executor.go, line 451 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I'm not sure this PR fully addresses @andreimatei's TODO. As I understand it his TODO also refers to the broader problem of using a prepared statement in a completely different transaction:

BEGIN;
CREATE TABLE t (a int, b int);
COMMIT;

BEGIN;
PREPARE query AS SELECT a + b from t;
COMMIT;

BEGIN;
ALTER TABLE DROP COLUMN a;
ALTER TABLE ADD COLUMN a string;
COMMIT;

BEGIN;
EXECUTE query; -- ?!?
COMMIT;

But I could be totally off base.

I've added a comment about what txn this prepare needs to use. I believe it should be okay now. It needs to only pick up a txn if it already existed, and if one doesn't exist it can make a new txn to pick up a new lease. If future statements do not use the same txn that's okay. The particular code segment you wrote up is a broader problem where the prepare gets type checked against a much older table version. I haven't tried your example out because we don't actually support PREPARE as a first class SQL request, it's part of the pg wire protocol used for placeholder queries.

---

Comments from Reviewable

[andreimatei]

If it doesn't need to be the same, then why ever make it the same? Why is this change necessary exactly?

I think that this is the opposite of what we generally want to do; I think we want to always obtain leases in a fresh transaction - otherwise what's preventing us from obtaining a lease at an arbitrarily old version of a descriptor? (i.e. a client txn that's been lingering for a while might not see new versions of the descriptor... unless its attempt to write a lease at an old version is guaranteed to conflict with reads that will have been performed in the process of incrementing the descriptor version...).
There's some subtleties here I'm not currently sure of.
If you don't mind, I'd like to talk to you about our lease philosophy more. But I'm just boarding the plane for this California meetup.

[vivekmenezes]

I agree we have #6774 . Feel free to add more of your concerns to that issue and let's not block this PR.

The reason why the entire leasing mechanism takes an external transaction is only for the purposes of supporting INSERT after CREATE TABLE.

See this comment:
https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/lease.go#L155

[benesch]

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

---

pkg/sql/executor.go, line 451 at r5 (raw file):

Previously, vivekmenezes wrote…

I've added a comment about what txn this prepare needs to use. I believe it should be okay now. It needs to only pick up a txn if it already existed, and if one doesn't exist it can make a new txn to pick up a new lease. If future statements do not use the same txn that's okay. The particular code segment you wrote up is a broader problem where the prepare gets type checked against a much older table version. I haven't tried your example out because we don't actually support PREPARE as a first class SQL request, it's part of the pg wire protocol used for placeholder queries.

👍 sounds like that TODO was referencing #6774, but it wasn't the right place to do so. Thanks for clarifying!

---

Comments from Reviewable

[jordanlewis]

Reviewed 1 of 1 files at r1, 2 of 2 files at r3, 2 of 2 files at r6.
Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

pkg/sql/pgwire/pgwire_test.go, line 464 at r6 (raw file):

	}

	if _, err := tx.Prepare(`INSERT INTO d.kv (k,v) VALUES ($1, $2);`); err != nil {

You should actually execute the result of this prepare as well, I think, to ensure that it works as well.

---

Comments from Reviewable

[vivekmenezes]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/sql/pgwire/pgwire_test.go, line 464 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You should actually execute the result of this prepare as well, I think, to ensure that it works as well.

Done.

---

Comments from Reviewable

[jordanlewis]

---

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

---

Comments from Reviewable

[andreimatei]

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

---

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

Previously, vivekmenezes wrote…

I agree we have #6774 . Feel free to add more of your concerns to that issue and let's not block this PR.

The reason why the entire leasing mechanism takes an external transaction is only for the purposes of supporting INSERT after CREATE TABLE.

See this comment:
https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/lease.go#L155

Well I was thinking that another idea for dealing with tables created in the current txn is to maintain a list of descriptors in the txnState. It's nice because you don't actually need a lease to operate on these, just a descriptor.

---

Comments from Reviewable

[vivekmenezes]

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

---

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

Previously, andreimatei (Andrei Matei) wrote…

Well I was thinking that another idea for dealing with tables created in the current txn is to maintain a list of descriptors in the txnState. It's nice because you don't actually need a lease to operate on these, just a descriptor.

Don't quite follow what you mean but I've always hated the fact that you need to pass in a txn into the leasing logic.

---

Comments from Reviewable