Agent/catalog incremental discovery

[jiangxinmeng1]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #26615

What this PR does / why we need it:

Agent/catalog incremental discovery

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Blocking security issue: table_changes bypasses the source table's SELECT privilege. The planner resolves the source only to derive the result schema, then emits a Node_FUNCTION_SCAN without a privilege-bearing TABLE_SCAN/source object reference. extractPrivilegeTipsFromPlan therefore sees no table, len(arr) == 0, and authentication allows the statement. The executor opens the relation directly.

I reproduced this on exact head b9b34016c79b with an authenticated embedded cluster:

1. Create secret(id primary key, payload) and insert classified.
2. Create a user whose role has only CONNECT, no SELECT on the database/table.
3. select payload from db.secret is correctly rejected with do not have privilege.
4. The same user successfully executes select payload from table_changes('db', 'secret', after, until) and receives classified.

Please make table_changes contribute the same source-table SELECT privilege requirement as a normal scan (including the existing cluster/catalog access rules), and add deny/allow black-box privilege tests. Tenant filtering is not a substitute for authorization within a tenant.

[jiangxinmeng1]

Follow-up for this review:

• The all-tables form is intentionally deferred from this PR and tracked in #26885, assigned to jiangxinmeng1. The issue covers syntax/NULL semantics, heterogeneous schemas, cursor ordering, per-table SELECT authorization, tenant filtering, and bounded memory.
• Added negative BVT coverage in commit 8f7fcd3ff: missing database/table, empty and NULL database/table names, malformed and NULL watermarks, plus existing interval validation.

The changes are pushed to agent/catalog-incremental-discovery; no all-tables implementation is included in this PR.

[LeftHandCold]

Requesting changes for two concrete production risks: (1) the new chunked on-disk column format is emitted without a rolling-upgrade protocol gate, so old readers cannot read newly written wide-column objects; and (2) bounded persisted replay retains every window filtered to zero rows, defeating the advertised memory bound. I rechecked interval/schema correctness, privilege and tenant isolation, net-effect merging across spill runs, cancellation, and handle/spill cleanup; I found no additional blocker. The all-tables form remains intentionally out of scope under #26885 and is not part of this review decision.

[LeftHandCold]

Deep-reviewed exact head 1e42f6647955 against base/merge-base 3c7392a0f86b. The prior filtered-empty replay-window leak is fixed, and its focused regression passes. Two independent production blockers remain:

P1 compatibility — the rollout gate still does not gate CN writers and becomes stale after protocol changes

chunkedColumnEnabled defaults to true, while the only production call to SetChunkedColumnEnabled is in TAE db.Open. Normal CN object production goes through CNS3Writer -> ioutil.Sinker -> objectio.Writer and never opens a TAE DB, so an upgraded CN can immediately emit Lz4Chunked objects while old CN readers are still live. The TN path also snapshots MOProtocolVersion only once during Open; queryservice.handleSetProtocolVersion later updates only the runtime variable, so changing/downgrading the rollout version does not update the writer gate. An old reader still treats algorithm id 2 as legacy LZ4 and fails on the MOCOLCH1 container.

Please make format production fail-closed and gate every actual writer with the service-specific, current deployment protocol (including runtime version transitions), rather than a process-global startup snapshot. Add a mixed-version test through the CN sinker/write path and a protocol-version transition test that proves no chunked extent is emitted below v14.

P1 availability — chunking duplicates a shared varlen area once per row

encodeChunkedColumn assumes a window's serialized size shrinks with its row count. MatrixOne vectors can legally have many row descriptors sharing one varlen area (for example UnionBatch from a constant vector; Sinker.Write reaches the same representation via UnionWindow). For a shared area larger than 8 MiB, every CloneWindow still serializes that whole area even after the halving loop reaches one row, so the encoder writes the same payload once per row.

I reproduced this on the exact head with three rows sharing one 8 MiB+1 payload: the original serialized column was 8,388,715 bytes, but the encoder produced three single-row chunks with 25,166,013 bytes of aggregate uncompressed payload (25,166,101 encoded bytes). At a legal 8,192-row block this scales toward 64 GiB from an approximately 8 MiB source vector, causing writer OOM/huge object output before the extent's uint32 size can even be represented.

Please chunk by row-reachable payload without duplicating shared area (or safely retain the legacy representation for this shape), and add an actual writer/sinker regression using repeated large constant/shared varlen values that asserts bounded physical size as well as round-trip correctness.

Focused evidence on this head:

• PASS: objectio chunk round-trip, malformed metadata, and rollout-gate unit tests.
• PASS: persisted zero-row window release and cross-chunk net-effect regression.
• PASS: table_changes planner/schema dependency tests.
• The table_function/frontend focused commands did not enter tests because the isolated checkout's reused native artifacts did not match the head's CGo declarations; exact-head CI is green, so I did not attribute that environment failure to this PR.

[XuPeng-SH]

Deep-reviewed exact head a875a39 after merging latest main 00cb7ef. No remaining blocking issue found.

The two latest P1 findings are closed systemically:

• persisted format emission is fail-closed and requires the live, service-specific v16 rollout gate at the actual CN Sinker write; runtime transitions and unowned legacy behavior are covered;
• shared varlen representations cannot multiply one large payload by row count: the writer falls back before amplification, while cached-window and legacy spill readers preserve/deduplicate the compact physical representation.

