feat: init version in postgres driver only if not set #11373

hfhchan-plb · 2025-03-30T09:19:41Z

Description of change

Fixes #11372

Pull-Request Checklist

Code is up-to-date with the master branch
npm run format to apply prettier formatting
npm run test passes with this change
This pull request links relevant issues as Fixes #0000
There are new or updated unit tests validating the change
Documentation has been updated to reflect this change
The new commits follow conventions explained in CONTRIBUTING.md

Summary by CodeRabbit

Bug Fixes
- Optimized PostgreSQL driver connection initialization to skip redundant metadata fetches when values are already cached, improving connection performance and resource efficiency.

coderabbitai · 2025-11-14T09:28:27Z

Walkthrough

Optimizes PostgreSQL driver initialization by adding a guard condition that skips metadata fetches (version, current database, search/schema) when these values are already populated. Query runner resource management remains tied to the guarded block.

Changes

Cohort / File(s)	Change Summary
Metadata fetch optimization `src/driver/postgres/PostgresDriver.ts`	Added guard condition to skip unnecessary initial metadata fetches when values are already set; maintains proper resource cleanup within guarded block

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Verify guard condition logic correctly identifies when all three metadata values are missing
Confirm query runner resource release remains properly scoped to guarded block
Check for any edge cases where partial metadata state could cause issues

Poem

🐰 A rabbit hops through the code with glee,
"Why fetch what we've already seen, you see?"
With guards in place, we skip the dance,
Resources freed—a swift advance! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: Init version in postgres driver only if not set' is specific and directly describes the main change—adding conditional logic to initialize the version only when not already set.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/driver/postgres/PostgresDriver.ts (1)
365-365: Consider documenting when metadata values would be pre-initialized.

The guard condition assumes version, database, or searchSchema might already be set before connect() is called. From the constructor (lines 323-328), only database and schema can be initialized from connection options. It's unclear when version or searchSchema would be pre-set outside of a previous connect() call.

