NEW Allow database read-only replicas

[emteknetnz]

Issue #11352

[emteknetnz]

I'm enforcing must_use_primary on anything "Security" related - i.e. any url that matches /Security, any DataObject under the Security namespace, to prevent any weird issues that could happen when a replica is out of sync with the primary

For instance a password is changed, the user logs out, but cannot log back in because the replica isn't synced up yet.

Having replicas be slightly out of sync is one of the trade-offs for using replicas, though for anything security related it introduces risk so it's disallowed

[GuySartorelli]

Haven't tested locally yet but here's some requested changes based on reviewing the code

[GuySartorelli]

We'll need to make sure this gets carried over to the new sake somehow

[emteknetnz]

Depending on which PR gets merged first, I'd expect it to create merge conflict in the 2nd PR as the cli-script.php was deleted

[GuySartorelli]

Fingers crossed whoever deals with the conflict things to check what changed and doesn't just go "it was deleted so we don't need it 😅

[GuySartorelli]

Suggested change

	return DB::$mustUsePrimary;
	return DB::$mustUsePrimary || DB::$withPrimaryCount > 0;

In both of these scenarios, the primary must be used. It doesn't matter why the primary must be used, just that it must.

[emteknetnz]

I'm not sure about this since it's named as a getting.

What I'll do instead is create a new method to 'willUsePrimary()' and add in the check for replica config, i.e. I'll extract the logic from the top of DB::getDefaultConnectionName()

[GuySartorelli]

Ahh okay. My intention was always for this to be the "tell me if I am using the primary" method, not the "Get that property's value" method.

Probably not worth having a getter just for this property on its own, that'll just be confusing I think especially when there's also a willUsePrimary.
IMO remove this getter

[emteknetnz]

Removed

[GuySartorelli]

Suggested change

	*
	* Prior to CMS 5.4 when replica connections were introduced, the 'default' connection
	* was not dynamic and instead represented what the 'primary' connection is now

We don't tend to keep historical information in PHPDocs - that's probably something for the changelog.

[emteknetnz]

I want to keep this one inline because 'default' is a poor way to describe 'either primary or replica if configured' i.e. it doesn't do what it sounds like it should do

[emteknetnz]

Have reworded a little now that I've changed the const to DB::CONN_DYNAMIC

[GuySartorelli]

No, it would go to this

    public static function query($sql, $errorLevel = E_USER_ERROR)
    {
        // ...
        updateConnectionIfNeeded($sql);
        $name = DB::getDefaultConnectionName();
        return DB::get_conn($name)->query($sql, $errorLevel);

I guess part of the problem is just naming because for backwards compatibility we have to use 'default' instead of 'dynamic'

We could move this to CMS 6 and rename it if you like. Or deprecate "default" now, and have the const set to "dynamic" - and if "default" is passed in, throw a deprecation warning and set it to "dynamic".

That would probably resolve this problem for me, since a "dynamic connection" could dynamically change based on sql whereas a "default connection" feels like it should have been determined already.

[GuySartorelli]

• NEW Allow database read-only replicas #11379 (comment) still hasn't been addressed
• New comment in NEW Allow database read-only replicas #11379 (comment) (sorry, forgot to start review)

[GuySartorelli]

Ahh okay. My intention was always for this to be the "tell me if I am using the primary" method, not the "Get that property's value" method.

Probably not worth having a getter just for this property on its own, that'll just be confusing I think especially when there's also a willUsePrimary.
IMO remove this getter