Update postgresql_type.go - check notNull and emitPointersForNull before SQLDriver

[theAnuragMishra]

this fixes the issue where despite having emit_pointers_for_null_types set to true, the generate models don't use pointers for timestamp field.

[benjaco]

Linked to issue #3837

I can see there is a lot of tests there fails, the question should then be if the new output is the desired or not? Is it to big of a breaking change? should it be behind a new flag?

[benjaco]

@theAnuragMishra maybe you could update the tests so it's easier to see the impact of this change? It needs to be updated anyway for it to get merged

[acartine]

is there a way i can commandeer this? Seems author might be tied up?

[benjaco]

@acartine I think you'd be more than welcome to give it a go in a new PR

[acartine]

@acartine I think you'd be more than welcome to give it a go in a new PR

ok i'll give it a shit. i'm on gmt+1 but hopefully it won't take too long

[acartine]

really proud of my typo above, first class

[acartine]

@benjaco from looking at the code more closely it appears there is an unclear configuration hierarchy

if you look at the first few types in

sqlc/internal/codegen/golang/postgresql_type.go

Lines 44 to 139 in 047d587

	case "serial", "serial4", "pg_catalog.serial4":
	if notNull {
	return "int32"
	}
	if emitPointersForNull {
	return "*int32"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int4"
	}
	return "sql.NullInt32"
	case "bigserial", "serial8", "pg_catalog.serial8":
	if notNull {
	return "int64"
	}
	if emitPointersForNull {
	return "*int64"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int8"
	}
	return "sql.NullInt64"
	case "smallserial", "serial2", "pg_catalog.serial2":
	if notNull {
	return "int16"
	}
	if emitPointersForNull {
	return "*int16"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int2"
	}
	return "sql.NullInt16"
	case "integer", "int", "int4", "pg_catalog.int4":
	if notNull {
	return "int32"
	}
	if emitPointersForNull {
	return "*int32"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int4"
	}
	return "sql.NullInt32"
	case "bigint", "int8", "pg_catalog.int8":
	if notNull {
	return "int64"
	}
	if emitPointersForNull {
	return "*int64"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int8"
	}
	return "sql.NullInt64"
	case "smallint", "int2", "pg_catalog.int2":
	if notNull {
	return "int16"
	}
	if emitPointersForNull {
	return "*int16"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Int2"
	}
	return "sql.NullInt16"
	case "float", "double precision", "float8", "pg_catalog.float8":
	if notNull {
	return "float64"
	}
	if emitPointersForNull {
	return "*float64"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Float8"
	}
	return "sql.NullFloat64"
	case "real", "float4", "pg_catalog.float4":
	if notNull {
	return "float32"
	}
	if emitPointersForNull {
	return "*float32"
	}
	if driver == opts.SQLDriverPGXV5 {
	return "pgtype.Float4"
	}
	return "sql.NullFloat64" // TODO: Change to sql.NullFloat32 after updating the go.mod file

the go types and emit-pointers-on-null supercede pgxv5

then for numerics/dates/times/uuid etc. that hierarchy is flipped.

this leads to a confusing devx because sometimes you get the go types / pointers and sometimes you get the pgtypes, which are more cumbersome to work with. the latter reason is probably why we try to use go types for nonnulls regardless of whether or not it is pgxv5 for the literal types.

i would think we need to draw some kind of line in the sand here and say there are two modes

1. strict pgtypes (no null config, since pgtypes support null)
2. go types with either pointers for null or sql.null. I would think just pointers because if you want values just use mode 1. keep it simple

is there a discord/slack for broader discussion about this?

[benjaco]

@acartine Issnt that what "emit_pointers_for_null_types" is suppose to configure?

Docs

• emit_pointers_for_null_types:
  • If true, generated types for nullable columns are emitted as pointers (ie. *string) instead of database/sql null types (ie. NullString). Currently only supported for PostgreSQL if sql_package is pgx/v4 or pgx/v5, and for SQLite. Defaults to false.

So if emit_pointers_for_null_types is set to true, then the emitPointersForNull check should always come before the database type check in the typegen code.

There is no broader discussion, its all in the issue #3837 and in the pr's linked around that issue

[acartine]

i think there is a bit more clarification need. would you prefer i do it on that issue instead of this one?

in that issue you use the text/varchar example. And that's fine if that is the expected behavior.

however 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.

that is a reasonably big change that will break any code that upgrades and does not use emitPointersForNull.

but if that's how you want me to implement it i'm happy to do it that way.

while we are at it, i can't seem to find a command to update the expected outputs, should I assume then that it is manual and not auto generated?

[benjaco]

@acartine lets move the discussion to the issue
but to sum up short - i dont know the mindset balancing breaking changes and configs, we need some attention from Kyle maybe, but a summery of the different solutions would properly help him here - and i dont know the internal tooling of this project, I'm a first time contributer as well