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?