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

Feature request: create delete query using where array #130

Open
engahmeds3ed opened this issue Feb 28, 2022 · 4 comments
Open

Feature request: create delete query using where array #130

engahmeds3ed opened this issue Feb 28, 2022 · 4 comments

Comments

@engahmeds3ed
Copy link

We only have one query method to delete item by ID here
https://github.com/berlindb/core/blob/master/src/Database/Query.php#L1995

we need a new method there to delete using where array so we may delete multiple rows with just one query.

If agreed, I can make a small PR to be reviewed by tomorrow.
Thanks.

@JJJ
Copy link
Collaborator

JJJ commented Feb 28, 2022

Related: #18.

I'm for this in general, but as usual, there are some caveats & conditions.

I imagine that a future delete_items() method would simply wrap an uncached get_items() call, then loop through the results and call delete_item() on them.

This is because WordPress core shies away from "multi" types of database queries, doing every transaction one at a time, and BerlinDB is built similarly.

It ensures that all hooks happen as anticipated. When a Row is deleted, action hooks fire. When deleting many Rows, doing them one at a time ensures all of the hooks fire in the order Rows are deleted, avoiding race conditions, etc...

That should also ensure that Meta Data relative to the Row is deleted at the same time. And also cleans the relative caches.

It also avoids a situation where a "delete many" MySQL query could lock a table or cause MySQL to run out of memory or timeout (by potentially deleting hundreds or thousands of Rows) causing a cascade into PHP where the above additional actions or hooks would never happen, leaving behind an unintentionally disconnected set of data.

Does that make sense? Any thoughts/opinions/ideas?

@engahmeds3ed
Copy link
Author

engahmeds3ed commented Mar 7, 2022

Thanks @JJJ for this detailed comment and sorry for late reply here.

After checking your comment and the code here:

core/src/Database/Query.php

Lines 2035 to 2036 in 2d94af5

$this->delete_all_item_meta( $item_id );
$this->clean_item_cache( $item );

I think there is no way to do this using direct query (the way that I was thinking of before your comment) to delete.

but I'm wondering, if there are too many items to be deleted, this may timeout as the maximum execution time is reached?

I think we can provide the user ability to change the limit to control the operation there, what do u think?

Thanks again for your time.

@engahmeds3ed
Copy link
Author

@JJJ when we truncate the table we don't touch the meta and cache, right?

@JJJ
Copy link
Collaborator

JJJ commented May 25, 2022

@JJJ when we truncate the table we don't touch the meta and cache, right?

Correct.

Cache: I'm uncertain how to feel about flushing the cache when certain large table operations occur. Probably is a good idea? Might be an expensive operation. Might not be possible to purge a single cache group, unfortunately.

Meta: Meta tables are currently more clunky than they should be. You still need to define/register them like a normal table, but certain operations like Meta-Queries work pretty invisibly. (In the future, I'd like for Meta tables to simply be an attribute of the Object table – like "do you want meta: yes|no" via a Table::metatable flag, or something similar.) That means it currently needs to be manually truncated along with it's sibling Object table.

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

No branches or pull requests

2 participants