Skip to content

Clarify wide column memory ownership in public APIs (#14626)#14626

Closed
xingbowang wants to merge 1 commit into
facebook:mainfrom
xingbowang:2026_04_16_wide_column_fix
Closed

Clarify wide column memory ownership in public APIs (#14626)#14626
xingbowang wants to merge 1 commit into
facebook:mainfrom
xingbowang:2026_04_16_wide_column_fix

Conversation

@xingbowang

@xingbowang xingbowang commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

Summary:
We have seen multiple coding error with the usage of WideColumn API. Hence, improving the documentation to reduce the chance of this happening again.

Clarify that WideColumn and WideColumns are non-owning views over caller-managed memory.

Update the public PutEntity() API documentation to state that column name and value storage must remain valid until the call returns.

Add safe and unsafe usage examples in wide_columns.h to make lifetime requirements explicit.

Testing

Not run. Documentation-only change.

Reviewed By: archang19

Differential Revision: D101280295

Pulled By: xingbowang

@meta-cla meta-cla Bot added the CLA Signed label Apr 17, 2026
@github-actions

Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

@github-actions

Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 7744d0e


Code Review: Clarify wide column memory ownership in public APIs

Scope: Documentation-only change (6 files, +43/-12 lines)

The core documentation claims are correctWideColumn is non-owning (holds Slice members), and PutEntity implementations copy/serialize the data so backing storage only needs to survive until the call returns. Three issues found:


F1 — Unsafe example comment formatting (MEDIUM)

include/rocksdb/wide_columns.h — The sentence "the temporary std::string storage is destroyed before PutEntity()" is split across two different comment prefix styles (// // then //), creating visual ambiguity:

// // Unsafe: the temporary std::string storage is destroyed before
// PutEntity(). WideColumns bad_columns{

Suggest using consistent formatting, e.g.:

// Unsafe example — the temporary std::string objects are destroyed before
// PutEntity() is called:
// WideColumns bad_columns{

F2 — ASSERT_OK in public header example (LOW)

include/rocksdb/wide_columns.h — The safe example uses ASSERT_OK(db->PutEntity(...)). This test-only macro never appears elsewhere in include/rocksdb/ public headers. Users copying this example will get a compile error. Suggest: Status s = db->PutEntity(...).

F3 — Missing documentation on additional PutEntity APIs (LOW)

A few public APIs taking WideColumns were not annotated:

  • Transaction::PutEntityUntracked() (utilities/transaction.h:581)
  • WriteBatchWithIndex::PutEntity() (utilities/write_batch_with_index.h:145)
  • DB::PutEntity(options, key, AttributeGroups) (db.h:454) — AttributeGroup contains WideColumns

ID Severity Description
F1 MEDIUM Unsafe example comment formatting is visually confusing
F2 LOW ASSERT_OK test macro used in public header example
F3 LOW A few additional PutEntity APIs not annotated

Documentation content is technically correct and addresses a real usability problem. Findings are presentation/coverage suggestions, not correctness issues.


ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase

@meta-codesync

meta-codesync Bot commented Apr 17, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D101280295.

Summary:
We have seen multiple coding error with the usage of WideColumn API. Hence, improving the documentation to reduce the chance of this happening again.

Clarify that `WideColumn` and `WideColumns` are non-owning views over caller-managed memory.

Update the public `PutEntity()` API documentation to state that column name and value storage must remain valid until the call returns.

Add safe and unsafe usage examples in `wide_columns.h` to make lifetime requirements explicit.

## Testing

Not run. Documentation-only change.


Reviewed By: archang19

Differential Revision: D101280295

Pulled By: xingbowang
@meta-codesync meta-codesync Bot changed the title Clarify wide column memory ownership in public APIs Clarify wide column memory ownership in public APIs (#14626) Jun 13, 2026
@xingbowang xingbowang force-pushed the 2026_04_16_wide_column_fix branch from 7744d0e to 62efb98 Compare June 13, 2026 12:58
@meta-codesync

meta-codesync Bot commented Jun 13, 2026

Copy link
Copy Markdown

@xingbowang has exported this pull request. If you are a Meta employee, you can view the originating Diff in D101280295.

@meta-codesync

meta-codesync Bot commented Jun 13, 2026

Copy link
Copy Markdown

@xingbowang merged this pull request in c9252b9.

@meta-codesync meta-codesync Bot added the Merged label Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants