Introduce statement snapshot type for Rdb_transaction #1497

laurynas-biveinis · 2024-10-14T08:20:44Z

Previously, the snapshot state was tracked by a combination of
flags (m_is_delayed_snapshot, m_tx_read_only), and by certain snapshot pointers
being null or non-null. (i.e. m_read_opts[USER_TABLE].snapshot,
m_explicit_snapshot). The transaction state specified by different combinations
of the above was not immediately obvious and the purpose of the read only flag
was not the RO-ness as such but rather using the DB instead of Transaction API
to acquire snapshots.

Consolidate the states into an enum class snapshot_type { ... }
statement_snapshot_type field, consuming the two flags above. Introduce and
assert invariant across the state values and snapshot pointers being null or
non-null. Add to the invariant of AC-NL-RO-RC transactions. Rename
snapshot_created to assign_snapshot to better reflect what the method does (as
it does not, in fact, take a newly-created snapshot i.e. in the case of existing
explicit snapshot). Make share_explicit_snapshot and create_explicit_snapshot
set the m_read_opts snapshot and remove the redundant code from the callers. For
the latter, make it create the snapshot instead of taking the m_read_opts one.

Two of the possible states are EXPLICIT and READ_ONLY_TRX, and the existing code
implicitly had another state which was the combination of the two. Avoid
introducing such new state, and use EXPLICIT in it too. This results in a minor
user-visible change that START TRANSACTION WITH SHARED|EXISTING SNAPSHOT
transactions are grouped with CREATE|ATTACH EXPLICIT SNAPSHOT ones instead of
START TRANSACTION WITH CONSISTENT SNAPSHOT for the purposes of
INFORMATION_SCHEMA read-only flag and diagnostics on write attempts. Update the
diagnostic messages to reflect this.

facebook-github-bot · 2024-10-23T17:42:13Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2024-10-31T05:30:03Z

could you take a look main.subselect_debug 'rocksdb_intrinsic_table' MTR? The following assert failed

storage/rocksdb/ha_rocksdb.cc:4976: std::unique_ptrrocksdb::Iterator myrocks::Rdb_transaction::get_iterator(rocksdb::ColumnFamilyHandle &, bool, const rocksdb::Slice &, const rocksdb::Slice &, myrocks::TABLE_TYPE, bool, bool): Assertion `statement_snapshot_type != snapshot_type::NONE || create_snapshot' failed.

facebook-github-bot · 2024-10-31T13:57:59Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis · 2024-10-31T13:58:37Z

@luqun , fixed, the assert did not accept temp tables. Also rebased, ready for review

facebook-github-bot · 2024-11-04T17:20:13Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

storage/rocksdb/ha_rocksdb.cc

facebook-github-bot · 2024-11-07T08:25:40Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-11-07T17:10:21Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2024-11-08T06:28:49Z

storage/rocksdb/ha_rocksdb.cc

+    // This is used by transactions started with "START TRANSACTION WITH
+    // CONSISTENT [ROCKSDB] SNAPSHOT". The snapshot has to be created via
+    // DB::GetSnapshot(), not via Transaction API.
+    READ_ONLY_TRX,


From rocksdb_explicit_snapshot/create_explicit_snapshot, Looks like EXPLICIT snapshot also create via DB::GetSnapshot(). maybe add a comment for EXPLICIT

luqun · 2024-11-08T06:46:33Z

storage/rocksdb/ha_rocksdb.cc

-    API.
-  */
-  bool is_tx_read_only() const { return m_tx_read_only; }
+  [[nodiscard]] bool is_tx_read_only() const noexcept {


how about add _snapshot into method name?

It is the whole transaction that is read only

luqun · 2024-11-08T06:48:05Z

storage/rocksdb/ha_rocksdb.cc

@@ -15163,7 +15362,8 @@ int ha_rocksdb::external_lock(THD *const thd, int lock_type) {
        DBUG_RETURN(HA_ERR_UNSUPPORTED);
      }

-      if (thd->get_explicit_snapshot()) {
+      if (unlikely(tx->has_explicit_snapshot() ||


why do we need check both(tx and thd)?

The state lifetimes don't fully match but only overlap. For instance,

CREATE EXPLICIT ROCKSDB SNAPSHOT; INSERT INTO T1 VALUES(); # tx state not explicit, thd flag = true

And the opposite case:

START TRANSACTION WITH SHARED ROCKSDB SNAPSHOT; INSERT INTO T1 VALUES(); # tx state explicit, thd flag = false

The tx state is set during transaction execution. The thd snapshot is set on
CREATE/ATTACH ROCKSDB SNAPSHOT, when there is no tx object.

I'm adding a clarifying comment.

facebook-github-bot · 2024-11-11T09:02:00Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis · 2024-11-11T10:10:39Z

@luqun , ready for review

facebook-github-bot · 2024-11-11T23:47:11Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hermanlee

Overall looks good to me.

hermanlee · 2024-11-11T21:29:34Z

storage/rocksdb/ha_rocksdb.cc

+      case snapshot_type::NONE:
+        assert(m_explicit_snapshot == nullptr);
+        assert(m_read_opts[USER_TABLE].snapshot == nullptr);
+        break;
+      case snapshot_type::CURRENT:
+        assert(m_explicit_snapshot == nullptr);
+        assert(m_read_opts[USER_TABLE].snapshot != nullptr);
+        break;
+      case snapshot_type::CURRENT_DELAYED:
+        assert(m_explicit_snapshot == nullptr);
+        assert(m_read_opts[USER_TABLE].snapshot == nullptr);
+        break;


Since these are all the same, it might be simpler to combine them all. Logically, it means that even though these are slightly different snapshot cases, they follow the same invariantsonce the snapshot is allocated.

Will do (but only for the 1st and the 3rd: the 2nd has the 2nd condition check inverted)

hermanlee · 2024-11-11T22:01:17Z

storage/rocksdb/ha_rocksdb.cc

+    switch (statement_snapshot_type) {
+      case snapshot_type::CURRENT:
+        assert(m_read_opts[USER_TABLE].snapshot == nullptr);
+        break;
+      case snapshot_type::CURRENT_DELAYED:
+        assert(m_read_opts[USER_TABLE].snapshot == nullptr);
+        statement_snapshot_type = snapshot_type::CURRENT;
+        break;
+      case snapshot_type::READ_ONLY_TRX:
+        assert(m_read_opts[USER_TABLE].snapshot == nullptr);
+        break;
+      case snapshot_type::EXPLICIT:
+        assert(snapshot ==
+               m_explicit_snapshot->get_snapshot()->snapshot());
+        break;
+      case snapshot_type::NONE:
+        assert(false);
+        __builtin_unreachable();
+        break;
+    }
+


Maybe have this whole block be under NDEBUG, and pull out the case for changing the snapshot type from CURRENT_DELAYED to CURRENT?

OK, also merging the case branches like for the previous comment

And removing __builtin_unreachable() because it no longer has effect in debug-only code

storage/rocksdb/ha_rocksdb.cc

Previously, the snapshot state was tracked by a combination of flags (m_is_delayed_snapshot, m_tx_read_only), and by certain snapshot pointers being null or non-null. (i.e. m_read_opts[USER_TABLE].snapshot, m_explicit_snapshot). The transaction state specified by different combinations of the above was not immediately obvious and the purpose of the read only flag was not the RO-ness as such but rather using the DB instead of Transaction API to acquire snapshots. Consolidate the states into an enum class snapshot_type { ... } statement_snapshot_type field, consuming the two flags above. Introduce and assert invariant across the state values and snapshot pointers being null or non-null. Add to the invariant of AC-NL-RO-RC transactions. Rename snapshot_created to assign_snapshot to better reflect what the method does (as it does not, in fact, take a newly-created snapshot i.e. in the case of existing explicit snapshot). Make share_explicit_snapshot and create_explicit_snapshot set the m_read_opts snapshot and remove the redundant code from the callers. For the latter, make it create the snapshot instead of taking the m_read_opts one. Two of the possible states are EXPLICIT and READ_ONLY_TRX, and the existing code implicitly had another state which was the combination of the two. Avoid introducing such new state, and use EXPLICIT in it too. This results in a minor user-visible change that START TRANSACTION WITH SHARED|EXISTING SNAPSHOT transactions are grouped with CREATE|ATTACH EXPLICIT SNAPSHOT ones instead of START TRANSACTION WITH CONSISTENT SNAPSHOT for the purposes of INFORMATION_SCHEMA read-only flag and diagnostics on write attempts. Update the diagnostic messages to reflect this.

facebook-github-bot · 2024-11-12T10:49:23Z

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis · 2024-11-12T10:49:40Z

Ready for review again

facebook-github-bot · 2024-11-13T18:51:44Z

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

luqun · 2024-11-14T21:49:36Z

rocksdb_rpl.rpl_skip_trx_api_binlog_format MTR failed with
virtual void myrocks::Rdb_writebatch_impl::acquire_snapshot(bool, TABLE_TYPE): Assertion `statement_snapshot_type == snapshot_type::CURRENT' failed.

