WT-16565 Refactor removal of shared metadata to use queue by jiechenbo · Pull Request #13115 · wiredtiger/wiredtiger

jiechenbo · 2026-01-30T03:05:09Z

After WT-16458 refactors how we update the shared metadata table, so that we put all updates in a queue and then process them during a checkpoint. I have re-used the queue to also queue removes. Here are some technical changes I have made:

Renamed queue and related variables from update_metadata_qh to shared_metadata_qh .
Added WT_SHARED_METADATA_OPS struct to the metadata queue entry.
Update metadata queue process to process removes as well
Add defer variable to shared metadata queue that will defer metadata operations to the checkpoint after if a concurrent checkpoint is happening.

There are few changes, I would like for reviewers to review:

The queue will have contain both insert/removes entries. This also means that we could have one entry performing creation of same table and then we directly do a delete after.
We have a very similar for __disagg_remove_shared_metadata and __disagg_update_shared_metadata. Ideally I would like to combine them.

korteland

LGTM!

pmacko86

Almost LGTM. I found one race condition, but as I explained in the comment, it should not be hard to fix if we choose the easiest option. All my other comments are relatively minor and mostly suggest better naming. Thanks.

pmacko86 · 2026-02-23T19:12:11Z

src/include/connection.h


-    TAILQ_ENTRY(__wt_disagg_update_metadata) q; /* Linked list of entries. */
+    /* Metadata type operation. */
+    u_int metadata_op;


Change the type of this to WT_SHARED_METADATA_OPS (and move the definition of WT_SHARED_METADATA_OPS above this struct definition).

pmacko86 · 2026-02-23T19:13:23Z

src/include/connection.h

+    SHARED_METADATA_UPDATE = 0,
+    SHARED_METADATA_CREATE,
+    SHARED_METADATA_REMOVE
+} WT_SHARED_METADATA_OPS;


As mentioned in my earlier comment, it would be better to use this as a type. In that case, it would be better to rename it to WT_SHARED_METADATA_OP.

pmacko86 · 2026-02-23T19:14:01Z

src/include/connection.h

+ * Identify the shared metadata operations inside the shared metadata queue.
+ */
+typedef enum {
+    SHARED_METADATA_UPDATE = 0,


Also, I think it would be better to use the WT_ prefix for these, such as WT_SHARED_METADATA_UPDATE?

pmacko86 · 2026-02-23T19:15:35Z

src/include/connection.h

-    TAILQ_HEAD(__wt_disagg_update_metadata_qh, __wt_disagg_update_metadata) update_metadata_qh;
-    WT_SPINLOCK update_metadata_lock;
+    TAILQ_HEAD(__wt_disagg_shared_metadata_qh, __wt_disagg_metadata_op) shared_metadata_qh;
+    WT_SPINLOCK shared_metadata_lock;


The new name of this lock implies that we are locking the entire shared metadata table (analogously to the schema lock), so please rename this field accordingly. Maybe shared_metadata_queue_lock? The name might feel a bit long, but I think here clarity trumps conciseness.

pmacko86 · 2026-02-23T19:16:55Z

src/conn/conn_layered.c

 */
 static void
-__disagg_update_metadata_clear(WT_SESSION_IMPL *session)
+__disagg_shared_metadata_clear(WT_SESSION_IMPL *session)


Same here – the new name implies clearing all of shared metadata. Instead, we are just clearing the queue, so you might want to name it something like __disagg_shared_metadata_queue_clear instead.

pmacko86 · 2026-02-23T19:21:37Z

src/conn/conn_layered.c


 /*
- * __disagg_update_metadata_free --
+ * __disagg_shared_metadata_free --


Same here – need a clearer name, e.g., __disagg_shared_metadata_op_free.

pmacko86 · 2026-02-23T19:23:56Z

src/conn/conn_layered.c

+      "operation to shared "
      "metadata table at next checkpoint:",
-      table_name, stable_uri);
+      table_name, stable_uri, metadata_op == SHARED_METADATA_UPDATE ? "UPDATE" : "DELETE");


This needs a case for CREATE. As this is already the second case of us needing to convert this to a string, it would be useful to have a helper function to do that. As the helper function would be returning just a static string constant, I think you can make it return a const char * type, which would make it easier to use.

pmacko86 · 2026-02-23T19:24:47Z

src/conn/conn_layered.c

 */
 int
-__wt_disagg_update_metadata_process(WT_SESSION_IMPL *session)
+__wt_disagg_shared_metadata_process(WT_SESSION_IMPL *session)


For consistency, I'd rename this to __wt_disagg_shared_metadata_queue_process, even though the risk of misunderstanding this function from its name is less.

pmacko86 · 2026-02-23T19:25:36Z

src/conn/conn_layered.c

+        if (entry->deferred) {
+            __wt_verbose_debug2(session, WT_VERB_DISAGGREGATED_STORAGE,
+              "Defer metadata %s operation for table \"%s\"", entry->table_name,
+              entry->metadata_op == SHARED_METADATA_REMOVE ? "create" : "update");


Same here – after you add the helper function to convert metadata_op to string, also use it here.

pmacko86 · 2026-02-23T19:49:59Z

src/conn/conn_layered.c

+     * metadata table for that checkpoint. We defer these metadata operations to the next checkpoint
+     * to keep the checkpoints metadata and table state consistent.
+     */
+    WT_ACQUIRE_READ_WITH_BARRIER(ckpt_running, conn->txn_global.checkpoint_running);


I am not sure if this is correct, because conn->txn_global.checkpoint_running is set to true before the checkpoint transaction's snapshot is acquired. We need to prevent the following scenario for create:

Thread 1: conn->txn_global.checkpoint_running = true

Thread 2: The new table gets created.

Thread 2: The _mdb_catalog is updated accordingly and committed.

Thread 1: The checkpoint transaction acquires its snapshot.
In this case, the update to _mdb_catalog would be included in the checkpoint, but the new table would not, which would lead to metadata inconsistency.

If we use another field that gets set after the transaction snapshot is acquired, we would risk a similar metadata inconsistency problem with drop.

I think we have the following options, in order of increasing difficulty:

Do not defer create, since it's always okay for the create to happen before the _mdb_catalog update. (But still keep the separate enum value for create, as it's likely we'd like to use it in the future.)

Use two different mechanism to determine whether we are deferring, depending on whether we are doing remove vs. create. This would be kind of ugly, but it's an option.

Do this exactly based on whether the transaction ID of the create/drop metadata update is included in the transaction snapshot. This would take careful thought to ensure we do this correctly and really understand all relevant corner cases, because each local metadata table update is done with a different transaction.

I think option 1 is probably what we'd like to do for now, as it's simplest, and as I think it should be the easiest to reason through its correctness.

Sure I went down option 1.

pmacko86

LGTM, thank you!

src/conn/conn_layered.c

src/include/connection.h

korteland

LGTM!

jiechenbo added 28 commits January 22, 2026 23:21

Work stuff

8977f9b

Added to palite

58576af

Final draft of trim

edc55ec

Fix up layered

9d20c20

Fix tests

a2dfd7e

Add error checking

944dd5c

Changed param order

080c758

Address PR changes

77b7d17

Address PR changes

dbc65ad

Properly remove all related files

a73c7b5

Ran s_all

4b3c097

Ran s_all

765f22a

Add delete to globals table

8f103e8

Updated test to test for shared metadata

4c81982

Refactored

cae427e

Uncomment layered28

139eb29

Fix test

37e0a13

Revert python file

e0e826f

Merge branch 'develop' into wt-16465-sls-trim-command

5dfa426

Address PR changes

6ee9dec

Update comment

5a11180

Run s_all

22b2292

Address PR changes

f8c7384

Update test

66d6612

Rename to shared metadata queue

e54ce03

Implement removal on queue now

b35ffa6

Merge branch 'develop' into wt-16565-refactor-remove-shared-queue

fa9edb9

Remove __layered_last_checkpoint_order

4a58c08

jiechenbo requested a review from a team as a code owner January 30, 2026 03:05

jiechenbo requested a review from mana2-bot January 30, 2026 03:05

jiechenbo added 2 commits February 16, 2026 04:05

Refactored functions

516b27c

Address PR changes

aac20fe

jiechenbo requested review from korteland and pmacko86 February 16, 2026 04:31

korteland approved these changes Feb 16, 2026

View reviewed changes

Add defer variable

d6fea41

jiechenbo requested a review from korteland February 19, 2026 03:09

jiechenbo added 8 commits February 19, 2026 03:13

Merge branch 'develop' into wt-16565-refactor-remove-shared-queue

98652ec

Fix PR

f2b2734

Deffer drop operation only

ffff956

Fix metadata ops

e8334ab

Add Shared metadata create

5cc7e1f

Merge branch 'develop' into wt-16565-refactor-remove-shared-queue

cef123e

Skip test

bb46787

Move ski[

260ad94

pmacko86 reviewed Feb 23, 2026

View reviewed changes

jiechenbo added 2 commits February 23, 2026 22:45

ADdress PR changes

c6c809f

Defer only remove

10cac01

jiechenbo requested a review from pmacko86 February 23, 2026 23:54

pmacko86 approved these changes Feb 24, 2026

View reviewed changes

korteland reviewed Feb 24, 2026

View reviewed changes

src/conn/conn_layered.c Outdated Show resolved Hide resolved

src/include/connection.h Outdated Show resolved Hide resolved

jiechenbo added 4 commits February 25, 2026 00:17

Address PR

0c0a0c9

Merge branch 'develop' into wt-16565-refactor-remove-shared-queue

85ccd03

Rmemoved unused func

b307546

Remove func again

23f7e70

korteland approved these changes Feb 25, 2026

View reviewed changes

jiechenbo added this pull request to the merge queue Feb 25, 2026

Merged via the queue into develop with commit 185f84f Feb 25, 2026
14 checks passed

jiechenbo deleted the wt-16565-refactor-remove-shared-queue branch February 25, 2026 23:37

Conversation

jiechenbo commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

korteland left a comment

Choose a reason for hiding this comment

Uh oh!

pmacko86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmacko86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

korteland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jiechenbo commented Jan 30, 2026 •

edited

Loading