Skip to content

[Cache] Add PdoTagAwareAdapter #58296

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 18 commits into
base: 7.4
Choose a base branch
from
Open

[Cache] Add PdoTagAwareAdapter #58296

wants to merge 18 commits into from

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Sep 17, 2024 •
edited
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

I noticed that unfortunately, the PdoAdapter in the cache component does not natively support cache tagging. This forces you to use it in combination with the general TagAwareAdapter which has a few downsides when it comes to performance. So I figured, why not try to add native support.

@carsonbot

This comment was marked as outdated.

Sign in to view
@Toflar Toflar marked this pull request as ready for review September 17, 2024 17:03
@carsonbot carsonbot added this to the 7.2 milestone Sep 17, 2024
@OskarStark OskarStark changed the title [Cache] Make PdoAdapter tag aware [Cache] Make PdoAdapter tag aware Sep 17, 2024
@nicolas-grekas
Copy link
Member

Thanks for looking into this topic.
I'm wondering about two things:

  • what's the perf improvement? aka is this providing measurable benefits?
  • shouldn't we turn this into a new adapter? Like we have Redis and RedisTagAware adapter?

@Toflar
Copy link
Contributor Author

Toflar commented Sep 18, 2024

what's the perf improvement? aka is this providing measurable benefits?

The problem as far as I could see (feel free to double-check and correct) is that the only alternative for this use case would be the general TagAwareAdapter. So new TagAwareAdapter(new PdoAdapter()).
The performance problem in the TagAwareAdapter comes from the fact that it keeps versions of the cache tags and thus they have to be checked on every cache item lookup (see TagAwareAdapter::getTagVersions()). So if you have a cache setup that works with a lot of cache invalidation, the TagAwareAdapter becomes rather unsuitable as for every cache lookup, it has to check quite a few tag versions. Adding native tagging support to the PdoAdapter can fix this.

shouldn't we turn this into a new adapter? Like we have Redis and RedisTagAware adapter?

Hmm, I guess we could. It would make the PR a bit more bloated because I guess we'd also follow the Redis schema and extract common logic into a PdoTrait?

@Toflar Toflar changed the title [Cache] Make PdoAdapter tag aware [Cache] Add PdoTagAwareAdapter Sep 18, 2024
@Toflar
Copy link
Contributor Author

Toflar commented Sep 18, 2024

@nicolas-grekas like this? Not sure how to best fix the remaining Psalm issue. It's the same in all other cache adapters as far as I could see but the baseline protects those. But yes, in theory $failed could be null.

stof
stof reviewed Sep 19, 2024
View reviewed changes
src/Symfony/Component/Cache/Adapter/PdoTagAwareAdapter.php
{
$conn = $this->getConnection();

$stmt = $conn->prepare("DELETE FROM $this->tagsTable WHERE $this->idCol NOT IN (SELECT $this->idCol FROM $this->table)");
Copy link
Member

Choose a reason for hiding this comment

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

should this adapter use a foreign key with ON DELETE CASCADE instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided against this on purpose. It's simpler like that 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

why is it simpler to require handling orphaned tags explicitly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if you think that is better, please provide the necessary changes. I have no clue how oci and sqlsrv work and no motivation to learn about them 😄 We can also just do it for the others and throw an exception for those ones and ask the community to finish the implementation. Whoever needs it. But the current query likely works on all platforms.

OskarStark
OskarStark reviewed Sep 19, 2024
View reviewed changes
OskarStark
OskarStark reviewed Sep 19, 2024
View reviewed changes
OskarStark
OskarStark reviewed Sep 19, 2024
View reviewed changes
@Toflar Toflar mentioned this pull request Sep 26, 2024
Open
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@bendavies
Copy link
Contributor

bendavies commented Nov 25, 2024 •
edited
Loading

was looking for this exact feature. thanks @Toflar.
what needs doing here?

@Toflar Toflar force-pushed the feature/cache-pdo-tagaware branch from cc64fd2 to 49d24d1 Compare November 25, 2024 18:09
@Toflar
Copy link
Contributor Author

Toflar commented Nov 25, 2024

Well I have incorporated the requested changes and then never heard back regarding the Psalm issue. I have rebased onto 7.3 now which is about all I can do I guess 😊 Failed 8.4 tests are unrelated.

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@OskarStark OskarStark OskarStark left review comments

Assignees
No one assigned
Labels
Cache Feature Status: Needs Review
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
7 participants
@Toflar @carsonbot @nicolas-grekas @bendavies @stof @OskarStark @fabpot