Conversation
Current benchmarking run on local laptop
Note: in all instances that return a full FeatureCollection, the entire set must be held in memory. Rust streaming in v0.10.0 stays very low constant memory. Rust streaming to geoparquet stays constant memory, but memory usage could still be high depending on row group size settings. Also, note that 911nh is still unhydrated, so any use of that path would still require the additional client side hydration, so it is not entirely apples to apples. pgstac search benchmark — bench_v0911 (0.9.11) vs bench_current (0.10), 100k items/collectionAll values are TTLR ms (lower = faster);
PAGING — total ms to fetch K pages of L items via keyset tokens, vs one streaming pass of K*L
INGEST throughput — v0.9.11 pypgstac load vs the 0.10 Rust loader (from ingest_bench.sh, 100k/coll)
|
… (squash of v010-export-search onto main) main already carries the v0.10 foundation (#438 rust crate, #448 fragments, #456 keyset). This adds the v010-export-search work on top: the pgstac-rs export/dump/restore engine (Rust loader, search_plan search, dump/export) with Python bindings + the pgstac CLI; flat-memory CLI search streaming via the server portal with fields projection; configurable parquet row-group size (builder, --row-group-size, PGSTAC_PARQUET_ROW_GROUP_SIZE env, Python search_to_geoparquet); SQL maintenance/comment cleanup; pypgstac updates. Foundation overlaps resolved to the developed v010io versions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gadomski
left a comment
There was a problem hiding this comment.
I didn't do a detailed review yet, just looked at some basic things. I've got a high level question about whether there's more stuff we can use from the stac crate (e.g. Search).
Move memprofile from a shipped [[bin]] to examples/ (it is a dev heap profiler, not a user tool). Remove the env-gated PGSTAC_LOAD_PROFILE / PGSTAC_PIPE_PROFILE / PGSTAC_DECODE_ONLY instrumentation from the ingest and parquet-pipeline hot paths. Fix Footprint::observe to read east/north at len/2 and len/2+1 so 6-element 3D bboxes are handled correctly (the old last-two logic placed north and max-elevation into east and north). Fix the stale stream_ndjson test call to pass the new token argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PgstacPool now implements stac::api::{ItemsClient, CollectionsClient, StreamItemsClient, TransactionClient}, returning stac::api::ItemCollection via a new TryFrom<Page>, so a pgstac database is a first-class rustac backend. Each method routes through the existing engine (the keyset search in crate::search and the Rust loader for writes) - the rustac-native surface over one engine, not a second implementation. Enables stac's async feature for StreamItemsClient, whose search_stream maps the crate's flat-memory portal stream into stac::api Items (constant memory, not page-by-page).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Turn the rust-crate CI job back on (it was gated off with if: false pending v0.10). Build the three template databases the tests need (clean + ingest templates and the rich fragment template) from the committed pgstac.sql and tracked fixtures via the setup scripts, then compile-check all features and run the suite with --features cli. Testing with cli rather than --all-features avoids linking libpython at runtime - the python feature is only a compile target here and has no runtime tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
items_staging_triggerfunc now raises if any staged item references a collection that does not exist, instead of silently dropping it in the collections JOIN below - matching the Rust loader, which already errors on this. Validated with pgtap + basicsql + pg_dump/restore (no regression). pgstac.sql reassembled via stageversion (unreleased, no version bump); the regenerated migrations are intentionally not committed (v0.10 migrations regenerate per-run and are excluded from the gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…role Groups the flat crate root into db/ read/ load/ json/ api/ python/ and finishes the rustac-native + cleanup pass. Precheck stays a library method, not yet wired into load. pgstac-rs: - reorg: flat root modules into db/ read/ load/ json/ api/ python/ (public API unchanged) - search CLI dedup: route the itemcollection command through stac::api ItemsClient::search; delete SearchSource and page_to_feature_collection; map bad search tokens to Error::InvalidToken - remove the profiling feature, memprofile example, and dhat dep (preserved on the archive/memprofile branch); drop the lib.rs dhat shim - drop the EXPERIMENTAL hedge from the precheck docs; comments describe current behavior only pgstac (SQL): - remove the unused pgstac_load up-privilege role (added this cycle, never released); the SECURITY DEFINER write functions remain the only write path through the privilege wall - add a pgtap proof-test that the wall holds: pgstac_ingest cannot directly INSERT/UPDATE/DELETE items, partition_stats, or item_fragments - regenerate pgstac.sql Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| #[derive(ValueEnum, Clone, Copy, Debug)] | ||
| enum CompressionArg { | ||
| Zstd, | ||
| Snappy, | ||
| Uncompressed, | ||
| } |
There was a problem hiding this comment.
This has an enum in rustac, can we use that?
| #[derive(ValueEnum, Clone, Copy, Debug, PartialEq, Eq)] | ||
| enum SearchFormat { | ||
| /// Newline-delimited JSON, one item per line (streams all pages). | ||
| Ndjson, | ||
| /// A single STAC FeatureCollection page (SQL-faithful next/prev + context). | ||
| Itemcollection, | ||
| /// stac-geoparquet of the (bounded) result. | ||
| Geoparquet, | ||
| } |
There was a problem hiding this comment.
Same as below, can we re-use the enum from rustac?
…gnore Opt-in --skip-unchanged toggle on `load` routes each batch through the per-partition precheck before loading: on re-ingest / sync it skips items already present-and-current and loads only new + changed. Benchmarks (NAIP 89.8k, release): re-ingest identical ~3.8-5.1x faster, 10%-changed ~3.3x; ~18% slower on an all-new load, so it stays opt-in. - policy-aware precheck: ignore/error skip an existing id by id alone (no content hash, and skip changed-existing too, matching ignore semantics); upsert/delsert keep the content-hash compare - error + --skip-unchanged is rejected (error must fail on an existing id, not skip it) - the survivor-count preflight bump makes partition_stats.n tighter on a precheck load (ignore exact, upsert new+changed) with no extra query and no stats-on-flush - concurrency test covering the id-only ignore path Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
This was a API-level review, meaning that I looked through the publicly exposed functions (via the documentation). I have not looked through the function bodies, except when I found myself digging deeper.
There's a lot of free functions that feel like they could/should be methods provided by the Pgstac trait or the Client struct, since they take a client as their first argument?
Also, Claude flagged up a couple of issues:
- is broken as
pgstac/src/pypgstac/src/pypgstac/load.py
Line 480 in 4400bfd
update_partition_stats_qdoesn't exist anymore - Is it intentional that
partition_statsisn't backfilled during a migration? - Is
indexes_pendingused anywhere to build the queryable indexes?
| #[derive(Debug, Clone)] | ||
| pub struct WkbGeometry(pub Vec<u8>); |
There was a problem hiding this comment.
Duplicative on the geometry from the geom module?
| #[derive(Debug, Clone)] | ||
| pub struct ConnectConfig { | ||
| /// A full connection string (libpq keyword/value or `postgresql://` URL). When set it is the base | ||
| /// configuration; the individual fields below still override or augment it. | ||
| pub dsn: Option<String>, | ||
| /// Database host (`PGHOST`). | ||
| pub host: Option<String>, | ||
| /// Database port (`PGPORT`). | ||
| pub port: Option<u16>, | ||
| /// Database name (`PGDATABASE`). | ||
| pub dbname: Option<String>, | ||
| /// User (`PGUSER`). | ||
| pub user: Option<String>, | ||
| /// Password (`PGPASSWORD`). | ||
| pub password: Option<String>, | ||
| /// Extra libpq `options` startup string (`PGOPTIONS`); the search path is merged into it. | ||
| pub options: Option<String>, | ||
| /// Application name (`PGAPPNAME`); defaults to [`DEFAULT_APPLICATION_NAME`]. | ||
| pub application_name: Option<String>, | ||
| /// Connection timeout in seconds (`PGCONNECT_TIMEOUT`). | ||
| pub connect_timeout: Option<u64>, | ||
| /// SSL mode (`PGSSLMODE`): `disable`, `allow`, `prefer` (the default), `require`, `verify-ca`, or | ||
| /// `verify-full`. | ||
| pub sslmode: Option<String>, | ||
| /// Path to a trusted CA certificate (`PGSSLROOTCERT`); consumed by the TLS connector. | ||
| pub sslrootcert: Option<String>, | ||
| /// Path to a client certificate (`PGSSLCERT`); consumed by the TLS connector. | ||
| pub sslcert: Option<String>, | ||
| /// Path to a client private key (`PGSSLKEY`); consumed by the TLS connector. | ||
| pub sslkey: Option<String>, | ||
| /// The search path applied at connection startup. Defaults to [`DEFAULT_SEARCH_PATH`]. | ||
| pub search_path: String, | ||
| } |
There was a problem hiding this comment.
This feels really duplicative on stuff that already exists for postgres, do we really need it?
| /// A page of search results. | ||
| #[derive(Debug, Deserialize, Serialize)] | ||
| pub struct Page { | ||
| /// These are the out features, usually STAC items, but maybe not legal STAC | ||
| /// items if fields are excluded. | ||
| pub features: Vec<Item>, | ||
|
|
||
| /// The next id. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub next: Option<String>, | ||
|
|
||
| /// The previous id. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub prev: Option<String>, | ||
|
|
||
| /// The search context. | ||
| /// | ||
| /// This was removed in pgstac v0.9 | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub context: Option<Context>, | ||
|
|
||
| /// The number of values returned. | ||
| /// | ||
| /// Added in pgstac v0.9 | ||
| #[serde(rename = "numberReturned", skip_serializing_if = "Option::is_none")] | ||
| pub number_returned: Option<usize>, | ||
|
|
||
| /// Links | ||
| /// | ||
| /// Added in pgstac v0.9 | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub links: Vec<Link>, | ||
|
|
||
| /// Additional fields. | ||
| #[serde(flatten)] | ||
| pub additional_fields: Map<String, Value>, | ||
| } |
There was a problem hiding this comment.
Is this duplicative on SearchPage?
The precheck path takes `&PgstacPool`, so it only compiles with the `pool` feature, but CI only ran `cargo check --all-features` — so the no-`pool` build was broken (E0425: cannot find `PgstacPool`). Gate the precheck items and their helpers behind `#[cfg(feature = "pool")]`, gate the two export-only integration tests, and replace the single all-features check with a feature-matrix check (no-default / pool / export / cli / all-features) so the break cannot recur. Per the #458 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… in CI cargo doc emitted 5 broken-intra-doc-link warnings — links to private / non-Rust items (precheck_one_partition, hydrate_fragment_core, PgConn, stac_daterange) become plain code, and GenericClient links to its real tokio_postgres path. Also refresh the crate features list (pool/export/cli/python; the old `tls` entry was stale) and the export module wording. Add a RUSTDOCFLAGS=-D warnings `cargo doc` CI step so doc warnings cannot regress. Per the #458 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The SQL comments called jsonb_canonical/jsonb_hash "RFC 8785-aligned" and pointed external implementers at the `rfc8785` crate / a generic RFC 8785 canonicalizer — but the actual form is a custom canonicalization (byte-order keys via COLLATE "C" + `float8::text` numbers) that is NOT RFC 8785 and would hash differently. Correct the comments so nobody reimplements the hash against the wrong spec. The Rust side already documented this correctly, and parity is CI-gated by tests/canonical_parity.rs. Comment-only; pgstac.sql regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…med constants - CLI --policy uses `ConflictPolicy` directly (cli-gated `ValueEnum` derive), dropping the duplicate `LoadPolicy` enum + its `From` impl; `delsert` is now also a CLI policy. - The Int queryable dehydrate uses `i32::try_from` (erroring) instead of `as i32`, so an out-of-range integer property fails loudly instead of silently truncating. - `DEFAULT_BATCH_SIZE` / `MAX_INGEST_PARALLELISM` constants replace the inline 5000 / 8. Per the #458 review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…async sweep New partitions are created index-light (indexes_pending=true) for fast ingest, but nothing consumed the flag. Wire it end-to-end: - the queryables trigger now flags every partition (SECURITY DEFINER, so it works for pgstac_ingest past the partition_stats write wall) instead of rebuilding all partitions synchronously in the trigger; - a new build_pending_indexes(_limit) maintenance sweep builds the missing per-partition queryable indexes and clears the flag, mirroring tighten_dirty_partition_stats (schedule via pg_cron / the maintenance CLI). Per the #458 review (indexes_pending was set but never consumed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CREATE DATABASE ... TEMPLATE copies the schema but not the per-database settings (pg_db_role_setting) of the template, so each cloned per-test DB lost the search_path = pgstac, public the template sets — and every Pgstac-trait test that uses an unqualified pgstac reference failed in CI (pgstac_version etc. passed because they are qualified). Re-apply the search_path on the clone. Full cargo test --features cli is green, matrix + docs clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Having both the `Pgstac` trait (a bare-connection trait returning `Page`/`JsonValue`) and `Client`/`PgstacPool` (the `stac::api` traits returning stac types) was two public ways to do the same thing. The trait has no external consumers except rustac, so remove it entirely: - low-level SQL-call helpers move to `db::call` (pub(crate) free fns); the 3 unused ones (pgstac_vec/opt/value) are dropped - version/settings/collection-maintenance/delete_item become inherent `Client` methods; search/item/collections/collection/add_collection stay on the `stac::api` impls - `ItemsClient::search` folds in the malformed-token to InvalidToken mapping and uses the existing `TryFrom<Page>` instead of a hand-rolled ItemCollection rebuild (also carrying number_matched/number_returned/links the old code dropped) - the pool search/collections impls delegate to `Client` (one impl, shared cache) - `Pgstac` and `Page` leave the public API; the crate docs/examples lead with `Client`; the ~80 test sites move onto the Client surface - net -140 lines Engines (ingest/search/stream/hydrate) are untouched; a pre/post A/B shows no perf regression and streaming is unchanged. Follow-up (separate rustac PR): migrate rustac crates/server/src/backend/pgstac.rs from the `Pgstac` trait to the `stac::api` traits on `PgstacPool`/`Client`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`ConnectConfig::from_env` reads `PGDATABASE` (and `PGHOST`/`PGPORT`/...) into the config, and `to_pg_config` applied those over a later-set `--dsn` — so `pgstac load --dsn <db>` connected to `PGDATABASE` instead of the database the DSN named. In CI (PostgreSQL 18, `PGDATABASE=postgis`) the CLI load tests failed with `function upsert_collection(jsonb, text) does not exist`, because the CLI connected to `postgis` (no pgstac) rather than the cloned test database. Locally it was masked by an empty `PGDATABASE` and a pgstac-first cluster `search_path`. Add `ConnectConfig::with_dsn`, which makes an explicit DSN the authoritative connection target (clearing host/port/dbname/user/password from the environment while keeping TLS/options/search_path), and route the CLI `--dsn` flag through it. Verified on a local PG18 container with `PGDATABASE=postgis`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gadomski
left a comment
There was a problem hiding this comment.
- Some comments from my previous PR review need to be addressed, e.g. around enum re-use and using some stac structures
- Got a local error running the test
canonical_and_hash_match_sql:function pgstac.jsonb_canonical_hash(jsonb) does not exist
| /// Postgres connection string. Defaults to $PGSTAC_DSN or a local dev DSN. | ||
| #[arg(long, env = "PGSTAC_DSN")] | ||
| dsn: Option<String>, | ||
|
|
There was a problem hiding this comment.
You can use global = true in arg to share dsn among all the argument structures, e.g. https://github.com/stac-utils/rustac/blob/main/crates/cli/src/lib.rs#L43C1-L57C34
| /// Item id. Given, deletes that item; omitted, deletes the whole collection (and its items). | ||
| #[arg(long)] | ||
| item: Option<String>, | ||
| } |
There was a problem hiding this comment.
Since delete is a dangerous operation, should we add a confirmation prompt by default that could be disabled with a boolean flag?
| /// value (unlimited) is treated as no cgroup limit. | ||
| fn cgroup_memory_limit() -> Option<u64> { | ||
| // cgroup v2 | ||
| if let Ok(s) = std::fs::read_to_string("/sys/fs/cgroup/memory.max") { |
There was a problem hiding this comment.
This feels pretty gnarly — does it work on non-linux systems?
| fn put(&self, rel_path: &str, bytes: &[u8]) -> Result<FileWritten>; | ||
|
|
||
| /// Writes a finished large file by streaming it from a local path. | ||
| fn put_file(&self, rel_path: &str, src: &Path) -> Result<FileWritten>; |
There was a problem hiding this comment.
| fn put_file(&self, rel_path: &str, src: &Path) -> Result<FileWritten>; | |
| fn put_from_path(&self, rel_path: &str, src: &Path) -> Result<FileWritten>; |
| // object_store sink (local dir + S3/GCS/Azure) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[cfg(feature = "cli")] |
There was a problem hiding this comment.
Since this doesn't have to be just for the CLI, to me this should be behind an object_store (or store) feature, and then the cli feature can depend on the object_store feature.
| pub struct ObjectStoreSink { | ||
| store: Arc<dyn ObjectStore>, | ||
| base_prefix: ObjPath, | ||
| runtime: tokio::runtime::Handle, |
There was a problem hiding this comment.
Why does this own a runtime? Can we just make the methods async on the trait?
Major reworking of io functions and tooling for pgstac.