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: can't create table, index and foreign key in the same transaction #24626

Closed
ruzkant opened this issue Apr 10, 2018 · 17 comments · Fixed by #25786
Closed

sql: can't create table, index and foreign key in the same transaction #24626

ruzkant opened this issue Apr 10, 2018 · 17 comments · Fixed by #25786
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-known-limitation docs-todo O-community Originated from the community

Comments

@ruzkant
Copy link

ruzkant commented Apr 10, 2018

This is on cockroachdb 2.0.0 on linux 64 bit.

These statements:

start transaction;
create table t_parent (id varchar primary key);
create table t_child (id varchar primary key, parent_id varchar);
create index idx_t_child_parent_id on t_child (parent_id);
alter table t_child add constraint fk_t_child_parent_id foreign key (parent_id) references t_parent (id);
commit;

Give this error:

pq: foreign key requires an existing index on columns ("parent_id")

I am not sure if this is an issue or a known limitation. I am working on a java application which uses automatic migration from the create scripts, but they fail when used in a transaction. I hope to avoid having only the foreign keys moved to their own transaction and separating the index code from its corresponding alter code.

@knz
Copy link
Contributor

knz commented Apr 10, 2018

Hi @ruzkant
First of all thanks for your interest on CockroachDB!
The reason you observe this error is that adding an index to an existing table is an asynchronous operation and it gets run at the end of the transaction, after the alter statement.

The workaround for this limitation is to organize your transaction as follows:

start transaction;
create table t_parent (id varchar primary key);
create table t_child (id varchar primary key, parent_id varchar, index idx_t_child_parent_id (parent_id));
alter table t_child add constraint fk_t_child_parent_id foreign key (parent_id) references t_parent (id);
commit;

Would that be a suitable solution?

@ruzkant
Copy link
Author

ruzkant commented Apr 10, 2018

@knz Thanks for the prompt response. Unfortunately this doesn't help me as much as I'd like because postgres does not support the above syntax.

At the moment because cockroach is aiming for postgres compatibility with tools, it makes it hard to differentiate. For example the java JDBC driver metadata always returns postgresql as the product and the version numbers are postgres aligned. So the tools aren't easily able to differentiate cockroachdb, which in turn means the path of least resistance at the moment seems to be to see how much mileage I can get maintaining one set of scripts and changes. I am dealing with a mix of pure sql migration and create scripts as well as liquibase changesets.

In the case of liquibase it is slightly easier to maintain both as I can move the alters to a different changeset, but I am awaiting feedback from the project maintainers. There is an option to extend liquibase to make cockroachdb a first class citizen. First would need to find a query or something to differentiate it from postgres, and a bit of poking didn't reveal a simple clean one that I could see, but I am new to cockroach. This however still makes it non-trivial in terms of needing to detect the index with foreign key situation and refactor the statements so it can be executed in one changeset.

@ruzkant
Copy link
Author

ruzkant commented Apr 10, 2018

I am glad to know the additional syntax though, thx

@knz
Copy link
Contributor

knz commented Apr 10, 2018

@ruzkant You can use SELECT version() to differentiate between pg and CockroachDB.

cc @dt @jordanlewis @vivekmenezes I think there's a possible workaround: if a CREATE INDEX is issued in the same txn as CREATE TABLE, we could modify the descriptor in-place instead of queuing a schema change. (same for other mutations possibly) What do you think?

@knz
Copy link
Contributor

knz commented Apr 10, 2018

cc @awoods187 for roadmapping

@ruzkant
Copy link
Author

ruzkant commented Apr 10, 2018

@knz Thanks for the query...

@vivekmenezes
Copy link
Contributor

@knz agreed we should not need to run schema changers on tables when they are created in the same transaction.

@ruzkant
Copy link
Author

ruzkant commented Apr 10, 2018

@knz @vivekmenezes Is there any ideal platform for discussing project intent regarding the jdbc driver support? For example I was contemplating submitting a pull request on the postgres jdbc driver to return the correct product name, but this will have far reaching consequences for existing guys relying on the postgres name.

  /**
   * Retrieves the name of this database product. We hope that it is PostgreSQL, so we return that
   * explicitly.
   *
   * @return "PostgreSQL"
   */
  @Override
  public String getDatabaseProductName() throws SQLException {
    return "PostgreSQL";
  }

