Subject: Re: [pgjdbc/pgjdbc] PR #2148: Avoid leaking server error details through BatchUpdateException when logServerErrorDetail=false

From: "frode-carlsen (@frode-carlsen)" <[email-masked]>
Date: Wed, 12 May 2021 12:41:04 +0000

PR for  https://github.com/pgjdbc/pgjdbc/issues/2147

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 12 May 2021 12:50:46 +0000

Thank you very much for the PR.

https://github.com/pgjdbc/pgjdbc/blob/master/docs/documentation/head/connect.md will require an entry as well.
Additionally can you add an entry into CHANGELOG.md?

Thanks

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 12 May 2021 12:50:55 +0000

I'll take a look at this for review. We should add a test for this as well.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 12 May 2021 13:01:05 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java)

This member variable can be `final`.

And for the comment how about: `// Whether to include error details in logging and exceptions`

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 12 May 2021 13:03:57 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/BatchResultHandler.java)

Let's wrap the new if-block on the outside. That way it's clear that the default of `"<unknown>"` would only be replaced if it's detailed logging is enabled:

```java
String queryString = "<unknown>";
if (pgStatement.getPGConnection().getLogServerErrorDetail()) {
    if (resultIndex < queries.length) {
        queryString = queries[resultIndex].toString(
            parameterLists == null ? null : parameterLists[resultIndex]);
      }
}
```

---

From: "frode-carlsen (@frode-carlsen)" <[email-masked]>
Date: Thu, 13 May 2021 15:34:44 +0000

👍

---

From: "frode-carlsen (@frode-carlsen)" <[email-masked]>
Date: Thu, 13 May 2021 15:34:53 +0000

👍

---

From: "frode-carlsen (@frode-carlsen)" <[email-masked]>
Date: Thu, 13 May 2021 15:35:09 +0000

Updated after codereview

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Thu, 13 May 2021 16:40:26 +0000

I've rebased this, cleaned up a few things, and added a test. I'm going to try pushing it to your branch to see if that actually works.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Thu, 13 May 2021 16:48:47 +0000

Nice looks like the push worked. Let's see it run through CI and then we'll merge it.

I split out your original fix into two commits. The first updates the docs. The second is the fix for the property handling with batches. The fix itself worked but was missing an update to the one other test class that also implements BaseConnection and there were a couple style issues.

Third commit adds some new tests with batching.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Thu, 13 May 2021 18:21:08 +0000

This is ready to go but let's wait for #2152 so that the CHANGELOG entry can be corrected as well.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Sat, 15 May 2021 13:14:49 +0000

This has been merged. Thanks for the bug report and PR.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 09 Apr 2025 17:43:51 +0000

The driver does not log exceptions, so it is unfortunate to start the property name with `log...`