Consider adding a comment explaining the lazy initialization rationale:
+        // Lazy initialization: Skip metadata fetches if already populated
+        // (e.g., from constructor options or previous connect() call)
         if (!this.version || !this.database || !this.searchSchema) {
This would help future maintainers understand the optimization's intent and the scenarios it handles.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2681051 and cad2e8d.

📒 Files selected for processing (1)

src/driver/postgres/PostgresDriver.ts (1 hunks)

🔇 Additional comments (1)

src/driver/postgres/PostgresDriver.ts (1)

365-381: Verify lazy initialization handles reconnection scenarios correctly.

The guard condition and individual checks are logically sound. However, one important scenario needs verification:

Stale metadata on reconnection: The disconnect() method clears connection pools but does NOT clear the cached this.version, this.database, and this.searchSchema properties (lines 625-626 set pools to undefined but don't reset metadata). This means if connect() is called again on the same driver instance after disconnect(), the old cached metadata will be reused rather than fetched from the database. Verify whether this is the intended behavior for issue #11372, or if disconnect() should also clear these properties.

The concern in the original review about null-checking at line 403 is not valid—VersionUtils.isGreaterOrEqual() safely handles undefined values by returning false (see src/util/VersionUtils.ts:6-7).

Verification checklist:

Confirm whether reconnection after disconnect() should fetch fresh metadata or reuse cached values

Add tests covering: (1) first connect() fetches all metadata, (2) second connect() on same instance with no disconnect() skips fetching, (3) connect() after disconnect() behavior

Consider whether disconnect() should reset metadata properties to ensure fresh fetches on reconnection

G0maa

Hello @alumni, is this comment still relevant to this PR?

alumni · 2025-11-23T13:28:21Z

@G0maa it was quite some time ago when I had that fresh in mind, right now I don't really remember what was the problem (maybe since I didn't request changes it wasn't that important).

We could maybe offer some connection option dbFeatureVersion (more explicit than just version) with the default being to automatically get the version from the DB (currently on connect, ideally when required).

G0maa

The general idea/approach looks good, I'm unsure if this would break things else where like @alumni said, waiting for maintainers' opinions.

Clarify that the Postgres version is set on the first connection attempt.

pkuczynski · 2025-11-24T08:59:50Z

This can't break anything as we already set the version. It improves this, by setting the version only if it was not set before (so does it less forcefully). Feels like a good change.

qodo-free-for-open-source-projects · 2025-11-24T09:00:10Z

PR Code Suggestions ✨

Latest suggestions up to 72b80b2

Category Suggestion Impact

Possible issue

Ensure query runner cleanup on error

Wrap the database metadata fetching logic in a try...finally block to ensure the
queryRunner is released even if an error occurs, preventing potential connection
leaks.

src/driver/postgres/PostgresDriver.ts [366-382]

 if (!this.version || !this.database || !this.searchSchema) {
     const queryRunner = this.createQueryRunner("master")
+    
+    try {
+        if (!this.version) {
+            this.version = await queryRunner.getVersion()
+        }
 
-    if (!this.version) {
-        this.version = await queryRunner.getVersion()
+        if (!this.database) {
+            this.database = await queryRunner.getCurrentDatabase()
+        }
+
+        if (!this.searchSchema) {
+            this.searchSchema = await queryRunner.getCurrentSchema()
+        }
+    } finally {
+        await queryRunner.release()
     }
-
-    if (!this.database) {
-        this.database = await queryRunner.getCurrentDatabase()
-    }
-
-    if (!this.searchSchema) {
-        this.searchSchema = await queryRunner.getCurrentSchema()
-    }
-
-    await queryRunner.release()
 }

Apply / Chat

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential connection leak if an error occurs while fetching database metadata and proposes using a try...finally block to ensure the queryRunner is always released, which is a critical improvement for resource management.

Medium

More

Previous suggestions

Suggestions up to commit 218e0b5

Category Suggestion Impact

Possible issue

Ensure query runner cleanup on error

Wrap the database metadata retrieval in a try...finally block to ensure the
queryRunner is always released, preventing potential connection leaks if an
error occurs.

src/driver/postgres/PostgresDriver.ts [366-382]

 if (!this.version || !this.database || !this.searchSchema) {
     const queryRunner = this.createQueryRunner("master")
+    
+    try {
+        if (!this.version) {
+            this.version = await queryRunner.getVersion()
+        }
 
-    if (!this.version) {
-        this.version = await queryRunner.getVersion()
+        if (!this.database) {
+            this.database = await queryRunner.getCurrentDatabase()
+        }
+
+        if (!this.searchSchema) {
+            this.searchSchema = await queryRunner.getCurrentSchema()
+        }
+    } finally {
+        await queryRunner.release()
     }
-
-    if (!this.database) {
-        this.database = await queryRunner.getCurrentDatabase()
-    }
-
-    if (!this.searchSchema) {
-        this.searchSchema = await queryRunner.getCurrentSchema()
-    }
-
-    await queryRunner.release()
 }

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential resource leak. If any of the await calls fail, the queryRunner.release() would not be called, leading to a connection not being returned to the pool. Using try...finally is the correct pattern to prevent this.

Medium

pkg-pr-new · 2025-11-24T09:01:31Z

typeorm-sql-js-example

npm i https://pkg.pr.new/typeorm/typeorm@11373

commit: 72b80b2

gioboa

LGTM 👏

alumni · 2025-11-24T15:23:52Z

@hfhchan-plb Hold on, how does this even work? How would I use this if I wanted to avoid reading the version from the DB?

I see it's relying on Driver.version, but how is that being set?

hfhchan-plb added 2 commits March 30, 2025 17:17

Init version in postgres driver only if not set

0566ef2

don't init query runner when not necessary

c184436

alumni mentioned this pull request Apr 6, 2025

Allow postgres version to be set statically #11372

Closed

18 tasks

gioboa requested review from G0maa and pkuczynski November 14, 2025 09:27

Merge branch 'master' into patch-2

cad2e8d

coderabbitai bot reviewed Nov 14, 2025

View reviewed changes

G0maa reviewed Nov 23, 2025

View reviewed changes

G0maa requested review from alumni and gioboa November 24, 2025 07:31

G0maa approved these changes Nov 24, 2025

View reviewed changes

pkuczynski changed the title ~~feat: Init version in postgres driver only if not set~~ feat: init version in postgres driver only if not set Nov 24, 2025

pkuczynski approved these changes Nov 24, 2025

View reviewed changes

Update comment for Postgres version property

218e0b5

Clarify that the Postgres version is set on the first connection attempt.

Merge branch 'master' into patch-2

72b80b2

pkuczynski approved these changes Nov 24, 2025

View reviewed changes

pkuczynski requested review from OSA413 and mguida22 November 24, 2025 11:49

gioboa approved these changes Nov 24, 2025

View reviewed changes

pkuczynski merged commit cb1284c into typeorm:master Nov 24, 2025
64 checks passed

ThbltLmr pushed a commit to ThbltLmr/typeorm that referenced this pull request Dec 2, 2025

feat: init version in postgres driver only if not set (typeorm#11373)

8e3ec3b

Minishlink mentioned this pull request Jan 12, 2026

fix(postgres): handle timestamptz correctly #11916

Open

mgohin pushed a commit to mgohin/typeorm that referenced this pull request Jan 15, 2026

feat: init version in postgres driver only if not set (typeorm#11373)

66b457b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: init version in postgres driver only if not set #11373

feat: init version in postgres driver only if not set #11373

hfhchan-plb commented Mar 30, 2025 •

edited by pkuczynski

Loading

Uh oh!

coderabbitai bot commented Nov 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

G0maa left a comment

Uh oh!

alumni commented Nov 23, 2025 •

edited

Loading

Uh oh!

G0maa left a comment

Uh oh!

pkuczynski commented Nov 24, 2025

Uh oh!

qodo-free-for-open-source-projects bot commented Nov 24, 2025 •

edited

Loading

Uh oh!

pkg-pr-new bot commented Nov 24, 2025

Uh oh!

gioboa left a comment

Uh oh!

Uh oh!

alumni commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

feat: init version in postgres driver only if not set #11373

feat: init version in postgres driver only if not set #11373

Conversation

hfhchan-plb commented Mar 30, 2025 • edited by pkuczynski Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Pull-Request Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

G0maa left a comment

Choose a reason for hiding this comment

Uh oh!

alumni commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G0maa left a comment

Choose a reason for hiding this comment

Uh oh!

pkuczynski commented Nov 24, 2025

Uh oh!

qodo-free-for-open-source-projects bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

pkg-pr-new bot commented Nov 24, 2025

Uh oh!

gioboa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alumni commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hfhchan-plb commented Mar 30, 2025 •

edited by pkuczynski

Loading

coderabbitai bot commented Nov 14, 2025 •

edited

Loading

alumni commented Nov 23, 2025 •

edited

Loading

qodo-free-for-open-source-projects bot commented Nov 24, 2025 •

edited

Loading