Subject: [pgjdbc/pgjdbc] PR #3220: setTimestamp use timestamptz db type

From: "arons (@arons)" <[email-masked]>
Date: Wed, 17 Apr 2024 16:38:00 +0000

setTimestamp use timestamptz db type
 old pull request #2835 
 this pull request should fix also the issue #1325 ( test cases added into class org.postgresql.test.jdbc2.TimestamptzTest )

### New Feature Submissions:

1. [x] Does your submission pass tests?
2. [x] Does `./gradlew styleCheck` pass ?
3. [x] Have you added your new test classes to an existing test suite in alphabetical order?

### Changes to Existing Features:

* [x] Does this break existing behaviour? If so please explain.
 it breaks currently behaviour only in case user set sqlTimestamptzAlways 
* [x] Have you added an explanation of what your changes do and why you'd like us to include them?
 explained already in pull request  #2835
* [x] Have you written new tests for your core changes, as applicable?
* [x] Have you successfully run tests with your changes locally?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 17 Apr 2024 17:55:20 +0000

Why do another PR ?

---

From: "arons (@arons)" <[email-masked]>
Date: Wed, 17 Apr 2024 18:06:50 +0000

I did some mistake on the original github fork and it closes the pull request. So I created a new one, sorry.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 23 Apr 2024 16:44:27 +0000

(on pgjdbc/src/main/java/org/postgresql/PGProperty.java:673)

s/time stamp/timestamp

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 23 Apr 2024 16:48:21 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/TimestamptzTest.java)

s/2004/2024

---

From: "liias (@liias)" <[email-masked]>
Date: Thu, 02 May 2024 09:51:16 +0000

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

Hmm `ZonedDateTime.toLocalDateTime()` would just forget the timezone of the zdt? 
Also, why would it support `ZonedDateTime`, but not `OffsetDateTime`?

I might be wrong with my assumptions here of course.

---

From: "arons (@arons)" <[email-masked]>
Date: Tue, 07 May 2024 10:16:36 +0000

Your assumption is correct, so I changed it to use OffsetDateTime type so that time zone information is not lost but stored into the timestamptz.

Anyway, if you use OffsetDateTime to fetch data back form the db, the time zone info is lost as illustrated by the test case TimestamptzTest.test_ZonedDateTime_StoreAndSelect(). 
How is the correct way to get back the information of the timezone (without using string) ?

NOTE: in our application we always store dates in UTC time zone in the db to avoid any problem. If we need to store information about the time zone to use for some business logic that is stored into a separate fields independent to any stored timestamptz.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 07 May 2024 10:39:51 +0000

Please avoid `fail(e.getMessage());` pattern as it adds no extra value, and it hides the stacktrace. In cases like this just let the exception propagate, and do not catch it.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 07 May 2024 10:48:53 +0000

@arons , can you please clarify which issue does this change solve?
Please see https://github.com/pgjdbc/pgjdbc/pull/2835#issuecomment-1466364745

I am afraid, it is hard to review the change without understanding the problem it solves.

---

From: "arons (@arons)" <[email-masked]>
Date: Wed, 08 May 2024 12:32:35 +0000

The problem it solves is to correctly pass the proper OID type for timestamps to the database, instead of just as text, which is the current behavior. 

This is useful in many different cases as it allows SQL and PL/SQL to use the parameter properly without requiring an explicit cast.
Within the class TimestamptzTest, there are basic examples of statements that will fail if a timestamp parameter is passed as a string. 
I can provide even more examples of problems if necessary.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 08 May 2024 12:43:52 +0000

>I can provide even more examples of problems if necessary

It would be great. I would suggest we solve problem a time rather than combine everything possible within a single discussion.

> Within the class TimestamptzTest, there are basic examples of statements that will fail if a timestamp parameter is passed as a string.

There are cases like `select ?` and `select pg_typeof(?)`, and I believe they are not quite niche, so we should not consider them as the primary driving factor.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 08 May 2024 12:52:36 +0000

How is this different than https://github.com/pgjdbc/pgjdbc/pull/2715 ?

---

From: "arons (@arons)" <[email-masked]>
Date: Wed, 08 May 2024 12:56:38 +0000

> There are cases like `select ?` and `select pg_typeof(?)`, and I believe they are not quite niche, so we should not consider them as the primary driving factor.

testTypeOnDbSite_functionOverloading and  testTypeOnDbSite_select are less trivial once already.

In the current implementation:
The first: it takes the worng function implementation in case of overloading.
The second:  throw an exception if  we try to add an interval to a timestamp passed parameter.

