Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pgtype.Time doesn't parse time with time zone properly #1940

Closed
nathan-artie opened this issue Mar 11, 2024 · 4 comments
Closed

pgtype.Time doesn't parse time with time zone properly #1940

nathan-artie opened this issue Mar 11, 2024 · 4 comments
Labels

Comments

@nathan-artie
Copy link

Describe the bug
When using pgtype.Time to parse a time with time zone column that has been retrieved from a Postgres database the time zone offset ends up being used as the resulting fractional seconds.

To Reproduce

Insert time data into Postgres to verify string output:

CREATE TABLE foo (time_col TIME WITH TIME ZONE);

INSERT INTO foo VALUES(TIME WITH TIME ZONE '05:34:17-05');
INSERT INTO foo VALUES('05:34:17'::time AT TIME ZONE 'America/New_York');

SELECT * FROM foo;

Outputs values:

  time_col   
-------------
 05:34:17-05
 01:34:17-04
(2 rows)

Then try to parse any of these values with pgtype.Time.Scan():

package main

import (
	"fmt"

	"github.com/jackc/pgx/v5/pgtype"
)

func main() {
	var timeValue pgtype.Time
	if err := timeValue.Scan("01:34:17-04"); err != nil {
		panic(err)
	}

	value, err := timeValue.Value()
	if err != nil {
		panic(err)
	}

	println(fmt.Sprintf("Value: %s Microseconds: %v", value, timeValue.Microseconds))
}

Expected behavior
Either the time zone offset should be ignored 01:34:17.000000, a parsing error should be thrown, or the offset should be applied to the time 05:34:17.000000.

Actual behavior

Value: 01:34:17.040000 Microseconds: 5657040000

Note the .04

Version

  • Go: go version go1.22.0 darwin/arm64
  • PostgreSQL: PostgreSQL 16.2 on x86_64-pc-linux-musl, compiled by gcc (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924, 64-bit
  • pgx: github.com/jackc/pgx/v5 v5.5.4
@nathan-artie nathan-artie changed the title pgtype.Time doesn't parse times with time zone types properly pgtype.Time doesn't parse times with time zone properly Mar 11, 2024
@nathan-artie nathan-artie changed the title pgtype.Time doesn't parse times with time zone properly pgtype.Time doesn't parse time with time zone properly Mar 11, 2024
@felix-roehrich
Copy link
Contributor

Looking at the types in pgtype the OID for timetz is defined, but there is no corresponding go type. The pgtype.Time is the wrong type for scanning; a pgtype.Timetz type would need to be defined.

There is also a bug in the string parsing of pgtype.Time: It is assumed the everything after the 9th char is ms, but the 9th char is never checked to be .. This is what is happening in your case.

Overall both things should be easy to solve. @jackc If you have no further comments I would implement this on the weekend.

@jackc
Copy link
Owner

jackc commented Mar 16, 2024

@felix-roehrich I suppose stricter parsing for time / pgtype.Time would be good.

I'm not especially enthusiastic about adding timetz support. The PostgreSQL docs themselves discourage using that type.

From https://www.postgresql.org/docs/current/datatype-datetime.html:

The type time with time zone is defined by the SQL standard, but the definition exhibits properties which lead to questionable usefulness.

and

We do not recommend using the type time with time zone (though it is supported by PostgreSQL for legacy applications and for compliance with the SQL standard)

It's not even clear to me exactly what the Go type would look like.

@felix-roehrich
Copy link
Contributor

We do not recommend using the type time with time zone (though it is supported by PostgreSQL for legacy applications and for compliance with the SQL standard).

I have thought about this myself. On the one hand it is still part of the SQL standard, on the other hand it is clearly used very rarely (otherwise this issue would have been reported long ago).
I suppose one way would be to make the parsing stricter and see how many people are affected?

@jackc
Copy link
Owner

jackc commented Mar 16, 2024

Making parsing stricter on time such that it would fail when parsing a timetz seems to be a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants