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: prepared statements can fail to be rejected by foreign key checks #68307

Closed
timchunght opened this issue Aug 1, 2021 · 8 comments · Fixed by #68486
Closed

sql: prepared statements can fail to be rejected by foreign key checks #68307

timchunght opened this issue Aug 1, 2021 · 8 comments · Fixed by #68486
Assignees
Labels
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-queries SQL Queries Team X-blathers-triaged blathers was able to find an owner

Comments

@timchunght
Copy link

timchunght commented Aug 1, 2021

A foreign key constraint enforced in Cockroach sql but does NOT get enforced when insert is done via Gorm.

To Reproduce
The following code in Go using Gorm succeeds despite the foreign key is violated within the parent_account_id column of the transactions table (parent_account_id REFERENCES accounts.id)

dbResult := self.DBSession.
		Table(constants.TransactionTable).
		Create(transaction)

The above Gorm method results in an INSERT of a row within the transactions table despite the FK violation.

=# select id, parent_account_id from transactions;
                  id                  |          parent_account_id
--------------------------------------+--------------------------------------
d3f2bf36-8072-423d-a75e-04aff2af1209 | deposit-acct-4

The gorm code generates the following SQL

INSERT INTO "transactions" ("id","organization_id","external_id","payment_order_id","notes","parent_account_id","type","amount","currency_code","funds_amount","interest_amount","fees_amount","overdraft_amount","overdraft_fees_amount","overdraft_interest_amount","technical_overdraft_amount","technical_overdraft_interest_amount","fraction_amount","total_balance","user_id","payment_channel_id","linked_transaction_id","linked_transaction_type","is_originator","card_transaction_id","value_date","booking_date","custom_data","created_at","updated_at") VALUES ('d3f2bf36-8072-423d-a75e-04aff2af1209','62cd5f10-8b5a-4646-8aa9-98f9f9bc5db5',NULL,'','','deposit-acct-4','deposit','1000','USD','1000','0','0','0','0','0','0','0','0','3399.7536',NULL,NULL,NULL,NULL,false,NULL,'2021-07-31 23:41:04.496','2021-07-31 23:41:04.496','null',1627789264,1627789264);

This SQL when pasted into cockroach sql results in the expected FK constraint error

ERROR: insert on table "transactions" violates foreign key constraint "fk_parent_account_id_ref_accounts"
SQLSTATE: 23503
DETAIL: Key (parent_account_id)=('deposit-acct-4') is not present in table "accounts".

Suspicion:

Gorm seems to use the parameters API in the postgres binary protocol that generates the following log when used against postgres that Cockroach might not enforce constraints when used. Please advise.

LOG:  statement: begin
LOG:  execute lrupsc_1_2: INSERT INTO "transactions" ("id","organization_id","external_id","payment_order_id","notes","parent_account_id","type","amount","currency_code","funds_amount","interest_amount","fees_amount","overdraft_amount","overdraft_fees_amount","overdraft_interest_amount","technical_overdraft_amount","technical_overdraft_interest_amount","fraction_amount","total_balance","user_id","payment_channel_id","linked_transaction_id","linked_transaction_type","is_originator","card_transaction_id","value_date","booking_date","custom_data","created_at","updated_at") VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29,$30)
DETAIL:  parameters: $1 = 'aab6d5a6-b119-45eb-98d6-0a77a215045a', $2 = 'a9d5ae61-29bb-4c64-808c-08f9196b1995', $3 = NULL, $4 = '', $5 = '', $6 = 'd3784d5a-88ae-471b-bd04-3ecc7ca8aced', $7 = 'deposit', $8 = '1000', $9 = 'USD', $10 = '1000', $11 = '0', $12 = '0', $13 = '0', $14 = '0', $15 = '0', $16 = '0', $17 = '0', $18 = '0', $19 = '4868.89', $20 = NULL, $21 = NULL, $22 = NULL, $23 = NULL, $24 = 'f', $25 = NULL, $26 = '2021-08-01', $27 = '2021-08-01', $28 = 'null', $29 = '1627796187', $30 = '1627796187'
LOG:  statement: commit

Expected behavior

A foreign key violation error is expected to happen.

Environment:

  • CockroachDB version v20
  • Server OS: Mac
  • Client app: cockroach sql, Gorm
@timchunght timchunght added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 1, 2021
@blathers-crl
Copy link

blathers-crl bot commented Aug 1, 2021

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

It looks like you have not filled out the issue in the format of any of our templates. To best assist you, we advise you to use one of these templates.

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

  • @cockroachdb/sql-experience (found keywords: Gorm)
  • @cockroachdb/kv (found keywords: raft)

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 otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Aug 1, 2021
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Aug 1, 2021
@otan
Copy link
Contributor

otan commented Aug 1, 2021

Hey @timchunght --

Can you provide the following:

  • Schemas of the tables you are inserting into (use SHOW CREATE TABLE <table_name>) showing the FK relationships
  • if available, a small repro we can run using GORM that can reproduce this error.