It should fix also the issue https://github.com/pgjdbc/pgjdbc/issues/1325 (also in the test case, no error any more in invoking the setObject method  )

---

From: "arons (@arons)" <[email-masked]>
Date: Wed, 08 May 2024 12:59:18 +0000

> How is this different than #2715 ?

If I understand the patch correctly, that is more about the returned meta data type and not about parameter set.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 08 May 2024 13:36:01 +0000

> If I understand the patch correctly, that is more about the returned meta data type and not about parameter set.

in which case these PR's should be combined.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 08 May 2024 13:40:14 +0000

> testTypeOnDbSite_functionOverloading

Currently it defines a function with `timestmaptz`, and a function with `text` parameters.

I would suggest adding `timestamp` (without tz), `time`, and `timetz` into the mix and suggest the way you would expect it to work.

Frankly speaking, I think `setTimestmap( new Timestamp(..))` should fail rather than arbitrary select `timestamptz` overload among `timestamp` and `timestamptz`.

---

From: "arons (@arons)" <[email-masked]>
Date: Wed, 08 May 2024 13:50:58 +0000

> Frankly speaking, I think `setTimestmap( new Timestamp(..))` should fail rather than arbitrary select `timestamptz` overload among `timestamp` and `timestamptz`.

I agree with that: https://wiki.postgresql.org/wiki/Don%27t_Do_This#Don.27t_use_timestamp_.28without_time_zone.29
So that is the reason I prefer to use timestamptz among timestamp, which for me should in general not be used.

So if you prefer to use timestamp over timestamptz I think we can close the discussion and close this pull request cause my motiviation is only about timestamptz.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 02 Jul 2024 12:38:33 +0000

timestamptz does not store timezone information so there is no way to get it definitely. In text mode it is returned adjusted for the session timezone (most likely client timezone). In binary mode we assume the client timezone.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 02 Jul 2024 12:40:59 +0000

> Frankly speaking, I think `setTimestmap( new Timestamp(..))` should fail rather than arbitrary select `timestamptz` overload among `timestamp` and `timestamptz`.

I'm missing something here. https://github.com/pgjdbc/pgjdbc/pull/3220/files#diff-d4946409bd7c59e525f34b4c974a3df76638dc84adc060cc5d13d5409c6aeb21R1511 only selects `timestamptz` if `isSqlTimestamptzAlways` is set?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:54:21 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java:1493)

Seems we now have timestamp with and without time zone. Why force it ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:55:24 +0000

This is a pretty ambiguous error message. Please be more verbose. If someone has to debug this, they need more information.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:56:21 +0000

This should assert the correct values here

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:56:44 +0000

we don't really write to the console in tests

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:57:05 +0000

is the FIXME still valid?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 20:58:32 +0000

(on pgjdbc/src/main/java/org/postgresql/PGProperty.java:667)

>From what I can see in this PR the only time it forces timestamp with timezone is for setNull

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:00:08 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java:275)

Why not use the correct oid's here ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:00:56 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/TimestamptzTest.java:153)

Should check the result. What if the result set is empty ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:01:14 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/TimestamptzTest.java:165)

same as above; check the resultset

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:23:56 +0000

the check is not about the result but more on the syntax, without the fix postgres return:
org.postgresql.util.PSQLException: ERROR: could not determine data type of parameter $1

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:39:32 +0000

solved

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:39:46 +0000

solved

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:44:40 +0000

done

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:45:07 +0000

added check for non empty result set

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:45:09 +0000

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:45:41 +0000

done

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:45:55 +0000

added more info in the error message

---

From: "arons (@arons)" <[email-masked]>
Date: Mon, 13 Jan 2025 21:46:37 +0000

to keep align with logic enforced by isSqlTimestamptzAlways

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 13 Jan 2025 22:54:41 +0000

AFAICT, this is the only logic with isSqlTimestamptzAlways

---

From: "arons (@arons)" <[email-masked]>
Date: Tue, 14 Jan 2025 06:16:53 +0000

it is used also below to set the oid also in case of not null. 
Line 1522:

`
if (connection.isSqlTimestamptzAlways()) {
      oid = Oid.TIMESTAMPTZ;
    }
`

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 14 Jan 2025 07:04:39 +0000

@arons , could we discuss `ZonedDateTime` and `Instant` support separately from this PR?
It looks like handling `ZonedDateTime` and `Instant` does not depend on `timestamptz db type`, so it should better go as a separate PR.

---

From: "arons (@arons)" <[email-masked]>
Date: Tue, 14 Jan 2025 15:40:52 +0000

OK, I'll try to remove the ZonedDateTime and Instant and move to a different PR.