On startup it receives this data:

    } else if ("server_version_num".equals(name)) {
      setServerVersionNum(Integer.parseInt(value));

which could be extended with:

    } else if ("crdb_version".equals(name)) {
      setXXX

Then the postgres driver itself doesn't have to hardcode that anymore either. Technically I guess they could choose to do that any day irrespective of any discussions here and then break a number of client tools which have been built to rely on Postgresql product name.

@knz
Copy link
Contributor

knz commented Apr 10, 2018

break a number of client tools which have been built to rely on Postgresql product name.

yes that would be my concern as well.

@ruzkant
Copy link
Author

ruzkant commented Apr 13, 2018

When creating a table with a foreign key in the create statement an implicit index is created but not with explicit alter. I am not sure if this inconsistency has a good reason, but otherwise it might be useful for the later statement to also create an implicit index if not present? I thought it might relate to this item.

@knz
Copy link
Contributor

knz commented Apr 13, 2018

@ruzkant thanks for your feedback. The reason why ALTER does not add an index is that adding an index can be a very costly operation if the table already has many rows. This concern does not exist during CREATE, since the table is still empty at that time.

@knz
Copy link
Contributor

knz commented Apr 23, 2018

@vivekmenezes moving this to the bulk io project. Needs to be triaged.

@knz knz added O-community Originated from the community C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed O-community-questions labels Apr 24, 2018
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 16, 2018
This change allows execution of all schema changes following
a CREATE TABLE in a transaction. Since the CREATE TABLE is
invisible during the transaction to the rest of the cluster,
the schema changes can executed without changing the version
of the table descriptor and without running through the
schema change state machine. This change is being done
to make cockroachdb more compatible with postgres.

fixes cockroachdb#24626

Release note (sql change): Can follow a CREATE TABLE with schema
changes like CREATE INDEX on the table in the same transaction.

Release note: None
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 17, 2018
This change allows execution of all schema changes following
a CREATE TABLE in a transaction. Since the CREATE TABLE is
invisible during the transaction to the rest of the cluster,
the schema changes can executed without changing the version
of the table descriptor and without running through the
schema change state machine. This change is being done
to make cockroachdb more compatible with postgres.

fixes cockroachdb#24626

Release note (sql change): Can follow a CREATE TABLE with schema
changes like CREATE INDEX on the table in the same transaction.

Release note: None
@knz
Copy link
Contributor

knz commented May 18, 2018

I did some QA on the fix for this and I ran into this: #25663.

@vivekmenezes vivekmenezes reopened this May 21, 2018
@vivekmenezes
Copy link
Contributor

reopening this issue because I found some corner cases regarding FKs and interleaved tables still not covered.

vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 22, 2018
Normally a table with references (FK, interleaved) is
placed in the ADD state to allow the referenced tables
to be upgraded with backreferences across the cluster,
before the cluster sees the ADD table in the PUBLIC state.
When all the table references are created in the same
transaction the table can be made PUBLIC immediately
because all the references are still not visible to
the cluster. This is particularly valuable when commands
within a transaction need to use the PUBLIC table and
will normally barf on seeing a table in the ADD state.

This also fixes a problem with column backfill in the
presence of FKs.

fixes cockroachdb#24626

Release note (sql fix): Fix problems with using tables
created in the same transaction when the tables have FOREIGN
KEY or INTERLEAVED references to other tables which have also
been created in the same transaction.
craig bot pushed a commit that referenced this issue May 22, 2018
25786: sql: make table PUBLIC when references created in transaction r=vivekmenezes a=vivekmenezes

Normally a table with references (FK, interleaved) is
placed in the ADD state to allow the referenced tables
to be upgraded with backreferences across the cluster,
before the cluster sees the ADD table in the PUBLIC state.
When all the table references are created in the same
transaction the table can be made PUBLIC immediately
because all the references are still not visible to
the cluster. This is particularly valuable when commands
within a transaction need to use the PUBLIC table and
will normally barf on seeing a table in the ADD state.

This also fixes a problem with column backfill in the
presence of FKs.

fixes #24626

Release note (sql fix): Fix problems with using tables
created in the same transaction when the tables have FOREIGN
KEY or INTERLEAVED references to other tables which have also
been created in the same transaction.

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
@craig craig bot closed this as completed in #25786 May 22, 2018
@sandstrom
Copy link

sandstrom commented May 9, 2019

Adding a comment here to aid GoogleBot and people searching for this. I was trying to find the relevant issue for transactional DDL statements (i.e. in Cockroach DDL statements are still non-transactional).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-known-limitation docs-todo O-community Originated from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants