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

DB-12303 Fix corrupted trigger after NSDS DML: DB-6516 regression #5694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msirek
Copy link
Contributor

@msirek msirek commented Jul 19, 2021

Short Description

A DML statement run in NSDS which fires a trigger may corrupt data dictionary dependencies. Subsequent DDL statements, such as TRUNCATE involving trigger tables corrupts the trigger, which may cause trigger rows to be written to the wrong table and/or be written incorrectly.

Long Description

Whenever a trigger needs recompilation due to some dictionary modification which the trigger may depend on, it is marked as invalid in the sys.sysstatements system table. For example, if the target table of the trigger is altered or another trigger is added to the same target table, the previous trigger is marked invalid. The next statement which fires the trigger causes recompilation of the trigger. First BasicDependencyManager.clearDependencies is called to remove the row in sys.sysdepends for the old version of the trigger SPSDecriptor. Then GenericTriggerExecutor.compile is called to build the new trigger SPSDecriptor and store a persistent representation of it in sys.sysstatements. Then BasicDependencyManager.addDependency is called to add the UUID of the new SPSDecriptor in sys.sysdepends, so that future DDLs know which stored statement to invalidate or clear.

The problem is that DB-6516 added a check in the addDependency method, called canUseDependencyManager, to skip adding the dependency if running NSDS. This causes any future DDL statements to not detect the trigger dependency and not recompile the trigger. When a truncate is done on the table, this should invalidate the trigger because the new table will have a new conglomerate number, but the old trigger statement still refers to the old conglomerate. When the trigger is later fired, it writes rows into a defunct table. This shows up as rows simply not appearing in the proper target table, as they are supposed to, or sometimes we hit a duplicate primary key violation, because the old table, invisible to the user, still exists in HBase, and still has rows in it (if Vacuum has not been run).

DB-6516 was trying to avoid memory leaks in the in-memory dependency manager because anything addDependency adds to the DependencyManager in NSDS was not getting released. However, if both the provider (TableDescriptor) and the dependent (SPSDescriptor) are persistent, then addDependency writes to the sys.sysdepends table and not the in-memory DependencyManager. We never want to skip writing persistent dependencies, so, the fix is to have canUseDependencyManager return true when the dependency is to be written to disk, no matter if we are running on NSDS or not.

How to test

CREATE TABLE ECOMMERCE_FS.TEST (
CUSTOMER_ID INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1)
,LAST_UPDATE_TS TIMESTAMP NOT NULL
,FEATURE_1 INTEGER
,FEATURE_2 INTEGER
, PRIMARY KEY(CUSTOMER_ID)) ;

CREATE TABLE ECOMMERCE_FS.TEST_HISTORY (
CUSTOMER_ID INTEGER NOT NULL
,ASOF_TS TIMESTAMP NOT NULL
,INGEST_TS TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
,FEATURE_1 INTEGER
,FEATURE_2 INTEGER
, CONSTRAINT SQLA6D10249017A67F3B7620004DDAD2420 PRIMARY KEY(CUSTOMER_ID,ASOF_TS));

CREATE TRIGGER TEST_HISTORY_INSERT AFTER INSERT ON ECOMMERCE_FS.TEST REFERENCING NEW_TABLE AS NEWW FOR EACH STATEMENT
  INSERT INTO ECOMMERCE_FS.TEST_history (ASOF_TS, INGEST_TS, CUSTOMER_ID, feature_1,feature_2)
        SELECT NEWW.LAST_UPDATE_TS, CURRENT_TIMESTAMP, NEWW.CUSTOMER_ID, NEWW.feature_1,NEWW.feature_2 FROM NEWW;

CREATE TRIGGER TEST_HISTORY_UPDATE AFTER  UPDATE ON ECOMMERCE_FS.TEST REFERENCING NEW_TABLE AS NEWW FOR EACH STATEMENT INSERT INTO ECOMMERCE_FS.TEST_history (ASOF_TS, INGEST_TS, CUSTOMER_ID, feature_1,feature_2)
        SELECT NEWW.LAST_UPDATE_TS, CURRENT_TIMESTAMP, NEWW.CUSTOMER_ID, NEWW.feature_1,NEWW.feature_2 FROM NEWW;

set schema splice;
drop table if exists test;

create table test(
    customer_id int,
    LAST_UPDATE_TS timestamp,
    feature_1 int,
    feature_2 int);

insert into test values(1, '2020-01-01 00:00:00', 1,1);
insert into test values(2, '2020-01-01 00:00:00', 1,1);

/* Now, in NSDS, run the following: */
import com.splicemachine.derby.utils._
import com.splicemachine.spark.splicemachine._
import com.splicemachine.derby.impl.SpliceSpark
val spliceJDBC = "jdbc:splice://localhost:1527/splicedb;user=splice;password=admin"
val spc = new SplicemachineContext(spliceJDBC)
val df = spc.df("select * from test")
spc.insert(df, "ecommerce_fs.test")
 
/* Now, back in sqlshell, run the following, which should not hit a duplicate PK constraint violation: */
truncate table ecommerce_fs.test;
truncate table ECOMMERCE_FS.TEST_history;
insert into ECOMMERCE_FS.TEST values(1, '2020-01-01 00:00:00', 1,1);
insert into ECOMMERCE_FS.TEST values(2, '2020-01-01 00:00:00', 1,1);

@msirek
Copy link
Contributor Author

msirek commented Jul 20, 2021

jenkins please test branch @dbaas3.0

@cloudspliceci
Copy link
Contributor

@carolp-503
Copy link

@msirek , this was a great description of the issue and the fix. Thanks for taking the time to clearly explain it!

@msirek
Copy link
Contributor Author

msirek commented Jul 20, 2021

jenkins please test branch @dbaas3.0

Copy link
Contributor

@ascend1 ascend1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix!

@cloudspliceci
Copy link
Contributor

@msirek
Copy link
Contributor Author

msirek commented Jul 20, 2021

jenkins please test branch @dbaas3.0

Copy link

@carolp-503 carolp-503 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@cloudspliceci
Copy link
Contributor

@msirek
Copy link
Contributor Author

msirek commented Jul 21, 2021

jenkins please test branch @dbaas3.0

@cloudspliceci
Copy link
Contributor

@msirek
Copy link
Contributor Author

msirek commented Jul 21, 2021

jenkins please test branch @dbaas3.0

@cloudspliceci
Copy link
Contributor

TEST SUCCEEDED +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants