int8/bigint/bigserial: number vs BigInt

[cakoose]

Thanks for adding int8/bigint/bigserial support in efdb893.

One problem: JS number can't losslessly represent all the values in an 64-bit integer.

• A JS number can represent integers up to about 53 bits. That might be enough for some use cases, but it seems dangerous for Mammoth to default to something that's lossy. Might catch people by surprise.
• Node 10.4+ supports BigInt, which can represent all int8 values.

Suggestion: expose two int8 types, one for number and one for BigInt.

• Option 1: Call them int8AsJsBigInt and int8AsJsNumber so the user knows what they're getting in to.
• Option 2: Call them int8 and int8AsJsNumber. Node 10 is the oldest actively-supported version of Node, so defaulting to BigInt might be reasonable. And the user can opt-in to number if they want.

[martijndeh]

BigInt is great and probably the right direction for those data types eventually.

Looking at pg though it defaults to string for those types instead of number. It's probably best to switch to string for now so you don't run into surprises during runtime. What do you think of this option 3?

The idea was for every data type to have a configurable type with a sane default e.g. bigint<BigInt>() switches the type to BigInt but this doesn't seem to work yet. I'll have a look why this isn't working properly.

[cakoose]

I was just about to file a separate bug for the string issue :-)

This is how to tell "pg" to return BigInt instead of string:

import pg from "pg";
import pgTypes from "pg-types";

const client = new pg.Client({
    connectionString: "postgres://localhost:5432/anrok?sslmode=disable",
    types: {
        getTypeParser(typeId: pgTypes.TypeId, format: 'text' | 'binary'): any {
            if (typeId === pgTypes.builtins.INT8) {  // also used for "bigint" and "bigserial"
                assert(format === 'text');
                return BigInt;
            } else {
                return pgTypes.getTypeParser(typeId, format);
            }
        }
    }
} as any);  // The "any" is necessary because "@types/pg" doesn't support the "types" parameter yet

Because "pg" defaults to string, it makes sense for Mammoth to also default to string. But I think many users will want BigInt and so it would be nice if Mammoth made that easy to do (and easy to discover in the docs).

The first option I thought of was to provide additional int8AsJsBigInt and int8AsJsNumber types. That way users could choose different JS types for different columns. For example:

• If an int8 is being used as an opaque ID, then string might be preferred.
• If an int8 is being used as a number that will never use more than 53 bits, then number might be preferred.
• If an int8 is being used as a number that might use the full 64 bits, then bigint is the best option.

However, the problem now is that Mammoth has to parse the string. But this would require Mammoth to transform every row returned by "pg", which isn't ideal.

An alternative is to match what "pg" allows -- a global change for the entire schema, e.g.:

const files = defineTable({
    name: text().primaryKey(),
    size: int8().notNull(),
});
...


const db1 = defineDb({files, ...}, queryFn);
const db2 = db1.int8AsBigInt();  // A variant that uses BigInt instead of string.

I'm not familiar with Mammoth internals, so I'm not sure exactly how this would work :-P

[cakoose]

That said, maybe the "best" option is not worth the effort/complexity.

A simpler option:

• Make it so that int8 requires a type parameter, to force the user to think about it.
• Make it easy to find the documentation for customizing pg.Client type parsing.

[martijndeh]

Thank you for your thoughts on this.

I just applied a fix in 86b6655 to default bigint(), bigserial(), int8() to strings instead of numbers as this is what pg is returning today. Also, number really doesn't make sense. This also includes a fix so you can overwrite the default types e.g. size: int8<BigInt>().notNull() gives you a BigInt type back (assuming your database driver is running this type at runtime).

It seems pg is currently working on getting BigInt the default type for those data types, so once they do Mammoth will switch as well. There is some more going on like data type date becoming string. See brianc/node-pg-types#78. For now you have to workaround this.

I documented this including the bigint workaround at https://mammoth.tools/defining-tables#data-types. I also added a suggestion on how to create a reusable data type so you don't have to specify e.g. bigint<BigInt>() all the time.

[cakoose]

Thanks!

This is a sort of a minor point, but the example you added to the docs mutates global state. It's definitely the easiest option, but for people who (like me :-) who prefer to avoid global state if possible, maybe it's worth pointing to the per-Client/per-Pool types setting: brianc/node-postgres#1838 (comment)

Feel free to ignore if you prefer.

[martijndeh]

I agree. I'll update this in the docs.