Cockroach: sql: support "DEFERRABLE INITIALLY DEFERRED" for constraints

[alex]

Currently attempting to use this syntax results in a syntax error:

ALTER TABLE "auth_permission" ADD CONSTRAINT "auth_permission_content_type_id_2f476e4b_fk_django_co" FOREIGN KEY ("content_type_id") REFERENCES "django_content_type" ("id") DEFERRABLE INITIALLY DEFERRED

It'd be great if it was supported; or failing that if it at least errored with an explicit "this feature will not be supported" message (as is done with SAVEPOINT)

[dt]

I'll add them to the grammar and return a more helpful error message for now, until they get implemented.

Also, I'm just curious: is that a query that django generates out of the box (versus something hand-written)? I just ask because I'm wondering if this is something that we'll need to implement for django compatibility.

[alex]

Yeah, it generates these when creating foreign keys.

[dt]

Unassigned since I'm not actively working on this right now. In case anyone decides to try this, I think we might be able to accumulate checks to perform in the session, and then run them during COMMIT. How to represent them and how that interacts with distsql need to be thought out further though.

[knz]

I think it is not incorrect as per sql specs to ignore the additional specifier and just perform the check immediately. I understand that deferrable means the engine may defer checking the constraint, but it doesn't need to. Is there a reason why we couldn't do this as a stop-gap?

[dt]

Well, if the user says INITIALLY DEFERRED, silently ignoring that seems bad.

Also, you _need_ deferrable to work with circular references.

[knz]

ah, I forgot about circular references. my bad.

[dt]

Yeah, deferrable isn't so much for the engine as it is for a txn, where you don't care that every intermediate state validate (yay isolation), so allowing them not to allows otherwise impossible behaviors (e.g. circular FKs) or significant performance optimizations (e.g. batched validation).

[akkinenirajesh]

I am willing to contribute to this issue. But new to our code base. Can someone give an highlevel plan which I will try to convert to code.

Initial idea: Collect all the checks(Some struct) into some collection inside client.Txn if differable and/or differed. Then while commit run those checks, and throw errors.

[dt]

@akkinenirajesh Thanks! I'm happy help with codebase/design/whatever!

That high level design sounds great, with one minor nit: the checks are sql-specific so I think we'd accumulate them on sql.Session (another lifecycle-scope object in sql), just since client.Txn is a little lower-level, pure KV logic..

We should get some thoughts from distsql folks (@andreimatei @RaduBerinde) about how this will be handled once they start distributing writes -- we can and should start with a local-only implementation and get tests in place, but they might have some thoughts on how the design will be reusable once multiple nodes participate in writes and will need the deferred checks they accumulate to propagate back to the coordinator for aggregation.

[andreimatei]

@akkinenirajesh see this walkthrough for various code pointers. https://github.com/andreimatei/cockroach/blob/blog/docs/life_of_a_query.md

The struct that is tied to the lifetime of a sql transactions is this txnState.

[dt]

@akkinenirajesh one thing to note, I think you'll want to define the struct used for representing a deferred check in a proto (maybe structured.proto) so that when we get to distributing writes, we can easily serialize and send them back to the coordinator.

[cuongdo]

Moved this to "later". This seems like too significant of a feature to target for 1.0.

[justinj]

This is still in "later", but for future reference here is a very interesting and in-depth article on deferrable constraints.

[jseldess]

FWIW, ran into this when using CockroachDB as the backend for the standard Django tutorial: https://docs.djangoproject.com/en/1.11/intro/tutorial02/#database-setup

[knz]

Earlier last month I created a duplicate issue #31632 by mistake. Unfortunately, we have started to track telemetry with that new issue number, so I cannot do "the right thing" and close the newer issue in favor of the older one.

Instead I am closing this issue in reference of the newer one. Please update references to #9297 and point to #31632 instead. Thank you.