Fix Caching won't work with replication enabled

[madeindjs]

Fix #5919

Description of change

I just added a new method QueryRunner.getReplicationMode to check if we can insert cache into DbQueryResultCache.storeInCache. If query runner is in slave mode, we create a new one in master mode

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change N/A
• The new commits follow conventions explained in CONTRIBUTING.md

[imnotjames]

Adding extra API surface area & exposing more internal details kinda seems.. well, a bit over-kill for a feature that shouldn't be so tightly wound up in TypeORM anyway.

I think it'd be reasonable for DbQueryResultCache to just create a new query runner instead. For most databases this means pulling another query runner off a pool. Just need to confirm it resolves the issue.

What do you think?

[madeindjs]

@imnotjames , to be sure to have understood correctly, you propose:

1. remove BaseQueryRunner.getReplicationMode that I added
2. in DbQueryResultCache.storeInCache, remove queryRunner options and always create a new queryRunner inside

[imnotjames]

@imnotjames , to be sure to have understood correctly, you propose:

1. remove BaseQueryRunner.getReplicationMode that I added
2. in DbQueryResultCache.storeInCache, remove queryRunner options and always create a new queryRunner inside

Yes. Would that cause much overhead to create a new query runner, you think?

I feel like it's the most consistent. You'd just need to release the query runner afterwards in a finally block.

[madeindjs]

I do not think this is a good idea because:

1. queryRunneris passed everywhere. If I remove it from storeInCache, I should do it also on theses methods to be consitent

export interface QueryResultCache {
    connect(): Promise<void>;
    disconnect(): Promise<void>;
-   synchronize(queryRunner?: QueryRunner): Promise<void>;
+   synchronize(): Promise<void>;
    getFromCache(options: QueryResultCacheOptions, queryRunner?: QueryRunner): Promise<QueryResultCacheOptions|undefined>;
-   storeInCache(options: QueryResultCacheOptions, savedCache: QueryResultCacheOptions|undefined, queryRunner?: QueryRunner): Promise<void>;
+   storeInCache(options: QueryResultCacheOptions, savedCache: QueryResultCacheOptions|undefined): Promise<void>;
    isExpired(savedCache: QueryResultCacheOptions): boolean;
-   clear(queryRunner?: QueryRunner): Promise<void>;
+   clear(): Promise<void>;
-   remove(identifiers: string[], queryRunner?: QueryRunner): Promise<void>;
+   remove(identifiers: string[]): Promise<void>;
}

2. if we pass a queryRunner from a transaction, this will imply that cache will be stored before transaction commit. Even if we use a master queryRunner

What do you think about it?

[imnotjames]

if we pass a queryRunner from a transaction, this will imply that cache will be stored before transaction commit. Even if we use a master queryRunner

This is happening anyway with the code you're suggesting in the case of using a replica.

I almost think we shouldn't fix this at all is that's a concern and instead cause DBResultCache to bail out if you're trying to use it with any sort of replication enabled.

[madeindjs]

This is happening anyway with the code you're suggesting in the case of using a replica.

Not necessary if the user use a querRunner from a transaction on master server.

instead cause DBResultCache to bail out if you're trying to use it with any sort of replication enabled.

I don't think so. Currently, I have an application on a production environment I want to scale with Postgres replication but I want to keep the cache system.

But if you ok with modifications I proposed on QueryResultCache, I can give it a try and update this PR. What do you think about it?

[imnotjames]

This is happening anyway with the code you're suggesting in the case of using a replica.

Not necessary if the user use a querRunner from a transaction on master server.

Which is why I said in the case of using a replica which is 99% of the time when you're caching. SELECT uses a replica by default.

Oh, but you mean that most times you use a transaction it's gonna be on the primary? That makes sense.

Even still, abusing transaction mechanics for cache invalidation feels a bit weird.

instead cause DBResultCache to bail out if you're trying to use it with any sort of replication enabled.

I don't think so. Currently, I have an application on a production environment I want to scale with Postgres replication but I want to keep the cache system.

You would be really better off building your own caching mechanism if you need specific code paths.

Also, if you're selecting in the middle of a transaction you maybe shouldn't be caching that value - because then you'd be getting the data from before your operations. The whole point would be that you know changes have been made.

But if you ok with modifications I proposed on QueryResultCache, I can give it a try and update this PR. What do you think about it?

I think the optimal solution would be to omit query runner's use in the built-in implementation and mark those field as deprecated in the interface but still pass them to the interface.

That would give folks time to migrate away from it for their own implementation.

Then at a later date we can remove these. Otherwise everyone gets very grumpy. 😄 I mean, understandably.

[pleerock]

Okay so here I think you both have valid points @madeindjs and @imnotjames just different opinions.

If you ask me,

• I'm okay with new getReplicationMode() function, it doesn't bring something complex and logically makes sense to be part of QueryRunner

• I'm okay with proposed solution on using given query runner or create new one if needed.

here @imnotjames is right because in scaled app most of times queries will be served by read replicas and having this check doesn't make much sense. However, in specific user scenarios, when for example he opens complex transaction with saves and queries, it will be better to keep using given query runner in the case if transaction fail (e.g. data was inserted, "finds" executed, "cached" in a separate connection, but then transaction fails and we have no data inserted, but we do have data in the cache).

• if user wants to use db cache for production, that's completely fine. Yes maybe it's not the most performant solution, but its the most performant from simplest solutions. And since we have such solution, and this bug makes this solution unusable - I think we need to fix it.

I'm good with this PR, we just need to fix conflicts.
Sorry for late review.

[madeindjs]

Great @pleerock . I currently use this branch on production since june. So I'll try to correct conflicts as soon as possible.

[madeindjs]

@pleerock , I just resolved conflicts.

[pleerock]

Thank you for contribution! 🎉