The final unhappy-path audit also fixed AddBlock error propagation, corrupt chunk-header allocation bounds, row-count overflow validation, partial range-read cleanup on error, and mixed-case primary-key lookup in table_changes. The all-tables form remains intentionally scoped to #26885, as previously agreed.

Fresh local evidence: full container/vector, objectio, objectio/ioutil, sql/colexec, table_function, and logtailreplay tests; focused planner/frontend table_changes tests; dependent production builds; and vet for the affected packages all pass.

[gouhongshen]

Codex automated review

All blockers from my prior reviews are fixed, including schema/prepared-plan identity, range and watermark bounds, bounded spill/read cleanup, legacy/chunked publication paths, and row-only read rejection. Other reviewers’ rollout-gate, empty-window, spill net-effect, and eager-recovery blockers were addressed; all-tables is explicitly deferred to #26885. One distinct blocking correctness gap remains.

P1 - Reject chunk payloads whose decoded row count disagrees with metadata (pkg/objectio/column_chunk.go:248)

parseColumnChunkHeader validates the declared row ranges, but after Decode(chunk) this code only checks the decoded type and unions source.Length(); it never compares source.Length() with meta.rowCount or the final dst.Length() with totalRows. A valid serialized two-row vector can therefore be stored in a chunk whose otherwise-valid header declares one row, causing full constructorFactory/DecompressColumnExtent reads to return extra rows; shorter payloads can silently drop rows. ReadOneBlockAllColumns and publication consume these full vectors directly, so column lengths and exposed row counts can become inconsistent. Validate each decoded chunk length and the final aggregate length against the header, with a malformed-payload regression test. This is distinct from the already-fixed header allocation and row-overflow checks.

[gouhongshen]

Codex automated review

Previously blocking privilege, metadata identity, recovery/memory, row-only, publication, rollout, shared-varlen, cleanup, and header-validation issues are fixed or intentionally scoped; all-tables discovery remains deferred to #26885. My prior full-decoder payload-row-count blocker is fixed by a91ad6. A distinct cross-chunk logical-type validation failure remains blocking.

P1 - Validate logical type before unioning chunk payloads (pkg/objectio/column_chunk.go:262)

Author response: a91ad6 adds per-chunk row-count and final aggregate row-count validation, closing the previously reported full-decoder row-count mismatch. Why this remains blocking: decoded chunk types are still never compared before dst.UnionBatch. A structurally valid extent can contain an int64 chunk followed by an int32 chunk with matching declared row counts. vector.UnionBatch does not type-check and uses the destination element width when slicing the source, so this can panic (8-byte slice from a 4-byte source) or silently reinterpret data when widths happen to match. The bounded table_changes path is also affected because readChunkedColumnWindow unions materialized chunks at pkg/objectio/funcs.go:500 without using this decoder. This is distinct from the fixed row-count issue because every chunk can have the correct row count. Validate logical type compatibility in a shared path before every union, and add full and ranged mixed-type corruption tests.

[gouhongshen]

Codex automated review

The prior cross-chunk type-validation blocker is fixed in both full and ranged readers with regression coverage. Earlier privilege, schema, cursor, bounded-replay, cleanup, persistence, publication, rollout, and header-validation blockers are fixed or explicitly scoped; all-tables remains deferred under #26885. No current findings remain.

[aunjgr]

Reviewed exact head 61d0d66ab24d0fc27658ddb090c4920903b7d953; all CI checks are terminal and successful. The prior privilege, schema/cursor, bounded-replay, spill/cleanup, publication/rollout, shared-varlen, and chunk-validation findings remain closed. Since the last full clean approval at c5028841ab7ad6e585d418de6a7271e575dc36df, the only PR-authored change is the required encodeChunkedColumn test call-site adjustment for its updated return signature; the rest is merged main. No current finding remains.

[XuPeng-SH]

Deep-reviewed exact head 61d0d66 against current main a2f8923. The prior payload row-count and cross-chunk type-validation findings are fixed, but latest main exposes one new P1 rolling-upgrade blocker.

P1 compatibility — the chunked on-disk format now reuses an already-deployed protocol epoch. This head gates chunked persisted extents at MORPCVersion16. Current main added MORPCVersion17 in #26781, while its v16/v17 readers do not contain the Lz4Chunked decoder. The PR is correspondingly merge-conflicted in pkg/defines/const.go. If that conflict is resolved by retaining the current v16 gate, a mixed-version cluster can negotiate v16 or v17, an upgraded CN will emit MOCOLCH1 extents, and an old CN that legitimately advertises the same protocol level will try to read them as legacy LZ4 and fail. The rollout gate therefore no longer proves reader support.

Please rebase onto latest main, allocate a new protocol version dedicated to chunked persisted extents (currently the next epoch after v17), and use that constant in the live CN writer policy. Extend the mixed-version/transition regression so the pre-feature latest protocol remains legacy and only the new chunk-format epoch enables emission. This is a format compatibility requirement, not merely a textual merge-conflict fix.

I rechecked the incremental malformed-header, decoded-row-count, and mixed-logical-type changes; no additional blocker was found in that delta.