jschaf / pggen

Generate type-safe Go for any Postgres query. If Postgres can run the query, pggen can generate code for it.
MIT License
287 stars 26 forks source link

Better detection of null values #45

Closed devfd closed 2 years ago

devfd commented 2 years ago

Hi,

I am currently evaluating pggen with various type of queries and I am struggling with null values.

Let's say I have a simple table

CREATE TYPE job_completion as ENUM (
    'ok',
    'error',
    'abort'
    );

CREATE TABLE job
(
    id         bigint primary key generated always as identity,
    created_at timestamptz    not null default now(),
    updated_at timestamptz    not null default now(),
    ended_at   timestamptz   ,
    sync_since timestamptz   ,
    completion job_completion,
    data       int           ,
    error      text          
);

1. Null values in parameters

To update a job I would write the following:

-- name: JobUpdate :one
update job
set updated_at = now(),
    data       = pggen.arg('data'),
    completion = pggen.arg('completion'),
    ended_at   = pggen.arg('ended_at')
where id = pggen.arg('id')
returning *;

resulting in the following parameters

type JobUpdateParams struct {
    Data       int32
    Completion JobCompletion
    EndedAt    pgtype.Timestamptz
    ID         int
}

But as the columns are NULLABLE I would expect Data Completion and EndedAt to be pointers.

To go around this one can use NULLIF in the query but that make queries more verbose. Is there another way to do this ?

2. Null values in output

When limiting or ordering results in a select statement I get all fields as pointers

select *
from job
limit 1;
type JobGetLatestRow struct {
    ID         *int               `json:"id"`
    CreatedAt  pgtype.Timestamptz `json:"created_at"`
    UpdatedAt  pgtype.Timestamptz `json:"updated_at"`
    EndedAt    pgtype.Timestamptz `json:"ended_at"`
    SyncSince  pgtype.Timestamptz `json:"sync_since"`
    Completion JobCompletion      `json:"completion"`
    Data       *int32             `json:"data"`
    Error      *string            `json:"error"`
}

It seems the Sort and Limit plan types are not handled in isColNullable.

The list of plan types is rather long (https://github.com/postgres/postgres/blob/master/src/backend/commands/explain.c#L1163) do you think we could have a simple case where the column is used to detect nullability if it's a select statement ?

Let me know if you want a PR Many thanks for your time and for this library.

saward commented 2 years ago

I'm evaluating this myself, too, and am wondering about a way to signal that a parameter can be sent as NULL. Without having looked in detail, if using the pggen.arg() function, could that give a way to specify/signal to the generator that a value is allowed to be provided as NULL?

EmilLaursen commented 2 years ago

Thank you for this library! Also evaluating this for my org. Have similar issues. pggen seems to use pointer values (*string,*bool etc), instead of pgtype, in many cases. I would expect it to always use pgtype, unless I manually override types. I can give a small example if needed, but I think it would be a repeat of case 2. above.

jschaf commented 2 years ago

do you think we could have a simple case where the column is used to detect nullability if it's a select statement ?

I've added some really basic heuristics with isColNullable but there's not an easy answer to determine if a column is nullable. Adding support for returning clauses seems reasonable. I think nulls caused by the limit clause was fixed in https://github.com/jschaf/pggen/commit/1266d15e85c54237d9b58b1a701acd5274bc2c45


could that give a way to specify/signal to the generator that a value is allowed to be provided as NULL?

That depends more on the type of the parameter. If the parameter is a string, there's no value we could send that pgx, the underlying library would interpret as null. For null there's two options:

  1. Use a nullable type like *string or pgtype.Text.
  2. Convert a default value to null in SQL like SELECT nullif(pggen.arg('foo')::text, '')

pggen seems to use pointer values (string,bool etc), instead of pgtype

Yes, that resulted from https://github.com/jschaf/pggen/issues/22. The pgtype structs aren't the most ergonomic types. pggen hardcodes support for some well-known types: https://github.com/jschaf/pggen/blob/2489d54a806652d73f2c83bb5736a7574582092c/internal/codegen/golang/gotype/known_types.go#L179-L178 (the second and third entries indicate there's a more ergonomic type to use.