Skip to content

Fix pg_index per-column vectors to have one entry per key column - #3150

Open
zachmu wants to merge 1 commit into
mainfrom
zachmu/issue3110
Open

Fix pg_index per-column vectors to have one entry per key column#3150
zachmu wants to merge 1 commit into
mainfrom
zachmu/issue3110

Conversation

@zachmu

@zachmu zachmu commented Aug 20, 2026

Copy link
Copy Markdown
Member

Fixes pg_index per-column vectors (indoption, indcollation, indclass) to have one entry per key column, and populates indnkeyatts.

Fixes #3110.

Fixes #3110: pg_index.indoption was hardcoded to a single-element
vector regardless of how many columns the index has. It now emits one
entry per key column (matching indkey's cardinality). Dolt indexes are
always stored ascending and don't record per-column direction, so every
entry is 0 (ASC NULLS LAST, the btree default).

Also fixes the neighboring per-column vectors and count that had the
same bug:
- indcollation and indclass were empty vectors; they now contain one
  zero OID per key column
- indnkeyatts was hardcoded to 0; it now equals indnatts (INCLUDE
  columns aren't supported, so all index columns are key columns)
@itoqa

itoqa Bot commented Aug 20, 2026

Copy link
Copy Markdown

Ito QA test results
Commit: c0f57c6: 14 test cases ran, 13 passed ✅, 1 additional finding ⚠️.

Summary

The run broadly covered index metadata integrity across single-column, composite, primary, unique, partial, ordinary, and newly created indexes, including identity, column counts, ordering, filtering, and related metadata. It also exercised schema-qualified lookups, joined catalog views, refresh behavior, boundary conditions, and concurrent-read resilience, with the normal and edge-case behaviors generally remaining healthy.

Safe to merge — the only failure is a medium-severity, pre-existing concurrency defect involving catalog reads and cache collision handling, and it is not attributable to this PR. The change shows no regressions or new failures, so the unrelated server-panic finding is a flag for later rather than a merge blocker.

Tests run by Ito

View full run

Result Severity Type Description
General One-column and two-column indexes report matching key and option counts. Each key has one default ordering value of zero.
General Single-column and composite indexes report matching total and key-column counts, and the counts stay correct through a relation join.
General Catalog joins kept each schema, table, index, and metadata vector together. Direct lookups returned the same rows for both similarly named tables.
General A newly created two-column index showed the same identity and key details in both catalog views. The key count and column mapping stayed stable after the catalog refresh.
Catalog Primary, unique, and partial indexes stayed linked to the right table. Their unique and primary markers, key mappings, and partial filter were all returned correctly.
Catalog Creating a regular index keeps the correct index and table links, maps to its one indexed column, and leaves the partial-index filter empty.
Index A two-column index reports two key positions and two default ordering values, both set to zero.
Index A single-column index reports one key, one default ordering value of zero, and an indoption length of one.
Keys A two-column index was created and its catalog row reported two total columns and two key columns.
Keys A single-column index reports one total attribute and one key attribute, as expected.
Lookup Looking up the primary index returned one matching record. Looking up the table returned both its primary and ordinary indexes with the expected IDs and column positions.
Oid A two-column index returned two zero values for both collation and operator-class metadata. Formatting the values produced 0,0 for each vector, as expected.
Oid Unnesting the index metadata returned two zero values for collation and two zero values for operator class, matching the two columns in the index.
⚠️ Medium severity General The expected result was two complete, equivalent catalog results. Instead, session A returned 60 rows for three index identities, session B returned zero rows, and the server reported an all-OIDs-have-been-taken panic while populating shared catalog state.
Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟡 Concurrent catalog reads can return no rows
  • Severity: Medium Medium severity
  • Description: The expected result was two complete, equivalent catalog results. Instead, session A returned 60 rows for three index identities, session B returned zero rows, and the server reported an all-OIDs-have-been-taken panic while populating shared catalog state.
  • Impact: When concurrent catalog reads hit the cache collision, one user may receive no index information and the server may stop that catalog operation. Other database data is not shown to be lost or corrupted.
  • Steps to Reproduce:
    1. Create a table with several indexes.
    2. Start two local database sessions and repeatedly query pg_catalog.pg_index from both sessions, using both full scans and indexed lookups.
    3. Compare the rows returned by both sessions and inspect the server output if one session returns no rows.
    4. Force or encounter an OID checksum collision while the catalog cache is being populated; the retry path repeats the occupied OID and the server reaches the all-OIDs-have-been-taken panic.
  • Stub / mock content: PostgreSQL authentication was disabled in the local test service so passwordless catalog queries could run. No application mocks or route interceptions were used, and this bypass does not affect OID allocation or catalog cache behavior.
  • Code Analysis: The failure is supported independently of the unavailable browser re-run. In server/tables/pgcatalog/pg_index.go, cachePgIndexes builds each catalog index record and calls id.Cache().ToOID for both index and table IDs at lines 334-342. That path reaches core/id/cache.go. ToOID computes an initial CRC32 OID at lines 65-71, then allocates modifiedBytes and updates modifiedBytes[0] in the collision loop at lines 75-78. However, line 79 hashes underlyingBytes again instead of modifiedBytes, so every retry tests the same occupied checksum. The final fallback at lines 87-92 also checks the stale oid variable rather than candidate i, so it does not actually scan for a free OID. A collision can therefore fall through to panic("all OIDs have been taken") at line 95, aborting catalog cache construction and explaining the empty concurrent result. The smallest practical fix is to hash modifiedBytes at line 79 and use i for the cache lookup and assignment in the fallback loop. The PR diff does not modify core/id/cache.go or the ToOID call sites, so this is a pre-existing defect rather than a regression caused by the vector-shape changes.
Evidence Package

Tip

Reply with @itoqa to send us feedback on this test run.

@github-actions

Copy link
Copy Markdown
Contributor
Main PR
Total 42090 42090
Successful 18981 18980
Failures 23109 23110
Partial Successes1 5461 5461
Main PR
Successful 45.0962% 45.0938%
Failures 54.9038% 54.9062%

${\color{red}Regressions (1)}$

subselect

QUERY:          select count(*) from tenk1 t
where (exists(select 1 from tenk1 k where k.unique1 = t.unique2) or ten < 0);
RECEIVED ERROR: timeout during Receive

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@coffeegoddd

Copy link
Copy Markdown
Contributor

@zachmu DOLT

read_tests from_latency_median to_latency_median is_faster
covering_index_scan_postgres 2.43 2.43 0
groupby_scan_postgres 75.82 75.82 0
index_join_postgres 2.22 2.22 0
index_join_scan_postgres 1.61 1.58 0
index_scan_postgres 484.44 484.44 0
oltp_point_select 0.36 0.37 0
oltp_read_only 6.32 6.32 0
select_random_points 0.7 0.7 0
select_random_ranges 1.01 1.01 0
table_scan_postgres 484.44 493.24 0
types_table_scan_postgres 1235.62 1235.62 0
write_tests from_latency_median to_latency_median is_faster
bulk_insert 0.001 0.001 0
oltp_delete_insert_postgres 6.67 6.67 0
oltp_insert 3.3 3.36 0
oltp_read_write 13.22 13.22 0
oltp_update_index 3.55 3.55 0
oltp_update_non_index 3.25 3.25 0
oltp_write_only 6.91 6.91 0
types_delete_insert_postgres 7.17 7.17 0

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.

pg_index missing indoption data

2 participants