@timchunght
Copy link
Author

timchunght commented Aug 2, 2021

@otan Here is the result of the SHOW CREATE TABLE. I will need some time to extract the code from our repo into a small isolated repo for testing. But essentially we are just using the default .Create from GORM (docs here https://gorm.io/docs/create.html#content-inner) and the postgres setting.

Background:
We are in the process of making our backend fully compatible with cockroachdb (we currently run on Postgres). During a code change, we were inserting data into transactions.parent_account_id that is invalid but still succeeded (as described above) where the raw query failed but the Gorm's .Create call succeeded. This is quite bewildering indeed.

 SHOW CREATE TABLE transactions;
   table_name  |                                                                                                                                                                                                                                                                                                create_statement
---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  transactions | CREATE TABLE public.transactions (
               |     id VARCHAR(36) NOT NULL,
               |     organization_id VARCHAR(36) NULL,
               |     external_id VARCHAR(255) NULL,
               |     payment_order_id VARCHAR(255) NULL,
               |     parent_account_id VARCHAR(36) NULL,
               |     payment_channel_id VARCHAR(36) NULL,
               |     type VARCHAR(255) NULL,
               |     amount DECIMAL(50,10) NULL,
               |     currency_code VARCHAR(32) NULL,
               |     funds_amount DECIMAL(50,10) NULL,
               |     interest_amount DECIMAL(50,10) NULL,
               |     fees_amount DECIMAL(50,10) NULL,
               |     overdraft_amount DECIMAL(50,10) NULL,
               |     overdraft_fees_amount DECIMAL(50,10) NULL,
               |     overdraft_interest_amount DECIMAL(50,10) NULL,
               |     technical_overdraft_amount DECIMAL(50,10) NULL,
               |     technical_overdraft_interest_amount DECIMAL(50,10) NULL,
               |     fraction_amount DECIMAL(50,10) NULL,
               |     taxes JSONB NULL,
               |     account_balances JSONB NULL,
               |     user_id VARCHAR(36) NULL,
               |     terms JSONB NULL,
               |     transaction_details JSONB NULL,
               |     transfer_details JSONB NULL,
               |     linked_transaction_id VARCHAR(36) NULL,
               |     linked_transaction_type VARCHAR(255) NULL,
               |     is_originator BOOL NULL DEFAULT false,
               |     fees JSONB NULL,
               |     notes STRING NULL,
               |     custom_data JSONB NULL,
               |     value_date DATE NULL,
               |     booking_date DATE NULL,
               |     created_at INT8 NULL,
               |     updated_at INT8 NULL,
               |     card_transaction_id VARCHAR(36) NULL,
               |     total_balance DECIMAL(50,10) NULL,
               |     CONSTRAINT "primary" PRIMARY KEY (id ASC),
               |     CONSTRAINT fk_payment_channel_id_ref_payment_channels FOREIGN KEY (payment_channel_id) REFERENCES public.payment_channels(id),
               |     CONSTRAINT fk_organization_id_ref_organizations FOREIGN KEY (organization_id) REFERENCES public.organizations(id),
               |     CONSTRAINT fk_parent_account_id_ref_accounts FOREIGN KEY (parent_account_id) REFERENCES public.accounts(id),
               |     CONSTRAINT fk_user_id_ref_users FOREIGN KEY (user_id) REFERENCES public.users(id),
               |     CONSTRAINT fk_card_transaction_id_ref_card_transactions FOREIGN KEY (card_transaction_id) REFERENCES public.card_transactions(id),
               |     UNIQUE INDEX transactions_organization_id_external_id_key (organization_id ASC, external_id ASC),
               |     FAMILY "primary" (id, organization_id, external_id, payment_order_id, parent_account_id, payment_channel_id, type, amount, currency_code, funds_amount, interest_amount, fees_amount, overdraft_amount, overdraft_fees_amount, overdraft_interest_amount, technical_overdraft_amount, technical_overdraft_interest_amount, fraction_amount, taxes, account_balances, user_id, terms, transaction_details, transfer_details, linked_transaction_id, linked_transaction_type, is_originator, fees, notes, custom_data, value_date, booking_date, created_at, updated_at, card_transaction_id, total_balance)
               | )
(1 row)

Time: 49ms total (execution 49ms / network 0ms)

@timchunght
Copy link
Author

@otan Just put together a debug repo that should demonstrates the exact setup here https://github.com/timchunght/gorm-cockroachdb-fk-debug

I noticed that with another foreign key explicitly NULL, the invalid foreign key does not return error. However, if the OTHER foreign key is simply missing, the invalid foreign key does return error. This is very strange behaviour and took us a while to simply find.

@cucaroach cucaroach self-assigned this Aug 3, 2021
@rafiss
Copy link
Collaborator

rafiss commented Aug 5, 2021

@timchunght Thanks for this repro; it is extremely helpful!

It looks like there definitely is a bug here. It seems specific to prepared statements. I can reproduce this using the CLI like so:

root@:26257/defaultdb> create table t1(a int primary key);
CREATE TABLE

root@:26257/defaultdb> create table t2(a int primary key);
CREATE TABLE

root@:26257/defaultdb> create table t3(a int primary key, b int references t1(a), c int references t2(a));
CREATE TABLE

root@:26257/defaultdb> prepare q as insert into t3(a,b,c) values ($1, $2, $3);
PREPARE

root@:26257/defaultdb> execute q(1,1,null);  -- this fails, which is correct
ERROR: insert on table "t3" violates foreign key constraint "fk_b_ref_t1"
SQLSTATE: 23503
DETAIL: Key (b)=(1) is not present in table "t1".
CONSTRAINT: fk_b_ref_t1

root@:26257/defaultdb> execute q(1,null,1);   -- this should fail!
INSERT 1

it seems like might relate to the order that the constraints are checked.

@rafiss
Copy link
Collaborator

rafiss commented Aug 5, 2021

This bug affects both v20.2 and v21.1 (as well as the current master branch HEAD).

@jordanlewis jordanlewis changed the title CockroachDB Gorm Create/Insert does not enforce foreign key constraints sql: prepared statements can fail to be rejected by foreign key checks Aug 5, 2021
@RaduBerinde RaduBerinde assigned RaduBerinde and unassigned cucaroach Aug 5, 2021
@RaduBerinde
Copy link
Member

@RaduBerinde
Copy link
Member

So the problem is in the insert fast path - as soon as we find one FK that doesn't need to be checked because of NULL, we don't run any subsequent checks on that node. In this one row case, it only happens with prepare because otherwise we elide that FK check entirely at build time.

Here's an example without prepare:

[email protected]:26257/defaultdb> insert into t1 values (1);
[email protected]:26257/defaultdb> insert into t2 values (1);
[email protected]:26257/defaultdb> insert into t3 values (1,1,1), (2,null,2);
INSERT 2

craig bot pushed a commit that referenced this issue Aug 5, 2021
68429: [CRDB-8149] Add CES survey link r=Santamaura a=Santamaura

The goal of this PR is to create a basic component that can open a link to the CES feedback survey. The current query parameters that are passed through are `clusterId` and `clusterVersion`. If there are any more query parameters that need to be included feel free to comment what they might be. The component will render empty (not able to open the survey) if for some reason we are unable to get the clusterId or clusterVersion values.

Below are some screenshots showcasing the feedback survey link and what the url looks like when opened in a local dev environment:

![image](https://user-images.githubusercontent.com/17861665/128223481-2e6b5e6d-77de-4e3e-8a8e-125171f42500.png)


![image](https://user-images.githubusercontent.com/17861665/128223593-c0350d3e-0104-4e1a-ab73-4f4baa939554.png)

This PR is in response to [this github issue](#66615)

68486: sql: fix accidental FK check skips r=RaduBerinde a=RaduBerinde

This change fixes a bug in the insert fast path where we accidentally
skip subsequent FK checks when a FK check can be skipped due to NULL
value.

Note that this bug does not manifest when optbuilder can determine
that the check can be elided entirely; notably, this is always the
case for non-prepared statements which insert a single row.

Fixes #68307.

Release note (bug fix): fixed missing foreign key checks in some cases
when there are multiple checks and the inserted data contains a NULL
for one of the checks.

Co-authored-by: Santamaura <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in 3eb6fb2 Aug 5, 2021
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Aug 5, 2021
This change fixes a bug in the insert fast path where we accidentally
skip subsequent FK checks when a FK check can be skipped due to NULL
value.

Note that this bug does not manifest when optbuilder can determine
that the check can be elided entirely; notably, this is always the
case for non-prepared statements which insert a single row.

Fixes cockroachdb#68307.

Release note (bug fix): fixed missing foreign key checks in some cases
when there are multiple checks and the inserted data contains a NULL
for one of the checks.
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Aug 5, 2021
This change fixes a bug in the insert fast path where we accidentally
skip subsequent FK checks when a FK check can be skipped due to NULL
value.

Note that this bug does not manifest when optbuilder can determine
that the check can be elided entirely; notably, this is always the
case for non-prepared statements which insert a single row.

Fixes cockroachdb#68307.

Release note (bug fix): fixed missing foreign key checks in some cases
when there are multiple checks and the inserted data contains a NULL
for one of the checks.
sajjadrizvi pushed a commit to sajjadrizvi/cockroach that referenced this issue Aug 10, 2021
This change fixes a bug in the insert fast path where we accidentally
skip subsequent FK checks when a FK check can be skipped due to NULL
value.

Note that this bug does not manifest when optbuilder can determine
that the check can be elided entirely; notably, this is always the
case for non-prepared statements which insert a single row.

Fixes cockroachdb#68307.

Release note (bug fix): fixed missing foreign key checks in some cases
when there are multiple checks and the inserted data contains a NULL
for one of the checks.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
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. O-community Originated from the community T-sql-queries SQL Queries Team X-blathers-triaged blathers was able to find an owner
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants