Skip to content
/ server Public

Conversation

@Simoffsky
Copy link

@Simoffsky Simoffsky commented Sep 3, 2024

  • The Jira issue number for this PR is: MDEV-12302

Description

Roadmap:
✅ Simple Delete
✅ Simple Update
✅ Fetch data from secondary indexes
✅ Handle changes coming from BEFORE triggers
✅ Virtual fields
✅ Test ON DELETE/UPDATE SET NULL
✅ Bit fields (probably just test it)
✅ Long blobs

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@CLAassistant
Copy link

CLAassistant commented Sep 3, 2024

CLA assistant check
All committers have signed the CLA.

@FooBarrior FooBarrior self-requested a review October 11, 2024 13:27
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 2 times, most recently from 954b0e6 to bfda058 Compare October 12, 2024 12:57
@FooBarrior
Copy link
Contributor

FooBarrior commented Nov 4, 2024

Hello @Simoffsky! As the project is close to its final stage, it's time to reorganize your code, also covering @vuvova's request from #3354.

Please move the triggers processing (and other sql-layer "hooks") code to the sql layer.

Let's consider two approaches to this.

1. Make two sql-side functions related to hooks: before and after

I will refer to hooks as to ephemere trigger like actions, that are to be done before or after a row change.

We can already observe that before hooks differ from after hooks: before hooks also include updating virtual fields, whereas after hooks don't. In the close future, the list will expand (like with row logging, CHEK constraints, etc).

So it makes sense for making two functions.

This approach would look as placing two functions in sql_class.cc:

int sql_do_before_triggers(TABLE*, bool is_delete);
int sql_do_after_triggers(TABLE*, bool is_delete);

This also could make sense to be done in four functions instead of two.

Then, in ha_innodb.cc, adding an sql-format row conversion function, which also retreives the necessary sql-layer objects, like maria_table:

dberr_t innodb_convert_ib_row_to_sql(dict_index_t *, row_t *,..., TABLE **maria_table /*!< out */,  ...);

This approach also requires a manual conversion back from sql format (in case if triggers change the data)

dberr_t innodb_convert_sql_row_to_ib(...);

2. Call an sql-layer function instead of innodb's row_upd.

We can instead make a whole row processing on the sql side:
sql_table.cc would be a suitable place for the following two functions:

sql_update_row(TABLE *);
sql_delete_row(TABLE *);

One will have to call table->file->ha_update_row (or ha_delete_row) inside.

Also positioning the cursor with table->file->ha_rnd_pos is mandatory before any ha_update_row and ha_delete_row calls.

In ha_innodb.cc, adding

dberr_t innodb_do_foreign_cascade(dict_table_t *, upd_node_t*,...);

and replace the row_upd_step call in row_update_cascade_for_mysql.

Which way to choose

1 preserves innodb row_upd_step call, which makes things faster, especially if no hooks are required to call.

OTOH, 2 gives a more flexibility on what the sql layer can do. ha_* will call some extra hooks, like long-text UNIQUE checks, and WITHOUT OVERLAPS feature. We can also replace ha_delete_row with TABLE::delete_row call, which can eventyally bring System Versioning support in the sql layer. This is actually very desired.

2 also needs no manual conversion backward, which can be less error-prone, and just easier.

sql_update_row and sql_delete_row wil allso be able to be eventually re-used Sql_cmd_{update|delete}, or in other commands, that have to make general-purpose row updates (like REPLACE or INSERT...ON DUPLICATE KEY UPDATE, or yet unimplemented MERGE)

So I'm asking you to go on with approach 2.

@FooBarrior
Copy link
Contributor

Please also join all tests in a single file. Test execution time is a problem for us, as every test run has a significant latency.

Note that we maintain 80 characters width for the c/c++ sode, with putting an opening { brace on a blank line.
Though https://github.com/MariaDB/server/blob/main/CODING_STANDARDS.md holds just a basic set of rules, it's better to read and follow it to pass a review with less edits.

@FooBarrior FooBarrior self-assigned this Nov 4, 2024
@cvicentiu cvicentiu added GSoC External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. labels Nov 26, 2024
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 2 times, most recently from b700b79 to 33545f7 Compare December 6, 2024 13:39
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 2 times, most recently from f2b8b9f to 1ca8660 Compare December 22, 2024 16:05
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 3 times, most recently from 42c3ab6 to 5709403 Compare December 30, 2024 12:48
@Simoffsky Simoffsky changed the base branch from 11.6 to main December 30, 2024 12:48
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 3 times, most recently from 9c53dcf to 11a22ef Compare March 2, 2025 15:48
@Simoffsky Simoffsky force-pushed the triggers-cascade branch 2 times, most recently from 37ddc47 to f1fe0e8 Compare March 30, 2025 15:25
@svoj svoj marked this pull request as draft July 12, 2025 06:51
@FooBarrior FooBarrior changed the title MDEV-12302: Execute triggers for foreign key updates/deletes [WIP] MDEV-12302: Execute triggers for foreign key updates/deletes Jan 27, 2026
@FooBarrior FooBarrior marked this pull request as ready for review January 27, 2026 17:24
Copy link
Contributor

@FooBarrior FooBarrior left a comment

Choose a reason for hiding this comment

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

@Simoffsky I'm going to have a time to focus on my leftovers, and this patch is of my high interest.

Let's go through it, perhaps, and see what could we miss?

row_sel_store_mysql_rec(maria_table->record[1], prebuilt, rec, NULL,
true, clust_index, offsets);

//if (node->upd_row == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you remember what was the problem with this line? Perhaps, just a comment not removed

thr->prebuilt->m_mysql_table = NULL;
row_upd_step(thr);
thr->prebuilt->m_mysql_table = mysql_table;
//row_upd_step(thr);
Copy link
Contributor

Choose a reason for hiding this comment

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

another quirk. what should we do with it?

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

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. GSoC

Development

Successfully merging this pull request may close these issues.

4 participants