Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CREATE TABLE and INSERT fail in same Transaction #14548

Closed
timchunght opened this issue Apr 2, 2017 · 12 comments
Closed

CREATE TABLE and INSERT fail in same Transaction #14548

timchunght opened this issue Apr 2, 2017 · 12 comments
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Milestone

Comments

@timchunght
Copy link

timchunght commented Apr 2, 2017

I believe there is a bug in the transaction. So when I CREATE TABLE and INSERT into the same table in the same transaction, cockroach just hangs indefinitely. This bug was discovered when using the goose migration tool with Cockroach. All the code only uses the lib/pq and the default database/sql and same issue persists even in isolation with the code moved into an individual script.

Everything works perfectly in postgres. However, if I were to move the CREATE TABLE outside of the transaction, everything works. Please advise.

txn, err := db.Begin()
	if err != nil {
		log.Println("cannot start transaction", err)

	}

createVersionTableSql := `CREATE TABLE goose_db_version (
           	id serial NOT NULL,
               version_id bigint NOT NULL,
               is_applied boolean NOT NULL,
               tstamp timestamp NULL default now(),
               PRIMARY KEY(id)
           );`
if _, err := txn.Exec(createVersionTableSql); err != nil {

	log.Println("error so creatingVersionTable", err)

}

version := 0
applied := true
if _, err := db.Exec("INSERT INTO goose_db_version (version_id, is_applied) VALUES ($1, $2);", version, applied); err != nil {

	txn.Rollback()
	log.Println("error inserting goose_db_version", err)
}

txn.Commit()
@benesch
Copy link
Contributor

benesch commented Apr 3, 2017

Hi @timchunght, thanks for reporting! In case you're curious, there are a few existing issues tracking oddities with schema changes and transactions:

As far as I know this specific bug hasn't yet been discovered, but I'll defer triaging to the schema change experts. +cc @vivekmenezes

The workaround, as you've discovered, is to execute the CREATE TABLE statement in a separate transaction. Based on the code sample you've posted, it seems this workaround may be perfectly viable, but the fact that we're not compatible with goose out-of-the-box is unfortunate.

@benesch benesch added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. labels Apr 3, 2017
@petermattis
Copy link
Collaborator

Creating a table was not historically a schema change in the same sense as other schema changes operations and some effort was made to allow subsequent inserts to the new table within the same transaction to work. Did we regress accidentally here, or was this intentional?

@petermattis petermattis added this to the 1.0 milestone Apr 3, 2017
@dt
Copy link
Member

dt commented Apr 3, 2017

At least when there are relations between tables (fks, views), CREATE does need to be a schema change (with the committed intermediate state + waitForOneVersion dance).

@vivekmenezes
Copy link
Contributor

#14368 gets rid of this kind of surprise by explicitly making this an error. It's in review.

@petermattis this used to work the way you describe but now is executed like a schema change. It's strictly not necessary, and we can consider adding back the lost behavior post 1.0

@timchunght
Copy link
Author

timchunght commented Apr 4, 2017

@benesch Thanks and I have looked at them before filling this issue. It appears that, as you acknowledged, transaction might have slight unexpected behaviours. Nonetheless, we were able to do a 'fix' by just splitting them into two subsequent transactions. However, we also sacrificed on the transaction guarantee (though not a big deal for this case, it could potentially be useful given a lot of existing PG tools implement transactions that combine CREATE TABLE with immediate INSERT) I had to fork the library and change that specific part...

@vivekmenezes
Copy link
Contributor

@timchunght thanks for pointing that out. We'll keep this issue open so that it gets fixed.

@cuongdo
Copy link
Contributor

cuongdo commented Apr 4, 2017

Is this fixed now that you've merged #14368?

@benesch
Copy link
Contributor

benesch commented Apr 4, 2017

Still hangs forever for me on the latest master. (Gist: https://gist.github.com/benesch/cce1a20c0981bc0d45c3a47165c47834)

@vivekmenezes
Copy link
Contributor

vivekmenezes commented Apr 4, 2017 via email

@vivekmenezes
Copy link
Contributor

This bug only happens when the INSERT statement used in the transaction uses placeholder variables. A workaround is to not use placeholder variables when running an INSERT following a CREATE TABLE in the same transaction. I'm going to fix this but the workaround might work for you in the interim.

@vivekmenezes
Copy link
Contributor

This is a bug in sql PREPARE. It uses a different transaction when reading the table descriptor to type check the placeholder variables. For the case where a table descriptor is created in the same transaction it needs to use the transaction that created the table while type checking the PREPARE placeholder variables.

@jordanlewis
Copy link
Member

Similar to the issue we saw in #14473, I believe the root cause of this is that Prepare statements do not run within a parent transaction if one exists: executor.go#L449

@jordanlewis jordanlewis added C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community and removed O-deprecated-community-questions labels Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
None yet
Development

No branches or pull requests

8 participants