setTimestamp use timestamptz db type

[arons]

In relation to :
https://www.postgresql.org/message-id/flat/CA%2BXOKQC67VVYk6HfPbFxWYDuwR4BmVzhWhBC%3DLa1y3jmYNta2Q%40mail.gmail.com#4d785a11e331943d7e97aab04ee25f10

here the patch we are currently using to let us having the proper data type on db side.
Should not create any issue to existing coda, logic is enabled by a property.

[davecramer]

how is this different than #2715

[arons]

I think is complementary to the other patch.
Still I think is missing the changes into PgConnection.java to allow to force the type timestamptz as parameter.
This setting force oid to be Oid.TIMESTAMPTZ into method PgPreparedStatement.setTimestamp final call bindString(i, getTimestampUtils().toString(cal, t), oid).
In this way the data type on db is also consistent to what the client is setting.

I added a test case to illustrate it org.postgresql.test.jdbc2.TimestamptzTest.
Method testTypeOnDbSite should be handle by mine patch, testTypeOnJavaSite should be handle from #2715.

Eventually the config parameter can be merged in a single one.

Let me know if would be better to do in a different way or if I need to provide more informations.

[vlsi]

Please refrain from making unrelated changes

[vlsi]

I believe always is a misleading property name here

[vlsi]

@arons, could you provide a test case to reproduce the issue?

I am afraid we won't be able to merge a PR that changes timestamp behaviour without having a clear case that exercises values of the timestamp in question.

I see you've added pg_typeof call, however, I believe that function is not used often in real-life applications (why call pg_typeof on the value that is passed from JDBC?!), and I see no tests that verify values that are passed to the database and/or received from the database.

[arons]

I've added some examples which require an explicit cast on db query, that even if the driver already know the data type it pass.

Also the patch does not modify the behavior of the driver if the parameter is not changed (which I agree the name is miss leading, I can change that).

[vlsi]

Note there's

    public Timestamp(int year, int month, int date,
                     int hour, int minute, int second, int nano) {

Which assumes

{@code year}, {@code month}, {@code date},
{@code hrs}, {@code min}, and {@code sec} arguments,
in the local time zone.

However, after the Timestamp has been created, there's no way to tell if it should be treated as "local timestamp" or "timestamp with time zone".

There might be use cases for both of that.
In other words, the driver does not know if setTimestamp(...) should be treated as "local timestamp" or "timestamp with time zone".

[arons]

The idea of the patch was to avoid as much a possible timestamp without timezone and be sure the timezone information is never lost.

See: https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_timestamp_.28without_time_zone.29

It is a general recommendation to do not use it. So better to use always with time zone and manipulate special cases ad hoc on db site.

[vlsi]

I would say developers should refrain from Timestamp and prefer java.time. classes to better convey the meaning of the data.

[davecramer]

The idea of the patch was to avoid as much a possible timestamp without timezone and be sure the timezone information is never lost.

See: https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_timestamp_.28without_time_zone.29

It is a general recommendation to do not use it. So better to use always with time zone and manipulate special cases ad hoc on db site.

while it might be better, it isn't the job of the driver to be opinionated. The driver must support all types.

[arons]

I think the driver support already all the types by passing a string literal to postgres.
Mine was a possibility to set the behavior specific to use timestamptz only.
With a parameter is a decision of the users.

I do not share your definition about the driver.
From my point of view timestamptz is the only correct type the driver should pass, on postgres side I can work with it and convert it to any other type conscious to what I'm doing and having all the information I need (also errors in this case become much more user friendly).
I was really surprise when I saw driver is passing a string to postgres for that method.
I spent a lot of time to search for errors in code and plsql until I decide to check the source code.

Anyway, do you still want to merge the patch?
if yes, can we summarize the necessary changes you would like to have for it? I can happily work on it.
if not, is also ok. I don't want to push for that.
I can run my software on top of my small patch, thanks to the open source and your work which I appreciate a lot.

[davecramer]

I do not share your definition about the driver.
From my point of view timestamptz is the only correct type the driver should pass, on postgres side I can work with it and convert it to any other type conscious to what I'm doing and having all the information I need (also errors in this case become much more user friendly).

You are not alone in this opinion. However what do we tell people who have used timestamp (without timezone) ?

[redalogobject]

I do not share your definition about the driver.
From my point of view timestamptz is the only correct type the driver should pass, on postgres side I can work with it and convert it to any other type conscious to what I'm doing and having all the information I need (also errors in this case become much more user friendly).

You are not alone in this opinion. However what do we tell people who have used timestamp (without timezone) ?

For them, they can continue using the driver without parameter, as today.
Or they can move to use with time zone, as in patch, by enabling the parameter, in that case they need to change either java or db site.

But I expect that most java programs using timestamp without time zone are managing the problem in the client side (java) in that case no changes should be needed.

I can maybe create some test cases or if you have some, I can test the behavior with new patch enabled.
I do not have some real cases at the moment for timestamp without time zone.

[davecramer]

I am only discussing the philosophy of the driver here.

I'm aware that they can chose which behaviour they want.

[vlsi]

But I expect that most java programs using timestamp without time zone are managing the problem in the client side (java) in that case no changes should be needed.

Can you list examples of the applications that would benefit from going with a non-default property for timestamp handling?

Frankly speaking, I think if an application was using setTimestamp, then it expected the API to work in a certain way. If you change the way the API works, then the app might get wrong results.

Of course, if there's an app that produces wrong results with the default behavior, then it might be something to consider. However, it looks like the new apps should use java.time. classes, and it should resolve the ambiguity issues.

[arons]

Of course, if there's an app that produces wrong results with the default behavior, then it might be something to consider. However, it looks like the new apps should use java.time. classes, and it should resolve the ambiguity issues.

If that is your vision, is ok for me, than we can close the pull request here.
Please have a look to my last commit cause the ZonedDateTime still does not look to me as a valid alternative for the moment.

[davecramer]

There's no reason to close it. We were still discussing it.

[vlsi]

Please have a look to my last commit cause the ZonedDateTime still does not look to me as a valid alternative for the moment

ZonedDateTime is not implemented right now. PRs are welcome. See #1325 (comment)