Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to set intervalStyle globally? #154

Closed
nareshbhatia opened this issue Jun 5, 2016 · 29 comments
Closed

How to set intervalStyle globally? #154

nareshbhatia opened this issue Jun 5, 2016 · 29 comments
Labels

Comments

@nareshbhatia
Copy link

I would like to tell Postgres to return intervals as ISO 8601 strings always and on all connections. To do this I must set intervalStyle as follows:

SET intervalStyle = iso_8601

What's the best way to do this? I tried db.none('SET intervalStyle = iso_8601'); but that's not working. I suspect it is setting the value on only one connection.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

You would have to override the default parser for that, using pgp.pg.types. See pg-types.

I don't really know enough about the intervals to be more specific.

@nareshbhatia
Copy link
Author

nareshbhatia commented Jun 5, 2016

Hmmm, isn't there any way to just issue a SET intervalStyle to Postgres and have it return the desired format instead of converting using pgp.pg.types?

I am switching over from knex, and there I used to be able to do this:

var pool = {
    // Ask Postgres to return intervals as ISO 8601 strings
    afterCreate: function(connection, callback) {
        Promise.promisify(connection.query, connection)('SET intervalStyle = iso_8601;', [])
            .then(function() {
                callback(null, connection);
            });
    }
};

var dbConfig = {
    client: 'postgresql',
    connection: {
        host: 'localhost',
        user: '',
        password: '',
        database: 'staffer',
        charset: 'utf8'
    },
    pool: pool
};

var knex = require('knex')(dbConfig);

I was assuming that pool.afterCreate() is a feature at node-postgres level and I should be able to reuse it over here.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

Knex and this library use the exact same driver underneath.

You can do the same with this library, but it is a terrible hack, which means you are executing a query on every single connection, slowing down your communications.

Are you sure you want to do something like this?

What's even worse, it would execute the command on every virtual connection, since both libraries use the connection pool.

It would be a lot more efficient if you needed it within a separate task.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

A similar ugly hack via pg-promise would be:

var pgOptions = {
    connect: function(client){
        client.query('SET intervalStyle = iso_8601');
    }
};

I'm not sure it would work, but you can try.

The only proper way to do it is via the connection parameters: https://www.postgresql.org/docs/9.5/static/runtime-config-client.html, but their support by the driver is too limited.

@nareshbhatia
Copy link
Author

Thanks, Vitaly. I see that the feature described above is at the knex level - see here.

However, can you please clarify why this will slow down the communications? If I understand it correctly, SET intervalStyle will be issued only once per connection in the pool - not every time we are executing a general query.

Another consideration is that by default Postgres is sending intervals like this: { "minutes": 15 }. Writing a parser to convert this to ISO 8601 seems like needless effort when Postgres can do it for me.

Would love to hear your thoughts.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

That code in knex you showed me uses an exclusive Client instance. That cannot be a good idea, because clients outside of the pool do not scale well. One should always use the pool. Like I said, it is an ugly hack either way.

Can you not use it within a separate task? Are you sure you need it for the entire application?

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

By the way, this gave me an idea about adding a flag into event connect to indicate when it is a fresh connection. That would help in your case a bit ;)

@nareshbhatia
Copy link
Author

You just read my mind :-). I was going to ask you how connection pooling works. From your answer, I am assuming that a connection obtained from the pool can either be a fresh connection or a previously used connection. I would have to run SET intervalStyle only on fresh connections. Correct?

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

Yes. Can you try that hack in the meantime - to see if that even works?

@nareshbhatia
Copy link
Author

Just tried it. It is not working. Investigating.

Here's my code:

var pgpOptions = {
    promiseLib: Promise,
    extend: (obj: any) => {
        ...
    },
    connect: (client) => {
        // Ask Postgres to return intervals as ISO 8601 strings
        client.query('SET intervalStyle = iso_8601');
    }
};

When I console.log the interval property it prints this:

duration: PostgresInterval {},

However the same query in psql returns correctly:

# SET intervalStyle = iso_8601;
# select duration from projects where id = 40;
| duration |
------------
| PT1H     |

Also used to work in knex.

@nareshbhatia
Copy link
Author

Ah, I think I know what it is. In my knex implementation, I had to override pg-type for this.

// Keep interval as an ISO 8601 string. By default pg converts it to an object.
var parseInterval = function(val) {
    return val;
};

var INTERVAL_OID = 1186;
types.setTypeParser(INTERVAL_OID, parseInterval);

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