laurynas-biveinis · 2024-11-15T08:30:07Z

rocksdb_rpl.rpl_skip_trx_api_binlog_format MTR failed with virtual void myrocks::Rdb_writebatch_impl::acquire_snapshot(bool, TABLE_TYPE): Assertion `statement_snapshot_type == snapshot_type::CURRENT' failed.

@luqun, I am unable to reproduce. Do you have more details?

facebook-github-bot added the CLA Signed label Oct 14, 2024

laurynas-biveinis force-pushed the myr-statement-snapshot branch from ce8893a to ff22bb6 Compare October 31, 2024 13:57

luqun reviewed Nov 6, 2024

View reviewed changes

storage/rocksdb/ha_rocksdb.cc Outdated Show resolved Hide resolved

laurynas-biveinis force-pushed the myr-statement-snapshot branch from ff22bb6 to 9c9cf12 Compare November 7, 2024 08:25

luqun reviewed Nov 8, 2024

View reviewed changes

laurynas-biveinis force-pushed the myr-statement-snapshot branch from 9c9cf12 to e507729 Compare November 11, 2024 09:01

hermanlee reviewed Nov 12, 2024

View reviewed changes

laurynas-biveinis force-pushed the myr-statement-snapshot branch from e507729 to fe22e56 Compare November 12, 2024 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce statement snapshot type for Rdb_transaction #1497

Introduce statement snapshot type for Rdb_transaction #1497

laurynas-biveinis commented Oct 14, 2024

facebook-github-bot commented Oct 23, 2024

luqun commented Oct 31, 2024

facebook-github-bot commented Oct 31, 2024

laurynas-biveinis commented Oct 31, 2024

facebook-github-bot commented Nov 4, 2024

facebook-github-bot commented Nov 7, 2024

facebook-github-bot commented Nov 7, 2024

luqun Nov 8, 2024 •

edited

Loading

luqun Nov 8, 2024

laurynas-biveinis Nov 11, 2024

luqun Nov 8, 2024

laurynas-biveinis Nov 11, 2024

facebook-github-bot commented Nov 11, 2024

laurynas-biveinis commented Nov 11, 2024

facebook-github-bot commented Nov 11, 2024

hermanlee left a comment

hermanlee Nov 11, 2024

laurynas-biveinis Nov 12, 2024

hermanlee Nov 11, 2024

laurynas-biveinis Nov 12, 2024

laurynas-biveinis Nov 12, 2024

facebook-github-bot commented Nov 12, 2024

laurynas-biveinis commented Nov 12, 2024

facebook-github-bot commented Nov 13, 2024

luqun commented Nov 14, 2024

laurynas-biveinis commented Nov 15, 2024

Introduce statement snapshot type for Rdb_transaction #1497

Are you sure you want to change the base?

Introduce statement snapshot type for Rdb_transaction #1497

Conversation

laurynas-biveinis commented Oct 14, 2024

facebook-github-bot commented Oct 23, 2024

luqun commented Oct 31, 2024

facebook-github-bot commented Oct 31, 2024

laurynas-biveinis commented Oct 31, 2024

facebook-github-bot commented Nov 4, 2024

facebook-github-bot commented Nov 7, 2024

facebook-github-bot commented Nov 7, 2024

luqun Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 11, 2024

laurynas-biveinis commented Nov 11, 2024

facebook-github-bot commented Nov 11, 2024

hermanlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2024

laurynas-biveinis commented Nov 12, 2024

facebook-github-bot commented Nov 13, 2024

luqun commented Nov 14, 2024

laurynas-biveinis commented Nov 15, 2024

luqun Nov 8, 2024 •

edited

Loading