jakajancar / pgc4d

A full-featured PostgreSQL Client for Deno
MIT License
21 stars 3 forks source link

Feature request: support more pg column types #2

Closed hiteshjasani closed 4 years ago

hiteshjasani commented 4 years ago

I went to insert a value into a column defined as character varying(80) and it threw an error. Looking at the code now, it appears that only text and int columns are supported. For the current project I'm working on, it would be nice if the following were also supported:

error: Uncaught Error: Error sending param $1: Unsupported type: varchar (oid 1043, typsend varcharsend)
            throw new Error(`Unsupported type: ${type.typname} (oid ${typeOid}, typsend ${type.typsend})`)
                  ^
    at TypeRegistry.send (https://deno.land/x/pgc4d/src/data_type_registry.ts:79:19)
    at https://deno.land/x/pgc4d/src/prepared_statement.ts:106:47
    at Array.map (<anonymous>)
    at PreparedStatementImpl._serializeParams (https://deno.land/x/pgc4d/src/prepared_statement.ts:102:23)
    at PreparedStatementImpl._executeStreamingWithoutWaitingForTurn (https://deno.land/x/pgc4d/src/prepared_statement.ts:38:34)
    at PgConnImpl.queryStreaming (https://deno.land/x/pgc4d/src/connection.ts:302:27)
    at async PgConnImpl.query (https://deno.land/x/pgc4d/src/connection.ts:306:17)
jakajancar commented 4 years ago

I have added support for char & varchar, try master or v1.1.1. Others were already supported, see here:

https://github.com/jakajancar/pgc4d/blob/master/test/data_types_test.ts

I suspect you were looking at data_type_registry.ts that led you to believe only text and int were supported, but I suggest you read the comment. This is just what is needed to obtain type information about the other types in the database.

hiteshjasani commented 4 years ago

I'm hitting an issue with a column defined as date. Looks like it's missing from the tests.

error: Uncaught Error: Error sending param $2: Unsupported type: date (oid 1082, typsend date_send)
            throw new Error(`Unsupported type: ${type.typname} (oid ${typeOid}, typsend ${type.typsend})`)
                  ^
    at TypeRegistry.send (https://deno.land/x/pgc4d@v1.1.1/src/data_type_registry.ts:79:19)
    at https://deno.land/x/pgc4d@v1.1.1/src/prepared_statement.ts:106:47
    at Array.map (<anonymous>)
    at PreparedStatementImpl._serializeParams (https://deno.land/x/pgc4d@v1.1.1/src/prepared_statement.ts:102:23)
    at PreparedStatementImpl._executeStreamingWithoutWaitingForTurn (https://deno.land/x/pgc4d@v1.1.1/src/prepared_statement.ts:38:34)
    at PgConnImpl.queryStreaming (https://deno.land/x/pgc4d@v1.1.1/src/connection.ts:302:27)
    at async PgConnImpl.query (https://deno.land/x/pgc4d@v1.1.1/src/connection.ts:306:17)
jakajancar commented 4 years ago

Ah, sorry, noticed timestamp but missed date. Not sure there is a good native type for a date (Date is an instant in time, so not ok). What type of value would you expect? A “yyyy-MM-dd” string?

In the mean time you can also cast it to a string by doing ‘SELECT my_date::text’.

hiteshjasani commented 4 years ago

Javascript not having a proper Date object sucks. I'm not sure what the right answer is but I'd suggest doing what node-postgres does for Dates, return a Date object with hours, minutes, seconds set to zero. If people want a string, they can always do the my_date::text. This means they can also send a Date object and it will ignore the time components for inserting into the db.

This path will provide the least amount of confusion to people migrating over from node.

hiteshjasani commented 4 years ago

BTW, the particular code I'm working with right now is trying to set a date and I don't have a current workaround. Do you know of one?

I'm lookin at the type registry code and wishing it was more open and I could register my own converters.

Error sending param $2: Unsupported type: date (oid 1082, typsend date_send)
> select oid, typname, typelem, typarray, typisdefined, typtype, typinput, typoutput, typsend, typreceive from pg_type where typname like '%date' or typname like '%timestamp%' order by typname;
 oid  |   typname    | typelem | typarray | typisdefined | typtype |    typinput    |    typoutput    |     typsend      |    typreceive    
------+--------------+---------+----------+--------------+---------+----------------+-----------------+------------------+------------------
 1182 | _date        |    1082 |        0 | t            | b       | array_in       | array_out       | array_send       | array_recv
 1115 | _timestamp   |    1114 |        0 | t            | b       | array_in       | array_out       | array_send       | array_recv
 1185 | _timestamptz |    1184 |        0 | t            | b       | array_in       | array_out       | array_send       | array_recv
 1082 | date         |       0 |     1182 | t            | b       | date_in        | date_out        | date_send        | date_recv
 1114 | timestamp    |       0 |     1115 | t            | b       | timestamp_in   | timestamp_out   | timestamp_send   | timestamp_recv
 1184 | timestamptz  |       0 |     1185 | t            | b       | timestamptz_in | timestamptz_out | timestamptz_send | timestamptz_recv
(6 rows)
jakajancar commented 4 years ago

Re. serializing/deserializing the date as Javascript Date with time set to 0, I think that's a bad idea, since then sending such a Date to a browser (or some other party) might cause it to move a day forward or backward, based on timezone. Altogether looks as a very inappropriate type. In Java terminology, pg timestamp/js Date are an Instant, the pg date is a LocalDate:

A date without a time-zone in the ISO-8601 calendar system, such as 2007-12-03. LocalDate is an immutable date-time object that represents a date, often viewed as year-month-day. Other date fields, such as day-of-year, day-of-week and week-of-year, can also be accessed. For example, the value "2nd October 2007" can be stored in a LocalDate.

This class does not store or represent a time or time-zone. Instead, it is a description of the date, as used for birthdays. It cannot represent an instant on the time-line without additional information such as an offset or time-zone.

Encoding as a "yyyy-mm-dd" string is conceptually OK, although it adds little value over just letting postgres do it using ::text. I can then add 50 more converters for various other datatypes... 🤷‍♂️

Can you show me your query? I'm curious why you cannot just cast/"type hint" it using ::text. This works amazingly well in postgres, even for input parameters.

Adding support for custom converters should be possible to add fairly simply, although converting from postgres binary format is not super trivial.

hiteshjasani commented 4 years ago

Thank you for asking more questions and even volunteering to look at my query! Because of your question, I went back and tried various combinations. I did get it working using to_date() and to_timestamp() functions.

    Column     |            Type             | Collation | Nullable |                   Default                    
---------------+-----------------------------+-----------+----------+----------------------------------------------
 period_end_on | date                        |           | not null | 
 updated_at    | timestamp(0) with time zone |           | not null | 
  const now = Date.now() / 1000.0;
  const sEndOfDay: string = format(endOfDay, 'yyyy-MM-dd', {});
  console.log(`end of day = ${sEndOfDay}`);   // 2020-06-16
  try {
    const res = await db.query(`
        INSERT into ${METRICS_DAILY_TBL}(name,period_end_on,jdoc_ver,jdoc,updated_at)
        VALUES($1, to_date($2, 'YYYY-MM-DD'), $3, $4, to_timestamp($5))
        ON CONFLICT ON CONSTRAINT ${METRICS_DAILY_CONSTRAINT}
        DO UPDATE SET jdoc_ver = $3, jdoc = $4, updated_at = to_timestamp($5)`
      ,[name, sEndOfDay, version, jdoc, now]);

These other things were tried but did not work.

sql error
$2::TEXT PgError: column "period_end_on" is of type date but expression is of type text
$2::DATE Error: Error sending param $2: Unsupported type: date (oid 1082, typsend date_send)
jakajancar commented 4 years ago

In addition to to_date($2, 'YYYY-MM-DD'), you should be able to also do $2::text::date. The first causes postgres to indicate to pgc4d to send text. The second converts it to the date for insertion into the table. The only difference is that it works for all types and you don't have to find specific conversion functions.

You shouldn't need to_timestamp though, if you just stop dividing now by 1000 (thus converting it to integer).

If you contribute a pull request to handle yyyy-MM-dd strings for dates, I'll accept it.

grantcarthew commented 4 years ago

Hi @jakajancar. After opening https://github.com/brianc/node-postgres/issues/2225 I learned about the sub-module in the node-postgres project called pg-protocol. I suggest you have a good look at that. The project is MIT licensed so I see no reason for you to not just implement that code.

jakajancar commented 4 years ago

@hiteshjasani Date types are now mapped to yyyy-MM-dd strings.

@grantcarthew Thanks. That might have been an option had I known about it, but now it's pretty much done. It was a learning experience at least :)