Build TargetDB on pfs.utils.database.db.DB - #156
Conversation
TargetDB was a hand-rolled SQLAlchemy wrapper carrying real debt: reset_*
passed raw strings to session.execute() and had been broken since SQLAlchemy
2.x, insert_by_copy/fetch_by_copy used psycopg2-only cursor APIs, and bulk
inserts went through the ORM despite fluxstd/sky reaching millions of rows.
Subclass pfs.utils.database.db.DB instead, which brings a process-wide engine
cache, pool_pre_ping and COPY-based bulk insert. Only what DB lacks is
implemented locally: a password and explicit dialect in the URL, update(),
close(), and the dry_run= keyword backing the CLI's --commit default.
Two overrides are load-bearing:
* connect() returns None. DB.connect() hands back a pooled Connection and
every caller here and downstream discards it, which would leak one
checked-out connection per call.
* connection() begins its transaction eagerly and rolls back under dry-run.
The eager begin matters because pandas.to_sql starts *and commits* a
transaction of its own when given a connection not already in one, which
would otherwise defeat the rollback and silently break --commit.
Standardise on psycopg3. normalize_drivername() rewrites the bare "postgresql"
dialect to postgresql+psycopg at the single point every path passes through,
so no deployed config file needs editing. Drop psycopg2-binary and raise
requires-python to >=3.12 (pfs-utils needs it); CI already installs 3.12.
Make the password optional: when absent it is omitted from the URL and libpq
resolves it via PGPASSWORD or ~/.pgpass, so a config file with no secrets can
be version controlled.
Also in this change:
* inserts drop DataFrame columns the table lacks. add_backref_values()
merges whole reference tables in to resolve foreign keys, leaving columns
like partner_name behind; bulk_insert_mappings() ignored unmapped keys and
COPY does not.
* fill in group_id for 8 rows of examples/data/proposals.csv. The column is
NOT NULL and the values were blank; psycopg2 stored the literal string
"NaN" there, which COPY correctly rejects as a null.
* fix the tbls DSN. Replacing the scheme on the rendered string produced
"postgres+psycopg://", which tbls cannot parse; swap the drivername on the
URL object instead.
* close three connection leaks where db.close() sat outside try/finally.
* alembic env.py now builds its URL from the TOML file named by
TARGETDB_CONF, falling back to alembic.ini. Credentials live in one place
and deployments pick up psycopg3 without hand-edited ini files.
config.set_main_option() is avoided because ConfigParser reads "%" as an
interpolation marker and would mangle passwords containing it.
* track alembic/pfsa-db01-gb-dev/ and add credential-free alembic.ini files,
so every deployment directory is runnable from a fresh checkout.
* lint and format the alembic env.py scripts. The previous blanket exclusion
of alembic/ was broader than its own rationale, which only covers the
historical revision scripts under versions/.
Removed: reset_all, reset_target, reset_fluxstd, reset_sky, insert_by_copy,
fetch_by_copy, insert_mappings, rollback. None are called from
ets_target_database, ets_pointing or pfs_obsproc_planning_tools. insert() no
longer takes return_defaults. connect, close, fetch_query, fetch_all and
fetch_by_id are kept unchanged for downstream callers.
Tests: new unit coverage for URL construction and the API surface, plus
integration tests proving the COPY dry-run rollback against a real server,
the absence of connection leaks, and TARGETDB_CONF-driven alembic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l password
Follow-ups to the pfs-utils migration, found while auditing the branch for
leaked credentials. No real credential was committed: every password-like
string in tracked files is a placeholder, and all URL logging goes through
render_as_string(), which masks it.
The audit did turn up three problems:
* draw_diagram() raised KeyError on a config with no password. Making the
password optional left the SchemaCrawler path indexing it directly, so the
very configs that change enables would crash. Omitting --password is safe:
pgjdbc resolves the password from PGPASSFILE, or from ~/.pgpass via the
JVM's user.home. Verified against a live server -- a password-less config
plus PGPASSFILE produces a diagram, a wrong password in that file fails
authentication, and no file at all reports that none was supplied.
The password is now appended after the debug log rather than before, so it
is no longer written to the log verbatim.
* examples/docker/db-data/ is a live 48MB PostgreSQL cluster that was neither
tracked nor ignored, so `git add -A` would have committed it. It holds no
role secrets today (the example cluster uses trust auth), but global/ would
carry a SCRAM verifier the moment one is set. Now ignored, along with the
alembic run logs and local tool state.
* a comment claimed DB.__init__ logs self.url; it logs only the database
name.
The four alembic note files are consolidated into alembic/README.md. The two
deployment READMEs were byte-identical, and local_test/ held only the stock
one-liner. Their 2022 debugging material is kept as lessons rather than a log:
the duplicate-row SQL is generalised and paired with the UniqueViolation it
explains, and memo.md yields a gotcha worth keeping -- SQLAlchemy cannot
reflect expression-based indexes, so autogenerate is blind to every q3c index
in this schema. Stale paths, row counts and Python 3.9 tracebacks are dropped;
git history still has them.
Finally, remove requirements.txt. Nothing referenced it -- not CI, Docker or
the docs -- and it had drifted in both directions: it still listed
psycopg2-binary and tomli, and was missing pfs-utils, psycopg and requests.
Anyone trusting it would have installed the driver this branch removed. The
pdm gen-requirements script that regenerates it goes too; the rest of the PDM
setup is left for a separate change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@wtgee I've tried to use |
|
@wanqqq31 Could you please test the integration code as is with the |
wtgee
left a comment
wtgee
left a comment
There was a problem hiding this comment.
I guess you need to maintain psycopg2 compatibility? All of this looks fine. We could move some of these features into pfs_utils directly: password ability; dry run support; dropping unknown columns, etc. I'm mostly putting that here for my own mental note as it is beyond the scope of this PR.
Note that I only reviewed src/targetdb/targetdb.py and src/targetdb/utils.py but LGTM.
I dropped pyscopg2 support as all dependent packages can be used with psycopg3, and both 2 and 3 versions can co-exist if needed. Also, packages using targetdb can also work with psycopg3, I suppose. For additional features for |
Got it, I see you are just keeping compatibility with the old config files.
Good idea, I'll make a ticket. We purposefully left password support out but I can see needing it in the real world. You have a host set in your config files but it is probably worthwhile to set sane defaults otherwise (maybe I missed it somewhere), like: https://github.com/Subaru-PFS/pfs_utils/blob/master/python/pfs/utils/database/opdb.py#L13-L16 |
TargetDB.__init__ carried non-None defaults (host="localhost", etc.) and raised ValueError when dbname/user were omitted, so the class-level DEFAULT_HOST/DEFAULT_USER/DEFAULT_DBNAME/DEFAULT_PORT that DB.__init__ resolves omitted arguments from were unreachable, and the inherited set_default_connection() classmethod was a silent no-op. Bring TargetDB in line with the OpDB/QaDB convention in pfs.utils.database.db: declare the four DEFAULT_* attributes and default every __init__ parameter to None. DEFAULT_USER is the read-only `obsproc` account rather than the `pfs` that OpDB/QaDB use, since every write path in this package already passes an explicit, privileged user from a config file -- a bare TargetDB() can now read production but never write to it. Confirmed against the live database that obsproc has SELECT-only grants and that INSERT is rejected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for taking the time to make it compatible! I ❤️ code consolidation! |
…tring dtype pandas' own default string dtype (the default from pandas 3.0, and available earlier via future.infer_string) uses its own NA sentinel, so assigning None into a filter_* column with that dtype silently rewrote it back to NaN -- defeating the point of the function, since a NaN reaching the database driver gets coerced to the literal string "NaN" and violates the filter_name foreign key. Casting the column to object dtype before the assignment keeps None as a real None regardless of which string dtype pandas resolved the column to. Found while chasing a test failure that turned out to be caused by running against pandas 3.0.5 in an ad hoc virtualenv instead of the pandas 2.3.3 this project's uv.lock pins -- the project's own pyproject.toml has no upper bound on pandas, so the same failure would have hit real inserts once pandas 3 was eventually resolved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Replaces the hand-rolled
TargetDBSQLAlchemy wrapper with a subclass ofpfs.utils.database.db.DB, and standardises on psycopg3.The old wrapper had accumulated real debt:
reset_*passed raw strings tosession.execute()and had been broken since SQLAlchemy 2.x,insert_by_copy/fetch_by_copyused psycopg2-only cursor APIs, and bulk inserts went through the ORM despitefluxstd/skyreaching millions of rows.DBsupplies an engine cache,pool_pre_pingandCOPY-based bulk insert; only what it lacks is implemented locally — a password and explicit dialect in the URL,update(),close(), and thedry_run=keyword behind--commit.Two overrides are load-bearing
connect()returnsNone.DB.connect()hands back a pooledConnection, and every caller here and downstream discards it — inheriting it would leak one checked-out connection per call.connection()begins its transaction eagerly.pandas.to_sqlstarts and commits a transaction of its own when given a connection not already in one, which would silently defeat the dry-run rollback and break the--commitdefault.tests/integration/test_dry_run.pyproves the rollback against a real server, including the raw-cursorCOPYpath.Other changes
normalize_drivername()rewrites the barepostgresqldialect at the single point every path passes through, so no deployed config file needs editing.psycopg2-binarydropped;requires-pythonraised to>=3.12(CI already installs 3.12).PGPASSWORD/~/.pgpass, letting a secret-free config be version controlled.env.pybuilds its URL fromTARGETDB_CONF, falling back toalembic.ini.config.set_main_option()is avoided because ConfigParser reads%as an interpolation marker.alembic/pfsa-db01-gb-dev/is now tracked, and every deployment directory has a credential-freealembic.ini.env.pyscripts are linted; the previous blanket exclusion ofalembic/was broader than its own rationale, which only covers the revision scripts.alembic/README.md.requirements.txtremoved — unreferenced, and drifted in both directions.Two things reviewers should know
examples/data/proposals.csvhad 8 rows with a blankgroup_id, a NOT NULL column. Verified experimentally that psycopg2 silently stored the literal string"NaN"there;COPYcorrectly rejects it as a null. The CSV is fixed, but the productionproposaltable likely still holds those 8 rows withgroup_id = 'NaN'.alembic/local_test/'s revision chain is broken and predates this work:80f8276e2ee7names adown_revisionthat is not in the repository. Its integration test therefore asserts on the connection rather than the exit status.Removed API
reset_all,reset_target,reset_fluxstd,reset_sky,insert_by_copy,fetch_by_copy,insert_mappings,rollback— none called fromets_target_database,ets_pointingorpfs_obsproc_planning_tools.insert()no longer takesreturn_defaults.connect,close,fetch_query,fetch_allandfetch_by_idare unchanged for downstream callers.Testing
81 passed, 1 skipped. The pre-existing integration suite passes unmodified — the main evidence the migration is sound — plus new coverage for URL construction, dry-run rollback, connection-leak regressions,
fetch_allcolumn order, andTARGETDB_CONF.tblsdiagram generation verified against a container, and SchemaCrawler verified end-to-end with a password-less config.Downstream verification of
ets_pointing/pfs_obsproc_planning_toolsis out of scope; test 4 reproduces their call patterns in-repo.🤖 Generated with Claude Code