Nullable datetimes are used for timestamps marked as not null in PGX / GO

[benjaco]

Version

1.28.0

What happened?

For pgx, pgtype.Timestamp is used for TIMESTAMP (and other time related types) regardless if the column is nullable or not.

time.Time was expected to be used for timestamps marked with NOT NULL

"database/sql" is handling it correctly https://play.sqlc.dev/p/e1e23e0d41d3d1340a4a009099bb81b589385b49187b89fd40c9aee4c33097cf

Database schema

CREATE TABLE authors (
  id   BIGSERIAL PRIMARY KEY,
  name text      NOT NULL,
  bio  text,
  
  born TIMESTAMP      NOT NULL,
  died  TIMESTAMP
);

SQL queries

-- name: ListAuthors :many
SELECT * FROM authors
ORDER BY name;

Configuration

{
  "version": "2",
  "sql": [{
    "schema": "schema.sql",
    "queries": "query.sql",
    "engine": "postgresql",
    "gen": {
      "go": {
        "out": "db",
        "sql_package": "pgx/v5"
      }
    }
  }]
}

Playground URL

https://play.sqlc.dev/p/7ac654a59b8d17aa0f7e9bd8dfd689bec9d035bc8e8ea7a5a1beb078be531769

What operating system are you using?

No response

What database engines are you using?

PostgreSQL

What type of code are you generating?

Go

[benjaco]

I would guess that the not null checks here should be moved up as the first check

sqlc/internal/codegen/golang/postgresql_type.go

Lines 236 to 246 in ec9d492

	case "pg_catalog.timestamp", "timestamp":
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Timestamp"
	}
	if notNull {
	return "time.Time"
	}
	if emitPointersForNull {
	return "*time.Time"
	}
	return "sql.NullTime"

As it is with the string types

sqlc/internal/codegen/golang/postgresql_type.go

Lines 260 to 271 in ec9d492

	case "text", "pg_catalog.varchar", "pg_catalog.bpchar", "string", "citext", "name":
	if notNull {
	return "string"
	}
	if emitPointersForNull {
	return "*string"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Text"
	}
	return "sql.NullString"

Is there a reason that it issn't? If not, then I could go ahead and do the pr for this on all cases where the null check is below the pgx check

[eashan-dirac]

Facing this same issue. This is a pretty urgent bug considering nullable timestamptz's are relatively common in Postgres and having them be represented as nil in Go is very idiomatic.

[benjaco]

@mcdoker18 I can see you authored the original part of this code in #1874, do you have any comments about the desired intent here? Great job by the way!

I have some questions about linked PR's which breaks a lot of tests, the question is if the new output is the desired or not? Is it to big of a breaking change? should it be behind a new flag?

[acartine]

I would guess that the not null checks here should be moved up as the first check

sqlc/internal/codegen/golang/postgresql_type.go

Lines 236 to 246 in ec9d492

case "pg_catalog.timestamp", "timestamp":
if driver == opts.SQLDriverPGXV5 {
return "pgtype.Timestamp"
}
if notNull {
return "time.Time"
}
if emitPointersForNull {
return "*time.Time"
}
return "sql.NullTime"
As it is with the string types

sqlc/internal/codegen/golang/postgresql_type.go

Lines 260 to 271 in ec9d492

case "text", "pg_catalog.varchar", "pg_catalog.bpchar", "string", "citext", "name":
if notNull {
return "string"
}
if emitPointersForNull {
return "*string"
}
if driver == opts.SQLDriverPGXV5 {
return "pgtype.Text"
}
return "sql.NullString"

Is there a reason that it issn't? If not, then I could go ahead and do the pr for this on all cases where the null check is below the pgx check

it should be noted that that style means that that the pgtype is used only if the type is nullable and !emitPointersForNull. It will never be used if the type is notNull.

If we want to be opinionated and go that route then yes, we should change all of the type handling to match the text/varchar example. I think this to be a bit strange tbh that the driver/package dictate that sometimes you get driver specific types. It would mean that if the user switched drivers, the updated generated code would break the user's codebase.

As I mentioned in #3839 (comment), i would suggest we offer 2 feature paths:

1. only go types and pointers for nullables (i suspect this is the default behavior most would want)
2. pgtypes when pgxv5 and "usePgtypes: true"

On another note, since i volunteered to resolve this, i couldn't see any obvious one-liner to update the expected test fixtures. Is it simply manual despite the "do not edit by hand" warnings?