berlindb / core

All of the required core code
MIT License
252 stars 27 forks source link

Feature request: create delete query using where array #130

Open engahmeds3ed opened 2 years ago

engahmeds3ed commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

After checking your comment and the code here: https://github.com/berlindb/core/blob/2d94af59a0d5642b659af7b62c7674811f25df00/src/Database/Query.php#L2035-L2036

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 commented 2 years ago

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

JJJ commented 2 years ago

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