Merge "Disallow mismatched beginMasterChanges/commitMasterChanges"

diff --combined includes/db/loadbalancer/LBFactory.php
index 226ac08,e40001d..2d91bb7
--- 1/includes/db/loadbalancer/LBFactory.php
--- 2/includes/db/loadbalancer/LBFactory.php
+++ b/includes/db/loadbalancer/LBFactory.php
@@@ -230,6 -230,7 +230,7 @@@ abstract class LBFactory implements Des
         *   - commitMasterChanges()
         *   - rollbackMasterChanges()
         *   - commitAll()
+        *
         * This allows for custom transaction rounds from any outer transaction scope.
         *
         * @param string $fname
@@@ -240,7 -241,7 +241,7 @@@
                if ( $this->trxRoundId !== false ) {
                        throw new DBTransactionError(
                                null,
-                               "Transaction round '{$this->trxRoundId}' already started."
+                               "$fname: transaction round '{$this->trxRoundId}' already started."
                        );
                }
                $this->trxRoundId = $fname;
@@@ -261,7 -262,7 +262,7 @@@
        /**
         * Commit on all connections. Done for two reasons:
         * 1. To commit changes to the masters.
 -       * 2. To release the snapshot on all connections, master and slave.
 +       * 2. To release the snapshot on all connections, master and replica DB.
         * @param string $fname Caller name
         * @param array $options Options map:
         *   - maxWriteDuration: abort if more than this much time was spent in write queries
@@@ -279,6 -280,12 +280,12 @@@
         * @throws Exception
         */
        public function commitMasterChanges( $fname = __METHOD__, array $options = [] ) {
+               if ( $this->trxRoundId !== false && $this->trxRoundId !== $fname ) {
+                       throw new DBTransactionError(
+                               null,
+                               "$fname: transaction round '{$this->trxRoundId}' still running."
+                       );
+               }
                // Run pre-commit callbacks and suppress post-commit callbacks, aborting on failure
                $this->forEachLBCallMethod( 'finalizeMasterChanges' );
                $this->trxRoundId = false;
@@@ -356,28 -363,19 +363,28 @@@
        }
  
        /**
 -       * Detemine if any lagged slave connection was used
 -       * @since 1.27
 +       * Detemine if any lagged replica DB connection was used
         * @return bool
 +       * @since 1.28
         */
 -      public function laggedSlaveUsed() {
 +      public function laggedReplicaUsed() {
                $ret = false;
                $this->forEachLB( function ( LoadBalancer $lb ) use ( &$ret ) {
 -                      $ret = $ret || $lb->laggedSlaveUsed();
 +                      $ret = $ret || $lb->laggedReplicaUsed();
                } );
  
                return $ret;
        }
  
 +      /**
 +       * @return bool
 +       * @since 1.27
 +       * @deprecated Since 1.28; use laggedReplicaUsed()
 +       */
 +      public function laggedSlaveUsed() {
 +              return $this->laggedReplicaUsed();
 +      }
 +
        /**
         * Determine if any master connection has pending/written changes from this request
         * @return bool
@@@ -392,10 -390,10 +399,10 @@@
        }
  
        /**
 -       * Waits for the slave DBs to catch up to the current master position
 +       * Waits for the replica DBs to catch up to the current master position
         *
         * Use this when updating very large numbers of rows, as in maintenance scripts,
 -       * to avoid causing too much lag. Of course, this is a no-op if there are no slaves.
 +       * to avoid causing too much lag. Of course, this is a no-op if there are no replica DBs.
         *
         * By default this waits on all DB clusters actually used in this request.
         * This makes sense when lag being waiting on is caused by the code that does this check.
@@@ -449,7 -447,7 +456,7 @@@
                $masterPositions = array_fill( 0, count( $lbs ), false );
                foreach ( $lbs as $i => $lb ) {
                        if ( $lb->getServerCount() <= 1 ) {
 -                              // Bug 27975 - Don't try to wait for slaves if there are none
 +                              // Bug 27975 - Don't try to wait for replica DBs if there are none
                                // Prevents permission error when getting master position
                                continue;
                        } elseif ( $opts['ifWritesSince']
@@@ -473,7 -471,7 +480,7 @@@
  
                if ( $failed ) {
                        throw new DBReplicationWaitError(
 -                              "Could not wait for slaves to catch up to " .
 +                              "Could not wait for replica DBs to catch up to " .
                                implode( ', ', $failed )
                        );
                }
@@@ -518,7 -516,7 +525,7 @@@
         * This will commit and wait unless $ticket indicates it is unsafe to do so
         *
         * @param string $fname Caller name (e.g. __METHOD__)
-        * @param mixed $ticket Result of getOuterTransactionScopeTicket()
+        * @param mixed $ticket Result of getEmptyTransactionTicket()
         * @param array $opts Options to waitForReplication()
         * @throws DBReplicationWaitError
         * @since 1.28
@@@ -530,8 -528,21 +537,21 @@@
                        return;
                }
  
-               $this->commitMasterChanges( $fname );
+               $fnameEffective = $fname;
+               // The transaction owner and any caller with the empty transaction ticket can commit
+               // so that getEmptyTransactionTicket() callers don't risk seeing DBTransactionError.
+               if ( $this->trxRoundId !== false && $fname !== $this->trxRoundId ) {
+                       $this->trxLogger->info( "$fname: committing on behalf of {$this->trxRoundId}." );
+                       $fnameEffective = $this->trxRoundId;
+               }
+ 
+               $this->commitMasterChanges( $fnameEffective );
                $this->waitForReplication( $opts );
+               // If a nested caller committed on behalf of $fname, start another empty $fname
+               // transaction, leaving the caller with the same empty transaction state as before.
+               if ( $fnameEffective !== $fname ) {
+                       $this->beginMasterChanges( $fname );
+               }
        }
  
        /**
@@@ -579,7 -590,7 +599,7 @@@
                // Write them to the stash
                $unsavedPositions = $cp->shutdown();
                // If the positions failed to write to the stash, at least wait on local datacenter
 -              // slaves to catch up before responding. Even if there are several DCs, this increases
 +              // replica DBs to catch up before responding. Even if there are several DCs, this increases
                // the chance that the user will see their own changes immediately afterwards. As long
                // as the sticky DC cookie applies (same domain), this is not even an issue.
                $this->forEachLB( function ( LoadBalancer $lb ) use ( $unsavedPositions ) {