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

sql: support transactionally dropping a column and creating one with the same name #109587

Open
Jolg42 opened this issue Aug 28, 2023 · 7 comments
Assignees
Labels
A-tools-prisma C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner

Comments

@Jolg42
Copy link

Jolg42 commented Aug 28, 2023

Describe the problem

This works with PostgreSQL but not with CockroachDB

To Reproduce

  1. Start CockroachDB v23.1
  2. Create a table
CREATE DATABASE hello;
USE hello;

CREATE TABLE "Dog" (
    id              SERIAL PRIMARY KEY,
    name            TEXT NOT NULL,
    is_good_dog     BOOLEAN NOT NULL DEFAULT TRUE
);
  1. Then execute:
begin;
      ALTER TABLE "Dog" DROP COLUMN is_good_dog;
      ALTER TABLE "Dog" ADD COLUMN is_good_dog INTEGER NOT NULL DEFAULT 100;

See error

Query 3 ERROR: ERROR:  column "is_good_dog" being dropped, try again later

Expected behavior
It should just work

Environment:

  • CockroachDB version: v23.1
  • Server OS: macOS using Docker
  • Client app: Prisma or Table Plus

Additional context
Spotted in Prisma Migrate test: https://github.com/prisma/prisma-engines/blob/44742ac2c7c57c525d1ab277dd9fd9496eb92d8b/schema-engine/sql-migration-tests/tests/migrations/cockroachdb/failure_modes.rs#L100-L129
See
prisma/prisma#20851
prisma/prisma#20707

Related cockroach db issue about indexes #98494

Jira issue: CRDB-31009

Epic CRDB-31472

@Jolg42 Jolg42 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 28, 2023
@blathers-crl
Copy link

blathers-crl bot commented Aug 28, 2023

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

  • @cockroachdb/disaster-recovery (found keywords: c2c)
  • @cockroachdb/sql-foundations (found keywords: ALTER TABLE,Prisma)

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

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

@blathers-crl blathers-crl bot added A-disaster-recovery O-community Originated from the community X-blathers-triaged blathers was able to find an owner T-disaster-recovery labels Aug 28, 2023
@blathers-crl
Copy link

blathers-crl bot commented Aug 28, 2023

cc @cockroachdb/disaster-recovery

@Jolg42
Copy link
Author

Jolg42 commented Aug 28, 2023

This works as a workaround:

rename the column first:

begin;
	  ALTER TABLE "Dog" RENAME COLUMN is_good_dog TO is_good_dog_drop;
      ALTER TABLE "Dog" DROP COLUMN is_good_dog_drop;
      ALTER TABLE "Dog" ADD COLUMN is_good_dog INTEGER NOT NULL DEFAULT 100;

@ferdypruis
Copy link

ferdypruis commented Aug 28, 2023

It's quite possible that's not actually a workaround, but the rename is currently finishing fast enough not to interfere with the drop.

This behavior changed in v22.2 when enable_implicit_transaction_for_batch_statements was enabled by default in #76834.

With it enabled, as far as I know, you need to commit both statements individually;

begin;
      ALTER TABLE "Dog" DROP COLUMN is_good_dog;
commit;

begin;
      ALTER TABLE "Dog" ADD COLUMN is_good_dog INTEGER NOT NULL DEFAULT 100;
commit;

@msbutler msbutler added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed A-disaster-recovery T-disaster-recovery labels Aug 28, 2023
@rafiss
Copy link
Collaborator

rafiss commented Aug 29, 2023

We have work underway that would support this better, but it won't be out until 23.2 or later. Until then, the rename workaround makes sense to use. Is this issue urgent for Prisma?

@Jolg42
Copy link
Author

Jolg42 commented Oct 24, 2023

(late answer) No, it does not look urgent for Prisma. Thanks!
I wanted to dump the info I found while it was fresh.

@Atomzwieback
Copy link

Atomzwieback commented Nov 20, 2024

We have work underway that would support this better, but it won't be out until 23.2 or later. Until then, the rename workaround makes sense to use. Is this issue urgent for Prisma?

Were now at v24.2.4 (atleast at our paid cluster) and this is still a problem in our case its with indexes that couldn`t be dropped and created in the same statement. Is there any new plan or ETA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tools-prisma C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner
Projects
No open projects
Status: Triage
Development

No branches or pull requests

6 participants