Repository navigation
Documentation update - #104
Conversation
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
b8f3002 to
6fd0291
Compare
matt-welch
left a comment
There was a problem hiding this comment.
Thanks Kelly, this is a big improvement. I checked the changed statements against the source and most of them are accurate now. Links all resolve, and build plus the full TAP and regression suites (including the hardware tests) pass on this branch. A few things to fix before release:
Setup steps fail at enrollment. vamana_databases only exists in a database where CREATE EXTENSION svs has been run. docs/build_guide/README.md line 86 says to create the extension in each database that hosts indexes, then line 88 says to INSERT into vamana_databases in the launcher database (default postgres), but nothing says to create the extension there. On a fresh cluster that INSERT fails with ERROR: relation "vamana_databases" does not exist. README.md line 38 also says to enroll before the CREATE EXTENSION svs block. Please add CREATE EXTENSION vector; CREATE EXTENSION svs; in the launcher database as an explicit step, and put the README steps in order.
The new hint in src/vamanabuild.c line 848 doesn't show up for real build failures. SVS errors during the build are raised earlier by CheckSVSError in src/svs_wrapper.c as SVS operation failed: dynamic index build, with no hint. The failed to build SVS index path only runs when SVS returns no index without reporting an error, and then the logged error code is 0. I tried two out-of-range alpha builds and both showed the SVS operation failed error with no hint. If we want users to see this hint, it needs to go on the CheckSVSError path for the dynamic build.
Smaller doc corrections:
- docs/dev/ARCHITECTURE.md line 338:
hot_standby_feedbackdoesn't make the primary keep WAL. It pins the primary's catalog_xmin so the catalog rows the standby's logical slot reads aren't vacuumed (see the comment at src/vamanaworker.c line 1599). WAL retention comes fromprimary_slot_name. - README.md line 64: the all-in-one build doesn't set
SVS_INSTALLorPG_CONFIGin the caller's shell (the config variable isSVS_INSTALL_DIR). The PostgreSQL it builds also has no--enable-tap-tests, somake prove_installcheckwon't run against it. - docs/USER_GUIDE.md lines 1170 and 1174: writes aren't serialized per index. One worker serves every Vamana index in a database, so splitting into partitions in the same database won't spread write load. ARCHITECTURE 8.4 already says this correctly.
- README.md line 27 and USER_GUIDE lines 67, 104 and 1110 to 1113 say LVQ/LeanVec need an Intel CPU, but the hint users get (src/svs_wrapper.c line 89) says "hardware with the required CPU instructions". Please make the docs and the hint say the same thing.
- docs/USER_GUIDE.md line 1226 still describes the
search_window_sizereloption as the "Default query window". Section 4.2 now says it isn't used. - docs/USER_GUIDE.md line 662: inserting another database's name into
vamana_databasesfrom a non-launcher database succeeds with no error, and the row is ignored. Only enrolling the current database from a non-launcher database is rejected. Suggested wording: "Enrollment must run in the launcher database. Rows written tovamana_databasesin any other database have no effect."
Nits:
- docs/build_guide/README.md lines 78 and 82 list
max_wal_sendersas required, but nothing in the extension reads it. - ARCHITECTURE.md line 457 says the LOAD wait has no fixed timeout. It waits up to
svs.worker_startup_timeout_ms. - BGW_LAUNCHER.md line 46 says the worker owns "that database's replication slot". Slots are per index.
- docs/RESOURCE_SIZING.md lines 25 and 26 (not in this PR) still say
CREATE EXTENSION svsinstallsvectorautomatically.
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
matt-welch
left a comment
There was a problem hiding this comment.
Thanks Kelly, this addresses everything from my last review. I ran the new setup steps on a fresh cluster with only the build guide's settings and they work, in both the README order and the build guide order. One README problem is left, plus a small doc fix and a few nits.
The Quick Start never creates mydb. README.md lines 41 to 46 enroll mydb from the launcher database, and line 49 then works in mydb, but nothing creates it. Run in order on a fresh cluster, the enroll step fails with ERROR: database "mydb" does not exist. Please add CREATE DATABASE mydb; before the launcher block, and the same before the enroll step in docs/build_guide/README.md (line 96).
docs/USER_GUIDE.md line 169 still says writes are serialized per index. Like the 9.15 fix, it should say per database, since one worker serves every Vamana index in a database.
Nits:
- README.md line 101 says the hardware tests need "AVX-512 hardware". Since compression needs an Intel CPU (as line 27 says), "an Intel CPU with AVX-512" would be accurate.
- docs/USER_GUIDE.md lines 1278 and 1279: simple mode turns on for a positive value, not whenever the GUC is set, so 0 also means off. ARCHITECTURE.md lines 726 and 727 already say "positive value".
- docs/USER_GUIDE.md line 67 still has the "AVX-512 optimized" bullet that was removed from the README.
The two CI failures on this PR come from main, not from these changes. We're fixing them in a separate PR, and nothing here needs to change for them.
Signed-off-by: klmckeig <kelly.l.mckeighan@intel.com>
matt-welch
left a comment
There was a problem hiding this comment.
Thanks Kelly, this covers everything. I ran the README Quick Start exactly as written on a fresh cluster and it works end to end: it creates and enrolls mydb, builds an index and returns results. The other fixes all check out.
One optional thing for later, not blocking: in docs/build_guide/README.md, line 84 says to create the extensions in each database that hosts indexes, but mydb isn't created until line 96, and the section ends without creating the extensions in mydb. Moving that sentence after the enroll step, as "Then, in mydb, run ...", would make it read in order.
The CI failures here come from main, and we're fixing them separately. Approving.
Description
Documentation pass for the first release (v0.1.0), plus two message strings and one comment changed in
src/. No new code and no behavior changes.installcheck,prove_installcheck, then a hardware dependent section (installcheck-hw,prove_installcheck-hw). The commands useSVS_INSTALLandPG_CONFIG.shared_preload_libraries,wal_level = logical, sender and slot settings,CREATE EXTENSION vector, and database enrollment.CREATE EXTENSIONprerequisite.search_window_sizereloption, TRUNCATE behavior, restart and pause behavior, error messages, parameter ranges and others. The checkpoint trigger rows now say "Simple" instead of "Legacy".security.md, which differed fromSECURITY.mdonly in link text and collided on case insensitive filesystems. Updated a stale comment inci/build_and_test.sh.src/(message strings and a comment only):search_window_sizereloption description now says it is not used andsvs.search_window_sizegoverns.maintenance_work_mem, which has no effect on builds. It now points to the server log andsvs.max_build_memory.VamanaResolveSearchWindowSizeinvamana.hnow says the reloption is not currently used.Related Issues
None.
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
make installcheck) and TAP tests (test/t/) pass with no failurestest/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)Documentation
docs/updated if architecture or usage changedTesting Notes
This PR adds no new code and no new tests. The only changes under
src/are two message strings (a reloption description and an error hint) and one comment. No test file references the old text.docs/**/*.md: no broken links.src/, theMakefileandsvs.control.leanvec_dimsand the secondary at the full dimension). This is a static check of the layout, not a measured index size.🤖 Generated with Claude Code