Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Cache] AddPdoTagAwareAdapter#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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Toflar wants to merge18 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromToflar:feature/cache-pdo-tagaware

Conversation

Toflar
Copy link
Contributor

@ToflarToflar commentedSep 17, 2024•
edited
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
Issues
LicenseMIT

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

fritzmg and soyuka reacted with thumbs up emoji
@carsonbot

This comment was marked as outdated.

@ToflarToflar marked this pull request as ready for reviewSeptember 17, 2024 17:03
@carsonbotcarsonbot added this to the7.2 milestoneSep 17, 2024
@OskarStarkOskarStark changed the title[Cache] Make PdoAdapter tag aware[Cache] MakePdoAdapter tag awareSep 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
ContributorAuthor

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 generalTagAwareAdapter. Sonew TagAwareAdapter(new PdoAdapter()).
The performance problem in theTagAwareAdapter comes from the fact that it keeps versions of the cache tags and thus they have to be checked on every cache item lookup (seeTagAwareAdapter::getTagVersions()). So if you have a cache setup that works with a lot of cache invalidation, theTagAwareAdapter becomes rather unsuitable as for every cache lookup, it has to check quite a few tag versions. Adding native tagging support to thePdoAdapter 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 aPdoTrait?

@ToflarToflar changed the title[Cache] MakePdoAdapter tag aware[Cache] AddPdoTagAwareAdapterSep 18, 2024
@Toflar
Copy link
ContributorAuthor

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

{
$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 withON DELETE CASCADE instead ?

Copy link
ContributorAuthor

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
ContributorAuthor

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 howoci andsqlsrv 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.

@bendavies
Copy link
Contributor

bendavies commentedNov 25, 2024•
edited
Loading

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

@ToflarToflarforce-pushed thefeature/cache-pdo-tagaware branch fromcc64fd2 to49d24d1CompareNovember 25, 2024 18:09
@Toflar
Copy link
ContributorAuthor

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.

bendavies reacted with thumbs up emoji

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

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

Assignees
No one assigned
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
[8]ページ先頭
©2009-2025 Movatter.jp