Does this mean that it now works for you? This was the solution I suggested from the beginning ;)

@nareshbhatia
Copy link
Author

nareshbhatia commented Jun 5, 2016

It is working now, but with both pieces of code applied (which is my intent):

  1. First tell Postgres to send intervals in ISO format: SET intervalStyle = iso_8601

  2. Then tell pg-types to not do any conversion on the returned value:

    var parseInterval = function(val) {
        return val;
    };
    

The combination works like a charm! Now if I could only have the fresh connection flag :-)

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

COOL. I am working on it now ;)

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

This seems to be possible to implement only for the JS client, but not for the Native Bindings. This is quite a bummer.

@nareshbhatia
Copy link
Author

It is a bummer. I am using the JS client though.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

I've got it working :)

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

@nareshbhatia released in v.4.4.3 ;)

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

I have figured out how to make this work for Native Bindings also. It ain't pretty, and not as efficient as for JS Bindings, but it works.

I will release it later.

@nareshbhatia
Copy link
Author

nareshbhatia commented Jun 5, 2016

@vitaly-t, thanks so much!

I am having bit of an issue compiling with TypeScript:

connect: (client, dc, fresh) => {
    // For fresh connections, ask Postgres to return intervals as ISO 8601 strings
    if (fresh === true || typeof fresh === undefined) {
        client.query('SET intervalStyle = iso_8601');
    }
}

I am getting this error:

error TS2345: Argument of type '{ promiseLib: PromiseConstructor; extend: (obj: any) => void; connect: (client: any, dc: any, fre...' is not assignable to parameter of type 'IOptions<{}>'.
  Types of property 'connect' are incompatible.
    Type '(client: any, dc: any, fresh: any) => void' is not assignable to type '(client: Client, dc: any) => void'.

Are the type definitions updated? I tried installing fresh type definitions, but I get this error:

$ typings install --save --global  github:vitaly-t/pg-promise
typings WARN badlocation "github:vitaly-t/pg-promise" is mutable and may change, consider specifying a commit hash
typings INFO reference Stripped reference "https://raw.githubusercontent.com/vitaly-t/pg-promise/master/typescript/pg-subset" during installation from "pg-promise" (main)
typings INFO reference Stripped reference "https://raw.githubusercontent.com/vitaly-t/pg-promise/master/typescript/pg-minify" during installation from "pg-promise" (main)
typings INFO reference Stripped reference "https://raw.githubusercontent.com/vitaly-t/pg-promise/master/typescript/ext-promise" during installation from "pg-promise" (main)

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

Ouch, in a rush, I forgot to update the typescript.

replace this line:

connect?:(client:pg.Client, dc:any) => void;

with this one:

connect?:(client:pg.Client, dc:any, fresh:boolean) => void;

I will fix it later ;)

@nareshbhatia
Copy link
Author

No worries.

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

@nareshbhatia All done, see v.4.4.4

@vitaly-t
Copy link
Owner

vitaly-t commented Jun 5, 2016

I have completely refactored it in 4.4.5, making it much simpler, and with much better performance.

@nareshbhatia
Copy link
Author

@vitaly-t, thanks so much for all your help on this issue. All working smoothly now.

@tmtron
Copy link

tmtron commented Feb 11, 2020

Maybe I missed something, but I think it should be as simple as returning PostgresInterval.toISO() from the type-parser:

pgp.pg.types.setTypeParser(INTERVAL_OID, stringVal => PostgresInterval(stringVal).toISO());

Then the database can send any string that postgres-interval understands and your application gets an ISO string without needing to init the connection.

@vitaly-t
Copy link
Owner

vitaly-t commented Feb 11, 2020

@tmtron It may be that simple, I don't know. But the issue did open a good possibility to configure fresh connections automatically.

This API was updated since though, and the correct syntax now is like this:

connect(client, dc, useCount) {
    // For fresh connections, ask Postgres to return intervals as ISO 8601 strings:
    const isFreshConnection = useCount === 0;
    if (isFreshConnection) {
        client.query('SET intervalStyle = iso_8601');
    }
}

@tmtron
Copy link

tmtron commented Feb 12, 2020

@vitaly-t this has the same effect as using the pg-pool connect event db.$pool.on('connect', .), right?

@vitaly-t
Copy link
Owner

vitaly-t commented Feb 12, 2020

@tmtron I can't say for sure, those are two separate things. This library does not use the connect event, it relies only on the pool allocating connections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants