Skip to content

Convert empty timestamp (Timestamp(0, 0)) to current timestamp on insert#11

Draft
richardsimmonds wants to merge 1 commit into
mainfrom
fix/513-empty-timestamp-insert
Draft

Convert empty timestamp (Timestamp(0, 0)) to current timestamp on insert#11
richardsimmonds wants to merge 1 commit into
mainfrom
fix/513-empty-timestamp-insert

Conversation

@richardsimmonds

Copy link
Copy Markdown
Owner

Fixes documentdb#513

Problem

Native MongoDB replaces an empty timestamp value Timestamp(0, 0) in a top-level field of an inserted document with the current timestamp. DocumentDB stored the empty timestamp literally:

// MongoDB
db.coll.insert({_id: 1, k: Timestamp(0, 0)});
db.coll.findOne({_id: 1});  // { "_id" : 1, "k" : Timestamp(1770764455, 1) }

// DocumentDB (before this change)
db.coll.findOne({_id: 1});  // { _id: 1, k: Timestamp({ t: 0, i: 0 }) }

Change

Adds ReplaceEmptyTimestampsWithCurrentTime to the insert preprocessing path (PreprocessInsertionDoc in pg_documentdb/src/commands/insert.c), which rewrites any top-level Timestamp(0, 0) field to the current time. Matching MongoDB semantics:

  • only top-level fields are converted (not nested documents or arrays)
  • the _id field is exempt and stored as-is
  • non-empty timestamps are unchanged

The common case (no empty timestamps) takes a scan-only fast path and returns the original document without rewriting. The increment portion is populated from the clock's nanosecond component, consistent with the existing $currentDate update operator (HandleUpdateDollarCurrentDate).

Tests

New regression test insert_empty_timestamp_tests (added to basic_schedule) covering:

  • single and multiple top-level empty timestamps via insert_one
  • the batch insert command path
  • _id exemption, nested-document and array non-conversion
  • non-empty timestamps stored as-is
  • validation that converted values fall within the clock window around the insert (would fail on the old code, which stores t: 0)

Self-review

Checked: symbol usage verified against headers (PgbsonWriterAppendValue, bson_iter_key_string_view, StringViewEquals, IdFieldStringView); clock_gettime usage mirrors the compiling pattern in bson_update_operators.c; unique regress collection id (1985100); DCO sign-off present; diff is minimal (one C function + tests + schedule line). Known limitation: full extension build not run locally — CI is the authoritative compile/regress gate.

Native MongoDB replaces an empty timestamp value Timestamp(0, 0) in a
top-level field of an inserted document with the current timestamp.
DocumentDB previously stored the empty timestamp literally.

Add ReplaceEmptyTimestampsWithCurrentTime to the insert preprocessing
path (PreprocessInsertionDoc), which rewrites any top-level
Timestamp(0, 0) field to the current time. Matching MongoDB semantics:
- only top-level fields are converted (not nested documents or arrays)
- the _id field is exempt and stored as-is
- non-empty timestamps are unchanged

The increment portion is populated from the clock's nanosecond
component, consistent with the existing $currentDate update operator
(HandleUpdateDollarCurrentDate).

Add regression test insert_empty_timestamp_tests covering insert_one
and batch insert paths, nested/array/_id exemptions, and validation
that the converted value falls within the insert time window.

Fixes documentdb#513

Signed-off-by: richardsimmonds <richardsimmonds314@gmail.com>

@patty-chow patty-chow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Verdict: LGTM — recommend merge (pending CI), with two non-blocking follow-ups noted below.

Correctness

  • Placement of the rewrite in PreprocessInsertionDoc is right: it runs after RewriteDocumentValueAddObjectId and before ComputeShardKeyHashForDocument / PgbsonGetDocumentId, so the shard key hash is computed over the converted value — the stored document and its hash stay consistent even if a timestamp field is part of the shard key.
  • All insert entry points funnel through this function: batch path (DoMultiInsertWithoutTransactionId, insert.c:621) and single/retryable path (ProcessInsertion, insert.c:1003). The worker path (CallInsertWorkerForInsertOnecommand_insert_worker) receives the already-preprocessed document, so conversion happens exactly once on the coordinator, and retryable writes return the retry record before preprocessing — no double-conversion or retry skew.
  • Detection scan + fast-path return of the original pgbson for the common case is the right shape; no extra allocation when no empty timestamps are present.
  • Semantics match documented MongoDB behavior: top-level only, _id exempt, non-empty timestamps untouched.

C safety

  • No leaks or null derefs spotted. The writer-built pgbson is allocated in the caller's memory context with the same lifetime as the document RewriteDocumentValueAddObjectId already produces there (including inside the batch path's subtransaction PG_TRY block). Stack iterators, no manual frees needed.
  • Symbols check out: IdFieldStringView via commands_common.h (already included), bson_iter_key_string_view/StringViewEquals via bson_core.h, clock_gettime mirrors the compiling usage in bson_update_operators.c:603.

Tests

  • Good coverage: insert_one, batch insert command, multi-field conversion, _id exemption, nested/array non-conversion, non-empty passthrough. The clock-window assertion would genuinely fail on old code (stored t=0 falls outside the window), so this is a real regression test, not a tautology.
  • Test helpers mirror the established test_update_currentDate pattern in bson_update_document_tests.sql (clock window + ±1s flake buffer). Unique collection id 1985100; schedule line reuses an existing parallel group — consistent with repo conventions.

Non-blocking follow-ups

  1. Upsert path gap — replace-style upserts (UpsertDocumentInsertOrReplaceDocument in update.c) don't go through PreprocessInsertionDoc, so an upserted document with Timestamp(0,0) is stored literally. Native MongoDB converts on the upsert-insert path too. Worth a follow-up issue rather than scope-creeping this PR.
  2. Increment semantics — MongoDB populates i with an ordinal counter (distinct value per replaced timestamp within a second); this PR stores tv_nsec, matching the existing $currentDate implementation's documented TODO. Consistent-with-codebase is the right call; just noting the known divergence.

DCO / scope

  • Signed-off-by present on the single commit. Diff is minimal — one static function, call site, test, schedule line. No unrelated changes.

CI

  • Status at review time: gateway tests in progress; the build/functional jobs (functional-pr-gate, image build+test, build-documentdb-local) show skipped on this fork — so the self-review's "CI is the compile gate" assumption may not hold here. Recommend confirming at least one compile-exercising job actually ran green before merge.

* Native Mongo compatibility: an empty timestamp value (Timestamp(0, 0))
* in a top-level field is replaced with the current timestamp on insert.
*/
insertDoc = ReplaceEmptyTimestampsWithCurrentTime(insertDoc);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placement is correct — conversion before ComputeShardKeyHashForDocument means the hash is computed over the converted value, keeping the stored document and shard key hash consistent.

One gap to track separately: replace-style upserts (UpsertDocument -> InsertOrReplaceDocument in update.c) don't funnel through PreprocessInsertionDoc, so an upserted document containing Timestamp(0,0) will still be stored literally. Native MongoDB converts on that path too. Suggest a follow-up issue rather than expanding this PR.

bson_value_t currentTimestampValue = { 0 };
currentTimestampValue.value_type = BSON_TYPE_TIMESTAMP;
currentTimestampValue.value.v_timestamp.timestamp = spec.tv_sec;
currentTimestampValue.value.v_timestamp.increment = spec.tv_nsec;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: native MongoDB sets i to an ordinal counter (so multiple empty timestamps replaced in the same second get distinct, ordered values); storing tv_nsec matches the existing $currentDate implementation (bson_update_operators.c:615, which carries the same TODO). Fine to keep consistent with the codebase — just flagging the known divergence so it's a deliberate choice.

LANGUAGE plpgsql;

-- an empty timestamp in a top-level field is converted to the current timestamp on insert
SELECT test_insert_one_empty_timestamp('{ "_id": 1, "k": { "$timestamp": { "t": 0, "i": 0 } } }', '{ "_id": 1 }', ARRAY['k']::text[]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — this assertion is a genuine regression guard: old code stores t=0, which falls outside the clock window, so the test fails pre-fix. Helper pattern matches the established test_update_currentDate approach in bson_update_document_tests.sql.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timestamp(0, 0) not auto-converted to current timestamp on insert

2 participants