Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Wed, Feb 20, 2013 at 2:16 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Where does this leave the PQconninfoParseParams/PQconninfodefaultsMerge patch? I'm not sure. Somehow I thought it would be necessary for this work, but it wasn't. I didn't remember that we already have PQconninfoParse() function, which was enough. So, what's the use case for those functions? I don't think that there is an immediate case. I still think they are useful, and would be more useful if we had some other functions that took PQconninfoOption. But the original reason for their being has been circumvented and I think we should just push them off to next release commit fest and discuss them then. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila amit.kap...@huawei.com wrote: On Tuesday, February 12, 2013 2:49 AM Heikki Linnakangas wrote: On 04.02.2013 17:32, Alvaro Herrera wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.com wrote: I think this patch would simplift the patch to pass a connection string to pg_basebackup and pg_receivexlog: http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@ko...@huawei.com. I note that pg_dumpall also has a similar issue as pg_basebackup and pg_receivexlog; there's no way to pass a connection string to it either. I have looked into both patches and my analysis is as below: In pg_basebackup patch, we have connection string and list of keywords (individual options specified by user), in the current available patch it has combined connection string and list of keywords as connection string and called PQconnectdb() which takes connection string as input. Now the patch of Phil Sober provides 2 new API's PQconninfoParseParams(), and PQconninfodefaultsMerge(), using these API's I can think of below way for patch pass a connection string to pg_basebackup, ... 1. Call existing function PQconinfoParse() with connection string input by user and get PQconninfoOption. 2. Now use the existing keywords (individual options specified by user) and extract the keywords from PQconninfoOption structure and call new API PQconninfoParseParams() which will return PQconninfoOption. The PQconninfoOption structure returned in this step will contain all keywords 3. Call PQconninfodefaultsMerge() to merge any default values if exist. Not sure if this step is required? 4. Extract individual keywords from PQconninfoOption structure and call PQconnectdbParams. Is this inline with what you have in mind or you have thought of some other simpler way of using new API's? With Regards, Amit Kapila. I think what would be nice is an additional connect function that took PQconninfoOption as a parameter. Or at least something that can convert from PQconninfoOption to a connection string or keyword/value arrays. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Btrfs clone WIP patch
On Wed, Feb 13, 2013 at 5:48 PM, Josh Berkus j...@agliodbs.com wrote: On 02/13/2013 02:13 PM, Tom Lane wrote: The big-picture question of course is whether we want to carry and maintain a filesystem-specific hack. I don't have a sense that btrfs is so widely used as to justify this. If this is a valuable hack, it seems like it could work on ZFS as well. If we could make it for any snapshot-capable filesystem, and not just BTRFS, then it would make more sense. I was thinking that too, but I think this is a file level clone, not a whole filesystem. As far as I can tell, you can't clone individual files in ZFS. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 11, 2013 at 4:19 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 04.02.2013 17:32, Alvaro Herrera wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.com wrote: Uh, no existing code can use this new functionality? That seems disappointing. I wrote this because I wanted to use it in pg_isready. I also wrote something for pg_isready to get around not having this. I think adding these two functions to libpq would be the better option, but wanted something that could fix an existing issue without having to patch libpq so late in the 9.3 development process. Actually, I think it was you who suggested that approach. Yes, I realize that (and yes, I did). But is no code other than pg_isready doing this? Not even the libpq URI test program? I think it probably would be able to benefit from this. Are you suggesting I patch that too? I thought it was usually frowned upon to touch random bits of working code like that. I'd be more than happy to do it if it helps build the case for getting this added. Well, this qualifies as refactoring, so yeah, it helps the case. I think this patch would simplift the patch to pass a connection string to pg_basebackup and pg_receivexlog: http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@ko...@huawei.com. I note that pg_dumpall also has a similar issue as pg_basebackup and pg_receivexlog; there's no way to pass a connection string to it either. If you could come up with a unified patch that takes care of all of those, that would be great. I will take a look. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Fri, Feb 8, 2013 at 1:07 PM, Phil Sorber p...@omniti.com wrote: On Fri, Feb 8, 2013 at 12:46 PM, Fujii Masao masao.fu...@gmail.com wrote: No maybe. But I think that all the client commands should follow the same rule. Otherwise a user would get confused when specifying options. I would consider the rest of the apps using it as a consensus. I will make sure it aligns in behavior. I've done as you suggested, and made sure they align with other command line utils. What I have found is that dbname is passed (almost) last in the param array so that it clobbers all previous values. I have made this patch as minimal as possible basing it off of master and not off of my previous attempt. For the record I still like the overall design of my previous attempt better, but I have not included a new version based on that here so as not to confuse the issue, however I would gladly do so upon request. Updated patch attached. Regards, -- Fujii Masao pg_isready_con_str_v4.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Fri, Feb 8, 2013 at 12:46 PM, Fujii Masao masao.fu...@gmail.com wrote: No maybe. But I think that all the client commands should follow the same rule. Otherwise a user would get confused when specifying options. I would consider the rest of the apps using it as a consensus. I will make sure it aligns in behavior. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Considering Gerrit for CFs
On Fri, Feb 8, 2013 at 10:20 AM, Peter Eisentraut pete...@gmx.net wrote: On 2/8/13 5:23 AM, Magnus Hagander wrote: But do you have any actual proof that the problem is in we loose reviewers because we're relying on email? Here is one: Me. Just yesterday I downloaded a piece of software that was previously unknown to me from GitHub and found a bug. Within 15 minutes or so I had fixed the bug, made a fork, sent a pull request. Today I read, the fix was merged last night, and I'm happy. How would this go with PostgreSQL? You can use the bug form on the web site, but you can't attach any code, so the bug will just linger and ultimately put more burden on a core contributor to deal with the minutiae of developing, testing, and committing a trivial fix and sending feedback to the submitter. Or the user could take the high road and develop and patch and submit it. Just make sure it's in context diff format! Search the wiki if you don't know how to do that! Send it to -hackers, your email will be held for moderation. We won't actually do anything with your patch, but we will tell you to add it to that commitfest app over there. You need to sign up for an account to use that. We will deal with your patch in one or two months. But only if you review another patch. And you should sign up for that other mailing list, to make sure you're doing it right. Chances are, the first review you're going to get is that your patch doesn't apply anymore, but which time you will have lost interest in the patch anyway. This. This times 1000. So, I don't have any further evidence that we are losing reviewers, but in light of the above and the options out there were interested developers can contribute much more easily, I'm amazed that we are getting any new contributors or reviewers at all. Of course, Gerrit doesn't actually address most of the issues above, but it could be part of a step forward. I'm not sure if Gerrit specifically is the answer, but there are definitely better ways to do code review like this. I really like the way github allows you to post a patch and then have conversation around it, offer comments on specific lines of code, and add updates to the patch all in one interface. Another benefit is that a lot more people are familiar and comfortable with this work flow. There are even some open source work-a-likes that we could use to we don't have to rely on a 3rd party like github. Gerrit seems to do it slightly differently with side by side diff's and patch revisions, but either way would be an improvement. I understand there are other concerns in this thread, like email, etc. I don't have a comprehensive plan that solves all this, but I wanted to add my +1 to the idea of something more sophisticated when it comes to code review. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] palloc unification
On Wed, Feb 6, 2013 at 9:59 AM, Simon Riggs si...@2ndquadrant.com wrote: On 6 February 2013 14:38, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas escribió: On Mon, Feb 4, 2013 at 5:50 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I propose to have a new subdirectory src/include/shared, and two header files: The frontend (pg_malloc) function definitions would live somewhere in, say, src/shared/fe_memutils.c. For the palloc() implementation, I think we should create another file, so that frontend-only programs that do not require those symbols (most of them) are not unnecessarily bloated. Opinions on having a new subdir? I like the idea of having a place for shared frontend and backend code very much, but I don't think src/include/shared or src/shared is a good name, because shared can mean a lot of things - like shared library, for example. I think that this should be set up in a manner analogous to libpgport, except not for portability code, but instead for other stuff. Maybe we could call it libpgframework or something. Yeah, I am doing this right now and the shared name doesn't seem so good. libpgframework sounds decent. So since libpgport comes from src/port, are we okay with src/framework and src/include/framework? common ? src/backend/common src/include/common +1 -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Tue, Feb 5, 2013 at 12:44 PM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:08 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. Surely the idea is that you can also give it a postgres:// URI, right? Absolutely. Here is it. I like this approach more than the previous one, but I'd like some feedback. Minor adjustment. There still seems to be a bit of a disconnect in libpq in my opinion. Taking options as a string (URI or conninfo) or a set of arrays, but returning info about connection parameters in PQconninfoOption? And nothing that takes that as an input. Seems odd to me. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services pg_isready_con_str_v3.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Feb 6, 2013 at 11:11 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Feb 7, 2013 at 12:05 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 12:44 PM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:08 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. Surely the idea is that you can also give it a postgres:// URI, right? Absolutely. Here is it. I like this approach more than the previous one, but I'd like some feedback. The patch looks complicated to me. I was thinking that we can address the problem just by using PQconninfoParse() and PQconndefaults() like uri-regress.c does. The patch should be very simple. Why do we need so complicated code? Did you like the previous version better? http://www.postgresql.org/message-id/cadakt-hnb3ohcpkr+pcg1c_bjrsb7j__bpv+-jrjs5opjr2...@mail.gmail.com Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] get_progname() should not be const char *?
On Wed, Feb 6, 2013 at 11:22 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Feb 5, 2013 at 12:18 AM, Phil Sorber p...@omniti.com wrote: On Mon, Feb 4, 2013 at 10:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: I don't believe that callers should be trying to free() the result. Whether it's been strdup'd or not is not any of their business. Is that just because of the nature of this specific function? I can't presume to speak for Tom, but I think so. Sometimes the API of a function includes the notion that the caller should pfree the result. Sometimes it doesn't. The advantage of NOT including that in the API contract is that you can sometimes do optimizations that would be impossible otherwise - e.g. you can return the same palloc'd string on successive calls to the function; or you can sometimes return a statically allocated string. Yeah. In this particular case, it seems rather obvious that the function should be returning the same string each time --- if it's actually doing a fresh malloc, that sounds like a bug. It does, but it's noted in a comment that it's only expected to be run once. But in any case, adding or removing a const qualifier from a function's result typically goes along with an API-contract change as to whether the caller is supposed to free the result or not. My objection here was specifically that I don't believe the contract for get_progname includes caller-free now, and I don't want it to start being that. That's fair. Thanks for the explanation. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Feb 6, 2013 at 11:36 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Feb 7, 2013 at 1:15 AM, Phil Sorber p...@omniti.com wrote: On Wed, Feb 6, 2013 at 11:11 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Feb 7, 2013 at 12:05 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 12:44 PM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:08 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. Surely the idea is that you can also give it a postgres:// URI, right? Absolutely. Here is it. I like this approach more than the previous one, but I'd like some feedback. The patch looks complicated to me. I was thinking that we can address the problem just by using PQconninfoParse() and PQconndefaults() like uri-regress.c does. The patch should be very simple. Why do we need so complicated code? Did you like the previous version better? http://www.postgresql.org/message-id/cadakt-hnb3ohcpkr+pcg1c_bjrsb7j__bpv+-jrjs5opjr2...@mail.gmail.com Yes because that version is simpler. But which version is better depends on the reason why you implemented new version. If you have some idea about the merit and demerit of each version, could you elaborate them? I didn't like the way that I had to hard code the options in the first one as you pointed out below. I also was looking through the code for something else and saw that a lot of the apps were starting with defaults then building from there, rather than trying to add the defaults at the end. I think they were still doing it wrong because they were using getenv() on their own rather than asking libpq for the defaults though. So the new version gets the defaults at the beginning and also makes it easy to add new params without changing function definitions. + set_connect_options(connect_options, pgdbname, pghost, pgport, connect_timeout, pguser); This code prevents us from giving options other than the above, for example application_name, in the conninfo. I think that pg_isready should accept all the libpq options. I'm with you there. The new version fixes that as well. When more than one -d options are specified, psql always prefer the last one and ignore the others. OTOH, pg_isready with this patch seems to merge them. I'm not sure if there is specific rule about the priority order of -d option. But it seems better to follow the existing way, i.e., always prefer the last -d option. The problem I am having here is resolving the differences between different -d options and other command line options. For example: -h foo -p 4321 -d host=bar port=1234 -d host=baz I would expect that to be 'baz:1234' but you are saying it should be 'baz:4321'? I look at -d as just a way to pass in multiple options (when you aren't strictly passing in dbname) and should be able to expand the above example to: -h foo -p 4321 -h bar -p 1234 -h baz If we hold off on parsing the value of -d until the end so we are sure we have the last one, then we might lose other parameters that were after the -d option. For example: -h foo -p 4321 -d host=bar port=1234 -d host=baz user=you -U me Should this be 'me@baz:1234' or 'you@baz:4321' or 'me@baz:4321'? So we would have to track the last instance of a parameter as well as the order those final versions came in. Sound to me like there is no clear answer there, but maybe there is a project consensus that has already been reached with regard to this? Or some general computing wisdom that applies? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. It works, but I don't really like it all that much, honestly. I also submitted a patch to add on to libpq to handle this, but Alvaro posed some questions I don't have good answers for. So I actually have another patch brewing that I actually like, but I need to put the finishing touches on. I plan on submitting that later this morning. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. Surely the idea is that you can also give it a postgres:// URI, right? Absolutely. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Tue, Feb 5, 2013 at 9:08 AM, Phil Sorber p...@omniti.com wrote: On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas robertmh...@gmail.com wrote: On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber p...@omniti.com wrote: OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. I'm sorry, can you remind me what this does for us vs. the existing coding? It's supposed to handle the connection string passed as dbname case to be able to get the right output for host:port. Surely the idea is that you can also give it a postgres:// URI, right? Absolutely. Here is it. I like this approach more than the previous one, but I'd like some feedback. There still seems to be a bit of a disconnect in libpq in my opinion. Taking options as a string (URI or conninfo) or a set of arrays, but returning info about connection parameters in PQconninfoOption? And nothing that takes that as an input. Seems odd to me. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services pg_isready_con_str_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.com wrote: This patch came up from discussion about pg_isready. PQconninfoParseParams is similar to PQconninfoParse but takes two arrays like PQconnectdbParams. It essentially exposes conninfo_array_parse(). PQconninfodefaultsMerge essentially exposes conninfo_add_defaults(). It allows you to pass a PQconninfoOption struct and it adds defaults for all NULL values. Uh, no existing code can use this new functionality? That seems disappointing. I wrote this because I wanted to use it in pg_isready. I also wrote something for pg_isready to get around not having this. I think adding these two functions to libpq would be the better option, but wanted something that could fix an existing issue without having to patch libpq so late in the 9.3 development process. Actually, I think it was you who suggested that approach. So long answer short, there is existing code that can use this functionality immediately. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Uh, no existing code can use this new functionality? That seems disappointing. I wrote this because I wanted to use it in pg_isready. I also wrote something for pg_isready to get around not having this. I think adding these two functions to libpq would be the better option, but wanted something that could fix an existing issue without having to patch libpq so late in the 9.3 development process. Actually, I think it was you who suggested that approach. Yes, I realize that (and yes, I did). But is no code other than pg_isready doing this? Not even the libpq URI test program? I think it probably would be able to benefit from this. Are you suggesting I patch that too? I thought it was usually frowned upon to touch random bits of working code like that. I'd be more than happy to do it if it helps build the case for getting this added. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] get_progname() should not be const char *?
get_progname() returns a strdup()'d value. Shouldn't it then be simply
char * and not const char *? Otherwise free() complains loudly without
a cast.
diff --git a/src/include/port.h b/src/include/port.h
new file mode 100644
index 99d3a9b..2d6a435
*** a/src/include/port.h
--- b/src/include/port.h
*** extern void make_native_path(char *path)
*** 45,51
extern bool path_contains_parent_reference(const char *path);
extern bool path_is_relative_and_below_cwd(const char *path);
extern bool path_is_prefix_of_path(const char *path1, const char *path2);
! extern const char *get_progname(const char *argv0);
extern void get_share_path(const char *my_exec_path, char *ret_path);
extern void get_etc_path(const char *my_exec_path, char *ret_path);
extern void get_include_path(const char *my_exec_path, char *ret_path);
--- 45,51
extern bool path_contains_parent_reference(const char *path);
extern bool path_is_relative_and_below_cwd(const char *path);
extern bool path_is_prefix_of_path(const char *path1, const char *path2);
! extern char *get_progname(const char *argv0);
extern void get_share_path(const char *my_exec_path, char *ret_path);
extern void get_etc_path(const char *my_exec_path, char *ret_path);
extern void get_include_path(const char *my_exec_path, char *ret_path);
diff --git a/src/port/path.c b/src/port/path.c
new file mode 100644
index 72ca24c..ebfc94c
*** a/src/port/path.c
--- b/src/port/path.c
*** path_is_prefix_of_path(const char *path1
*** 411,417
* Extracts the actual name of the program as called -
* stripped of .exe suffix if any
*/
! const char *
get_progname(const char *argv0)
{
const char *nodir_name;
--- 411,417
* Extracts the actual name of the program as called -
* stripped of .exe suffix if any
*/
! char *
get_progname(const char *argv0)
{
const char *nodir_name;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] get_progname() should not be const char *?
On Mon, Feb 4, 2013 at 10:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: get_progname() returns a strdup()'d value. Shouldn't it then be simply char * and not const char *? Otherwise free() complains loudly without a cast. I don't believe that callers should be trying to free() the result. Whether it's been strdup'd or not is not any of their business. Is that just because of the nature of this specific function? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Tue, Jan 29, 2013 at 11:43 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jan 29, 2013 at 3:12 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao masao.fu...@gmail.com wrote: Maybe. But I'm not inclined to add new libpq interface at this stage. Because we are in the last CommitFest and I'm not sure whether we have enough time to implement that. Instead, how about using both PQconninfoParse() and PQconndefaults() like uri-regress.c does? Or just remove that output message? At least I don't think that the information about host and port needs to be output. Which would make the code very simpler. I think that output is important as do others up thread. I think it'd be simpler to just disable dbname's ability to double as conninfo. If we are looking for easy, that is. I don't mind duplicating the behavior from conninfo_array_parse() or uri-regress.c either. I can just put a comment that at some point in the future we should add a libpq interface for it. I suggest duplicate the code for 9.3, and submit a patch to refactor into a new libpq function for CF2013-next. If the patch is simple enough, we can consider putting it into 9.3. Agreed. Regards, -- Fujii Masao OK, here is the patch that handles the connection string in dbname. I'll post the other patch under a different posting because I am sure it will get plenty of debate on it's own. pg_isready_con_str.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
This patch came up from discussion about pg_isready. PQconninfoParseParams is similar to PQconninfoParse but takes two arrays like PQconnectdbParams. It essentially exposes conninfo_array_parse(). PQconninfodefaultsMerge essentially exposes conninfo_add_defaults(). It allows you to pass a PQconninfoOption struct and it adds defaults for all NULL values. There are no docs yet. I assumed I would let bikeshedding ensue, and also debate on whether we even want these first. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Sun, Feb 3, 2013 at 1:37 AM, Magnus Hagander mag...@hagander.net wrote: On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.com wrote: This patch came up from discussion about pg_isready. PQconninfoParseParams is similar to PQconninfoParse but takes two arrays like PQconnectdbParams. It essentially exposes conninfo_array_parse(). PQconninfodefaultsMerge essentially exposes conninfo_add_defaults(). It allows you to pass a PQconninfoOption struct and it adds defaults for all NULL values. There are no docs yet. I assumed I would let bikeshedding ensue, and also debate on whether we even want these first. I think you forgot to attach the patch. /Magnus /me hangs head in shame :-( Here is is... libpq_params_parse_merge.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao masao.fu...@gmail.com wrote: Maybe. But I'm not inclined to add new libpq interface at this stage. Because we are in the last CommitFest and I'm not sure whether we have enough time to implement that. Instead, how about using both PQconninfoParse() and PQconndefaults() like uri-regress.c does? Or just remove that output message? At least I don't think that the information about host and port needs to be output. Which would make the code very simpler. I think that output is important as do others up thread. I think it'd be simpler to just disable dbname's ability to double as conninfo. If we are looking for easy, that is. I don't mind duplicating the behavior from conninfo_array_parse() or uri-regress.c either. I can just put a comment that at some point in the future we should add a libpq interface for it. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Mon, Jan 28, 2013 at 1:12 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Mon, Jan 28, 2013 at 11:20 AM, Fujii Masao masao.fu...@gmail.com wrote: Maybe. But I'm not inclined to add new libpq interface at this stage. Because we are in the last CommitFest and I'm not sure whether we have enough time to implement that. Instead, how about using both PQconninfoParse() and PQconndefaults() like uri-regress.c does? Or just remove that output message? At least I don't think that the information about host and port needs to be output. Which would make the code very simpler. I think that output is important as do others up thread. I think it'd be simpler to just disable dbname's ability to double as conninfo. If we are looking for easy, that is. I don't mind duplicating the behavior from conninfo_array_parse() or uri-regress.c either. I can just put a comment that at some point in the future we should add a libpq interface for it. I suggest duplicate the code for 9.3, and submit a patch to refactor into a new libpq function for CF2013-next. If the patch is simple enough, we can consider putting it into 9.3. Ok, sounds good to me. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Fri, Jan 25, 2013 at 11:20 AM, Fujii Masao masao.fu...@gmail.com wrote: On Fri, Jan 25, 2013 at 4:10 AM, Phil Sorber p...@omniti.com wrote: On Thu, Jan 24, 2013 at 1:12 PM, Fujii Masao masao.fu...@gmail.com wrote: set_pglocale_pgservice() should be called? I think that the command name (i.e., pg_isready) should be given to PQpingParams() as fallback_application_name. Otherwise, the server by default uses unknown as the application name of pg_isready. It's undesirable. Why isn't the following message output only when invalid option is specified? Try \%s --help\ for more information. I've updated the patch to address these three issues. Attached. When the conninfo string including the hostname or port number is specified in -d option, pg_isready displays the wrong information as follows. $ pg_isready -d port= /tmp:5432 - no response This is what i asked about in my previous email about precedence of the parameters. I can parse that with PQconninfoParse, but what are the rules for merging both individual and conninfo params together? If I read conninfo_array_parse() correctly, PQpingParams() prefer the option which is set to its keyword array later. It would be really nice to expose conninfo_array_parse() or some wrapped version directly to a libpq consumer. Otherwise, I need to recreate this behavior in pg_isready.c. Thoughts on adding: PQconninfoOption *PQparamsParse(const char **keywords, const char **values, char **errmsg, bool use_defaults, int expand_dbname) or similar? Or perhaps there is a better way to accomplish this that I am not aware of? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Sun, Jan 27, 2013 at 2:38 PM, Phil Sorber p...@omniti.com wrote: On Fri, Jan 25, 2013 at 11:20 AM, Fujii Masao masao.fu...@gmail.com wrote: On Fri, Jan 25, 2013 at 4:10 AM, Phil Sorber p...@omniti.com wrote: On Thu, Jan 24, 2013 at 1:12 PM, Fujii Masao masao.fu...@gmail.com wrote: set_pglocale_pgservice() should be called? I think that the command name (i.e., pg_isready) should be given to PQpingParams() as fallback_application_name. Otherwise, the server by default uses unknown as the application name of pg_isready. It's undesirable. Why isn't the following message output only when invalid option is specified? Try \%s --help\ for more information. I've updated the patch to address these three issues. Attached. When the conninfo string including the hostname or port number is specified in -d option, pg_isready displays the wrong information as follows. $ pg_isready -d port= /tmp:5432 - no response This is what i asked about in my previous email about precedence of the parameters. I can parse that with PQconninfoParse, but what are the rules for merging both individual and conninfo params together? If I read conninfo_array_parse() correctly, PQpingParams() prefer the option which is set to its keyword array later. It would be really nice to expose conninfo_array_parse() or some wrapped version directly to a libpq consumer. Otherwise, I need to recreate this behavior in pg_isready.c. Thoughts on adding: PQconninfoOption *PQparamsParse(const char **keywords, const char **values, char **errmsg, bool use_defaults, int expand_dbname) or similar? Or perhaps there is a better way to accomplish this that I am not aware of? It would also be nice to be able to pass user_defaults to PQconninfoParse(). Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Sat, Jan 26, 2013 at 4:02 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello We now haw to solve small puppet issue, because our puppets try to start server too early, when old instance live still. Maybe some new parameter - is_done can be useful. What about something like: pg_isready; while [ $? -ne 2 ]; do sleep 1; pg_isready; done Perhaps with a counter to break out of the loop after some number of attempts. Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Sat, Jan 26, 2013 at 11:53 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 4:02 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello We now haw to solve small puppet issue, because our puppets try to start server too early, when old instance live still. Maybe some new parameter - is_done can be useful. What about something like: pg_isready; while [ $? -ne 2 ]; do sleep 1; pg_isready; done it is not enough - server is done in a moment, where can be started again - or when we can do safe copy of database data directory. I guess i am not completely understanding the case you are trying to solve. Can you explain a bit further? Regards Pavel Perhaps with a counter to break out of the loop after some number of attempts. Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Sat, Jan 26, 2013 at 12:39 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 11:53 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 4:02 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello We now haw to solve small puppet issue, because our puppets try to start server too early, when old instance live still. Maybe some new parameter - is_done can be useful. What about something like: pg_isready; while [ $? -ne 2 ]; do sleep 1; pg_isready; done it is not enough - server is done in a moment, where can be started again - or when we can do safe copy of database data directory. I guess i am not completely understanding the case you are trying to solve. Can you explain a bit further? We use puppets and due some simplification we cannot to use reload when configuration is changed. Our puppets has not enough intelligence to understand when is reload enough and when is restart necessary. So any change to configuration require restarting postgres. I don't know why service restart are not used. I believe so our puppet guru know it. It just do sequence STOP:START Now puppets are smart and able to wait for time, when server is ready. But there are missing simple test if server is really done and I see a error messages related to too early try to start. So some important feature can be verification so server is really done. We can do it with test on pid file now - and probably we will use it. But I see so this is similar use case (in opposite direction) I guess I am still not clear why you can't do: stop_pg_via_puppet pg_isready while [ $? -ne 2 ] do sleep 1 pg_isready done do_post_stop_things start_pg_via_puppet Regards Pavel Regards Pavel Perhaps with a counter to break out of the loop after some number of attempts. Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Request for vote to move forward with recovery.conf overhaul
On Wed, Jan 23, 2013 at 6:36 AM, Michael Paquier michael.paqu...@gmail.com wrote: On 2013/01/23, at 18:12, Simon Riggs si...@2ndquadrant.com wrote: On 23 January 2013 04:49, Michael Paquier michael.paqu...@gmail.com wrote: - recovery.conf is removed (no backward compatibility in this version of the patch) If you want to pursue that, you know where it leads. No, rebasing a rejected patch doesn't help, its just relighting a fire that shouldn't ever have been lit. Pushing to do that out of order is just going to drain essential time out of this CF from all of us. No problem to support both. The only problem I see is if the same parameter is defined in recovery.conf and postgresql.conf, is the priority given to recovery.conf? I would think that if someone created a recovery.conf file they would expect that to be given priority. Otherwise they would know that was a deprecated method and would set it in postgresql.conf only. -- Michael Paquier http://michael.otacoo.com (Sent from my mobile phone) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Sat, Jan 26, 2013 at 4:37 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 12:39 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 11:53 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 4:02 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello We now haw to solve small puppet issue, because our puppets try to start server too early, when old instance live still. Maybe some new parameter - is_done can be useful. What about something like: pg_isready; while [ $? -ne 2 ]; do sleep 1; pg_isready; done it is not enough - server is done in a moment, where can be started again - or when we can do safe copy of database data directory. I guess i am not completely understanding the case you are trying to solve. Can you explain a bit further? We use puppets and due some simplification we cannot to use reload when configuration is changed. Our puppets has not enough intelligence to understand when is reload enough and when is restart necessary. So any change to configuration require restarting postgres. I don't know why service restart are not used. I believe so our puppet guru know it. It just do sequence STOP:START Now puppets are smart and able to wait for time, when server is ready. But there are missing simple test if server is really done and I see a error messages related to too early try to start. So some important feature can be verification so server is really done. We can do it with test on pid file now - and probably we will use it. But I see so this is similar use case (in opposite direction) I guess I am still not clear why you can't do: stop_pg_via_puppet pg_isready while [ $? -ne 2 ] do sleep 1 pg_isready done do_post_stop_things start_pg_via_puppet because ! pg_isready pg_isdone So you are proposing a different utility? Sorry, I thought you were proposing a new option to pg_isready. What would pg_isdone be testing for specifically? Is this something that would block until it has confirmed a shutdown? Regards Pavel Regards Pavel Perhaps with a counter to break out of the loop after some number of attempts. Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Jan 26, 2013 6:56 PM, Craig Ringer cr...@2ndquadrant.com wrote: On 01/27/2013 06:20 AM, Phil Sorber wrote: On Sat, Jan 26, 2013 at 4:37 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 12:39 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 11:53 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/1/26 Phil Sorber p...@omniti.com: On Sat, Jan 26, 2013 at 4:02 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello We now haw to solve small puppet issue, because our puppets try to start server too early, when old instance live still. Maybe some new parameter - is_done can be useful. What about something like: pg_isready; while [ $? -ne 2 ]; do sleep 1; pg_isready; done it is not enough - server is done in a moment, where can be started again - or when we can do safe copy of database data directory. I guess i am not completely understanding the case you are trying to solve. Can you explain a bit further? We use puppets and due some simplification we cannot to use reload when configuration is changed. Our puppets has not enough intelligence to understand when is reload enough and when is restart necessary. So any change to configuration require restarting postgres. I don't know why service restart are not used. I believe so our puppet guru know it. It just do sequence STOP:START Now puppets are smart and able to wait for time, when server is ready. But there are missing simple test if server is really done and I see a error messages related to too early try to start. So some important feature can be verification so server is really done. We can do it with test on pid file now - and probably we will use it. But I see so this is similar use case (in opposite direction) I guess I am still not clear why you can't do: stop_pg_via_puppet pg_isready while [ $? -ne 2 ] do sleep 1 pg_isready done do_post_stop_things start_pg_via_puppet because ! pg_isready pg_isdone So you are proposing a different utility? Sorry, I thought you were proposing a new option to pg_isready. What would pg_isdone be testing for specifically? Is this something that would block until it has confirmed a shutdown? That's what it sounds like - confirming that PostgreSQL is really fully shut down. I'm not sure how you could do that over a protocol connection, myself. I'd just read the postmaster pid from the pidfile on disk and then `kill -0` it in a delay loop until the `kill` command returns failure. This could be a useful convenience utility but I'm not convinced it should be added to pg_isready because it requires local and possibly privileged execution, unlike pg_isready's network based operation. Privileges could be avoided by using an aliveness test other than `kill -0`, but you absolutely have to be local to verify that the postmaster has fully terminated - and it wouldn't make sense for a non-local process to care about this anyway. Maybe something to add to pg_ctl? -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] My first patch! (to \df output)
On Thu, Jan 24, 2013 at 2:27 AM, Craig Ringer cr...@2ndquadrant.com wrote: On 01/24/2013 01:50 AM, Phil Sorber wrote: This looks good to me now. Compiles and works as described. Ready to go? https://commitfest.postgresql.org/action/patch_view?id=1008 I guess I wasn't ready to be so bold, but sure. :) I changed it to 'ready for committer'. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Jan 23, 2013 at 8:12 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Jan 24, 2013 at 8:47 AM, Phil Sorber p...@omniti.com wrote: On Wed, Jan 23, 2013 at 6:07 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: On Wed, Jan 23, 2013 at 1:58 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Jan 23, 2013 at 12:27:45PM -0500, Tom Lane wrote: +1 for default timeout --- if this isn't like ping where you are expecting to run indefinitely, I can't see that it's a good idea for it to sit very long by default, in any circumstance. FYI, the pg_ctl -w (wait) default is 60 seconds: Great. That is what I came to on my own as well. Figured that might be a sticking point, but as there is precedent, I'm happy with it. I'm not sure that's a relevant precedent at all. What that number is is the time that pg_ctl will wait around for the postmaster to start or stop before reporting a problem --- and in either case, a significant delay (multiple seconds) is not surprising, because of crash-recovery work, shutdown checkpointing, etc. For pg_isready, you'd expect to get a response more or less instantly, wouldn't you? Personally, I'd decide that pg_isready is broken if it didn't give me an answer in a couple of seconds, much less a minute. What I had in mind was a default timeout of maybe 3 or 4 seconds... I was thinking that if it was in a script you wouldn't care if it was 60 seconds. If it was at the command line you would ^C it much earlier. I think the default linux TCP connection timeout is around 20 seconds. My feeling is everyone is going to have a differing opinion on this, which is why I was hoping that some good precedent existed. I'm fine with 3 or 4, whatever can be agreed upon. +1 with 3 or 4 secounds. Aside from this issue, I have one minor comment. ISTM you need to add the following line to the end of the help message. This line has been included in the help message of all the other client commands. Report bugs to pgsql-b...@postgresql.org. Ok, I set the default timeout to 3 seconds, added the bugs email to the help, and also added docs that I forgot last time. Also, still hoping to get some feedback on my other issues. Thanks. Regards, -- Fujii Masao pg_isready_timeout_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Thu, Jan 24, 2013 at 1:12 PM, Fujii Masao masao.fu...@gmail.com wrote: set_pglocale_pgservice() should be called? I think that the command name (i.e., pg_isready) should be given to PQpingParams() as fallback_application_name. Otherwise, the server by default uses unknown as the application name of pg_isready. It's undesirable. Why isn't the following message output only when invalid option is specified? Try \%s --help\ for more information. I've updated the patch to address these three issues. Attached. When the conninfo string including the hostname or port number is specified in -d option, pg_isready displays the wrong information as follows. $ pg_isready -d port= /tmp:5432 - no response This is what i asked about in my previous email about precedence of the parameters. I can parse that with PQconninfoParse, but what are the rules for merging both individual and conninfo params together? For example if someone did: pg_isready -h foo -d host=bar port=4321 -p 1234 What should the connection parameters be? Regards, -- Fujii Masao pg_isready_timeout_v3.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] .gitignore additions
On Wed, Jan 23, 2013 at 1:32 AM, David Fetter da...@fetter.org wrote: On Wed, Jan 23, 2013 at 01:05:12PM +0800, Craig Ringer wrote: Hi all Would a committer be willing to pop some entries in .gitignore for Windows native build outputs? *.sln *.vcproj *.vcxproj It'd make life easier when testing Windows changes. While they're at it, it'd be nice to have tags from ctags (via our tools or otherwise) get ignored globally, along with cscope.out , as follows: tags /cscope.out +1 on cscope.out! Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] .gitignore additions
On Jan 23, 2013 8:59 AM, Andrew Dunstan and...@dunslane.net wrote: On 01/23/2013 08:47 AM, Phil Sorber wrote: On Wed, Jan 23, 2013 at 1:32 AM, David Fetter da...@fetter.org wrote: On Wed, Jan 23, 2013 at 01:05:12PM +0800, Craig Ringer wrote: Hi all Would a committer be willing to pop some entries in .gitignore for Windows native build outputs? *.sln *.vcproj *.vcxproj It'd make life easier when testing Windows changes. While they're at it, it'd be nice to have tags from ctags (via our tools or otherwise) get ignored globally, along with cscope.out , as follows: tags /cscope.out +1 on cscope.out! There doesn't seem anything postgres-specific about these. Pretty much everything we list is a byproduct of a standard build, not some other tool. man gitignore says Patterns which a user wants git to ignore in all situations (e.g., backup or temporary files generated by the user’s editor of choice) generally go into a file specified by core.excludesfile in the user’s ~/.gitconfig. That's a good point. Will do that instead. I would think tags files and cscope.out probably come into that category, although I don't have terribly strong feelings about it. cheers andrew
Re: [HACKERS] [PATCH] PQping Docs
On Wed, Jan 23, 2013 at 10:37 AM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 21, 2013 at 1:45 PM, Phil Sorber p...@omniti.com wrote: Attached is a patch that adds a note about the FATAL messages that appear in the logs if you don't pass a valid user or dbname to PQping or PQpingParams. This was requested in the pg_isready thread. Can I counter-propose the attached, which puts the relevant text closer to the scene of the crime? This seems reasonable to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Jan 23, 2013 at 11:07 AM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 21, 2013 at 12:23 PM, Phil Sorber p...@omniti.com wrote: Changing up the subject line because this is no longer a work in progress nor is it pg_ping anymore. OK, I committed this. However, I have one suggestion. Maybe it would be a good idea to add a -c or -t option that sets the connect_timeout parameter. Because: [rhaas pgsql]$ pg_isready -h www.google.com grows old, dies Oh, hrmm. Yes, I will address that with a follow up patch. I guess in my testing I was using a host that responded properly with port unreachable or TCP RST or something. Do you think we should have a default timeout, or only have one if specified at the command line? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Add Makefile dep in bin/scripts for libpgport
I get the following error when I try to compile just a specific binary in src/bin/scripts: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard reindexdb.o common.o dumputils.o kwlookup.o keywords.o -L../../../src/port -lpgport -L../../../src/interfaces/libpq -lpq -L../../../src/port -Wl,--as-needed -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags -lpgport -lz -lreadline -lcrypt -ldl -lm -o reindexdb /usr/bin/ld: cannot find -lpgport /usr/bin/ld: cannot find -lpgport collect2: error: ld returned 1 exit status make: *** [reindexdb] Error 1 It appears it is missing the libpgport dependency. Attached is a patch to correct that. This is not normally a problem because when building the whole tree libpgport is usually compiled already. scripts_makefile_pgport_dep.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] My first patch! (to \df output)
On Wed, Jan 23, 2013 at 12:31 AM, Jon Erdman postgre...@thewickedtribe.net wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Done. Attached. - -- Jon T Erdman (aka StuckMojo) PostgreSQL Zealot On 01/22/2013 11:17 PM, Phil Sorber wrote: On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman postgre...@thewickedtribe.net wrote: Updated the patch in commitfest with the doc change, and added a comment to explain the whitespace change (it was to clean up the sql indentation). I've also attached the new patch here for reference. Docs looks good. Spaces gone. Still need to replace 'definer' and 'invoker' with %s and add the corresponding gettext_noop() calls I think. This looks good to me now. Compiles and works as described. One thing I would note for the future though, when updating a patch, add a version to the file name to distinguish it from older versions of the patch. -BEGIN PGP SIGNATURE- Comment: Using GnuPG with undefined - http://www.enigmail.net/ iEYEARECAAYFAlD/dcoACgkQRAk1+p0GhSEKHQCZAW8UNqSjYxBgBvt2nuffrkrV +9AAn1hChpY5Jg8G8T3XmlIb+3VUSEQ2 =3cFD -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Jan 23, 2013 at 1:58 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Jan 23, 2013 at 12:27:45PM -0500, Tom Lane wrote: Phil Sorber p...@omniti.com writes: On Wed, Jan 23, 2013 at 11:07 AM, Robert Haas robertmh...@gmail.com wrote: [rhaas pgsql]$ pg_isready -h www.google.com grows old, dies Do you think we should have a default timeout, or only have one if specified at the command line? +1 for default timeout --- if this isn't like ping where you are expecting to run indefinitely, I can't see that it's a good idea for it to sit very long by default, in any circumstance. FYI, the pg_ctl -w (wait) default is 60 seconds: from pg_ctl.c: #define DEFAULT_WAIT60 Great. That is what I came to on my own as well. Figured that might be a sticking point, but as there is precedent, I'm happy with it. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Jan 23, 2013 at 2:51 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Jan 23, 2013 at 02:50:01PM -0500, Phil Sorber wrote: On Wed, Jan 23, 2013 at 1:58 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Jan 23, 2013 at 12:27:45PM -0500, Tom Lane wrote: Phil Sorber p...@omniti.com writes: On Wed, Jan 23, 2013 at 11:07 AM, Robert Haas robertmh...@gmail.com wrote: [rhaas pgsql]$ pg_isready -h www.google.com grows old, dies Do you think we should have a default timeout, or only have one if specified at the command line? +1 for default timeout --- if this isn't like ping where you are expecting to run indefinitely, I can't see that it's a good idea for it to sit very long by default, in any circumstance. FYI, the pg_ctl -w (wait) default is 60 seconds: from pg_ctl.c: #define DEFAULT_WAIT60 Great. That is what I came to on my own as well. Figured that might be a sticking point, but as there is precedent, I'm happy with it. Yeah, being able to point to precedent is always helpful. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + Attached is the patch to add a connect_timeout. I also factored out the setting of user and dbname from the default gathering loop as we only need host and port defaults and made more sense to handle user/dbname in the same area of the code as connect_timeout. This was something mentioned by Robert upthread. This also coincidentally fixes a bug in the size of the keywords and values arrays. Must have added an option during review and not extended that array. Is there a best practice to making sure that doesn't happen in the future? I was thinking define MAX_PARAMS and then setting the array size to MAX_PARAMS+1 and then checking in the getopt loop to see how many params we expect to process and erroring if we are doing to many, but that only works at runtime. One other thing I noticed while refactoring the defaults gathering code. If someone passes in a connect string for dbname, we output the wrong info at the end. This is not addressed in this patch because I wanted to get some feedback before fixing and make a separate patch. I see the only real option being to use PQconninfoParse to get the params from the connect string. If someone passes in a connect string via dbname should that have precedence over other values passed in via individual command line options? Should ordering of the command line options matter? For example if someone did: pg_isready -h foo -d host=bar port=4321 -p 1234 What should the connection parameters be? pg_isready_timeout.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CF3+4 (was Re: Parallel query execution)
On Wed, Jan 23, 2013 at 1:44 PM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: For all of that, I'm not sure that people failing to seek consensus before coding is really so much of a problem as you seem to think. For my part, I don't think the lack of consensus-finding before submitting patches is, in itself, a problem. The problem, imv, is that everyone is expecting that once they've written a patch and put it on a commitfest that it's going to get committed- and it seems like committers are feeling under pressure that, because something's on the CF app, it needs to be committed in some form. FWIW, I have NO delusions that something I propose or submit or put in a CF is necessarily going to get committed. For me it's not committed until I can see it in 'git log' and even then, I've seen stuff get reverted. I would hope that if a committer isn't comfortable with a patch they would explain why, and decline to commit. Then it's up to the submitter as to whether or not they want to make changes, try to explain why they are right and the committer is wrong, or withdraw the patch. There's a lot of good stuff out there, sure, and even more good *ideas*, but it's important to make sure we can provide a stable system with regular releases. As discussed, we really need to be ready to truely triage the remaining patch set, figure out who is going to work on what, and punt the rest til post-9.3. Thanks, Stephen -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
On Wed, Jan 23, 2013 at 6:07 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: On Wed, Jan 23, 2013 at 1:58 PM, Bruce Momjian br...@momjian.us wrote: On Wed, Jan 23, 2013 at 12:27:45PM -0500, Tom Lane wrote: +1 for default timeout --- if this isn't like ping where you are expecting to run indefinitely, I can't see that it's a good idea for it to sit very long by default, in any circumstance. FYI, the pg_ctl -w (wait) default is 60 seconds: Great. That is what I came to on my own as well. Figured that might be a sticking point, but as there is precedent, I'm happy with it. I'm not sure that's a relevant precedent at all. What that number is is the time that pg_ctl will wait around for the postmaster to start or stop before reporting a problem --- and in either case, a significant delay (multiple seconds) is not surprising, because of crash-recovery work, shutdown checkpointing, etc. For pg_isready, you'd expect to get a response more or less instantly, wouldn't you? Personally, I'd decide that pg_isready is broken if it didn't give me an answer in a couple of seconds, much less a minute. What I had in mind was a default timeout of maybe 3 or 4 seconds... I was thinking that if it was in a script you wouldn't care if it was 60 seconds. If it was at the command line you would ^C it much earlier. I think the default linux TCP connection timeout is around 20 seconds. My feeling is everyone is going to have a differing opinion on this, which is why I was hoping that some good precedent existed. I'm fine with 3 or 4, whatever can be agreed upon. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Teaching pg_receivexlog to follow timeline switches
On Tue, Jan 22, 2013 at 8:33 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: You might not want to keep a copy of the whole data directory around, as you have to in a cascading standby. I can see value in a separate WAL proxy software, especially if it's integrated into a larger backup manager program like barman or wal-e. +1 I somehow forgot about $PGDATA here. Time for a little break I guess :) Another idea is to have a daemon mode pg_receivexlog where not only it can maintain a local archive but also feed it using the replication protocol to standbies, keeping track of their position. I'm not sure if i described it well, but that's essentially what I was asking about. It would have both wal receiving and and wal sending capability. Along with it's own local WAL storage perhaps governed in size by a keep_wal_segments and also a longer term archive that you could have compressed but also pull from with a archive and restore command. And also be able to act as a synchronous replication peer. I think it has already been discussed to have pg_receivexlog do that last one. So yeah, a cascading standby without $PGDATA or hot_standby or large shared_buffers resources. It seems like maybe we could add through subtraction. Add a parameter that disables wal replay? I'm sure there'd be more things it would have to disable, but then it's not two separate binaries. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] My first patch! (to \df output)
On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman postgre...@thewickedtribe.net wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Updated the patch in commitfest with the doc change, and added a comment to explain the whitespace change (it was to clean up the sql indentation). I've also attached the new patch here for reference. Docs looks good. Spaces gone. Still need to replace 'definer' and 'invoker' with %s and add the corresponding gettext_noop() calls I think. - -- Jon T Erdman (aka StuckMojo) PostgreSQL Zealot On 01/20/2013 08:27 PM, Craig Ringer wrote: On 01/19/2013 11:54 PM, Jon Erdman wrote: I did realize that since I moved it to + the doc should change, but I didn't address that. I'll get on it this weekend. Held as waiting on author, then. Please update https://commitfest.postgresql.org/action/patch_view?id=1008 when you post a new revision. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -BEGIN PGP SIGNATURE- Comment: Using GnuPG with undefined - http://www.enigmail.net/ iEYEARECAAYFAlD/cNYACgkQRAk1+p0GhSGwJQCfa+8SbL9cYHZkqfmlRlgqcXf9 qD4AnjSZwSXQmOMK8thSs6CdiDxQkJCJ =H+km -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_ctl idempotent option
On Fri, Jan 18, 2013 at 8:48 PM, Phil Sorber p...@omniti.com wrote: +1 Is there more work being done on this, or is the current patch ready to review? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Teaching pg_receivexlog to follow timeline switches
On Fri, Jan 18, 2013 at 7:55 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 18.01.2013 06:38, Phil Sorber wrote: Is it possible to re-use walreceiver code from the backend? I was thinking that it would actually be very useful to have the whole replication functionality modularized and in a standalone binary that could act as a replication proxy and WAL archiver that could run without all the overhead of an entire PG instance There's much sense in trying to extract that into a stand-along module. src/bin/pg_basebackup/receivelog.c is about 1000 lines of code at the moment, and it looks quite different from the corresponding code in the backend, because it doesn't have all the backend infrastructure available. - Heikki That's fair. What do you think about the idea of a full WAL proxy? Probably not for 9.3 at this point though. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Request for vote to move forward with recovery.conf overhaul
On Fri, Dec 21, 2012 at 2:46 PM, Bruce Momjian br...@momjian.us wrote: There has been discussion in the past of removing or significantly changing the way streaming replication/point-in-time-recovery (PITR) is setup in Postgres. Currently the file recovery.conf is used, but that was designed for PITR and does not serve streaming replication well. This all should have been overhauled when streaming replication was added in 2010 in Postgres 9.0. However, time constraints and concern about backward compatibility has hampered this overhaul. At this point, backward compatibility seems to be hampering our ability to move forward. I would like a vote that supports creation of a new method for setting up streaming replication/point-in-time-recovery, where backward compatibility is considered only where it is minimally invasive. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers +1 I seemed to have lost track of the thread that this spawned out of. Is there a coherent plan for a way forward at this point? Last I recall it was Simon's plan vs Bruce's plan. I also seem to recall there was a patch out there too. I think it's even in the commitfest waiting on author. /me searches Here: https://commitfest.postgresql.org/action/patch_view?id=1026 Perhaps we can get the two plans enumerated, vote, and then get a patch out? I'd really like to see this in 9.3, but not sure if that ship has sailed for this feature or not. Thanks. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] pg_isready (was: [WIP] pg_ping utility)
Changing up the subject line because this is no longer a work in progress nor is it pg_ping anymore. On Sun, Jan 20, 2013 at 10:36 PM, Craig Ringer cr...@2ndquadrant.com wrote: On 01/21/2013 11:26 AM, Robert Haas wrote: On Sun, Jan 20, 2013 at 2:59 PM, Phil Sorber p...@omniti.com wrote: Ok. I can add something to the notes section of the docs. I can also add some code comments for this and for grabbing the default params. Sounds good. I added the code comments, but realized it was already in the docs. I will provide a separate patch for the PQping docs. I also added the handling of extra params as Robert suggested. Oh, I see. Is it really important to have the host and port in the output, or should we trim that down to just e.g. accepting connections? It seems useful to have that if a human is looking at the output, but maybe not if a machine is looking at the output. And if somebody doesn't want it, getting rid of it with sed or awk is nontrivial - imagine: pg_isready -d /tmp:5432 - accepting connections If you are scripting I'd assume you would use the return code value. It might be reasonable to make adding the host and port the verbose method and have just accepting connections as the default output, but my concern there is a misdiagnosis because someone doesn't realize what server they are connecting to. This way they can't miss it and they don't have to add another command line option to get that output. It's a fair concern. Does anyone else have an opinion on this? I have a strong view that the host and port *should* be printed in output. One of the most common issues on Stack Overflow questions from new users is with I can't connect problems that turn out to be them connecting to the wrong host, port, or socket path. It is absolutely vital that the unix socket path being used be printed if unix socket connections are tested, as Mac OS X's insane setup means that PostgreSQL tool builds on that platform regularly use the system libpq not the user-installed PostgreSQL's libpq, and it defaults to a different socket path. If users use pg_ping to verify that their PostgreSQL instance is running it'll use the user-installed libpq - fine, that's what we want. But the user will then wonder why the heck Ruby on Rails's `pg' gem reports it can't connect to the unix socket. They can't compare the unix socket paths printed in the error message if pg_ping doesn't show it. I would like to see pg_ping produce a similar error to psql on connection failure. As stated above this is no longer called pg_ping. Probably should have switched the subject line a while ago. I've left it outputting the host and port as default. Also, we went over a lot of iterations on how the errors should look. I am hesitant to change that now. I think the current errors are good because not being able to connect isn't necessarily an unexpected outcome like it is for psql. $ psql -p psql: could not connect to server: No such file or directory Is the server running locally and accepting connections on Unix domain socket /tmp/.s.PGSQL.? $ psql -h localhost -p psql: could not connect to server: Connection refused Is the server running on host localhost (::1) and accepting TCP/IP connections on port ? could not connect to server: Connection refused Is the server running on host localhost (127.0.0.1) and accepting TCP/IP connections on port ? -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services pg_isready_bin_v11.diff Description: Binary data pg_isready_docs_v11.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] PQping Docs
Attached is a patch that adds a note about the FATAL messages that appear in the logs if you don't pass a valid user or dbname to PQping or PQpingParams. This was requested in the pg_isready thread. libpq_pqping_doc.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CF3+4 (was Re: Parallel query execution)
On Wed, Jan 16, 2013 at 8:18 AM, Abhijit Menon-Sen a...@2ndquadrant.com wrote: Here's a breakdown based purely on the names from the CF page (i.e. I didn't check archives to see who actually posted reviews, and didn't take into account reviews posted without updating the CF page). FWIW, I reviewed at least one at the point you did this survey, and I did update the CF page, but I didn't put my name into that box marked reviewers because it is an extra step (Edit Patch) that isn't immediatly obvious. Isn't there a way to automatically populate that based on people linking in their reviews? I guess it might be difficult when a CF manager comes along to add them and they aren't their reviews. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CF3+4 (was Re: Parallel query execution)
On Mon, Jan 21, 2013 at 8:23 PM, Robert Haas robertmh...@gmail.com wrote: What I don't like is when I (or anyone) posts a patch and somebody says something that boils down to no one wants that. *That* ticks me off. Because you know what? At a minimum, *I* want that. If I didn't, I wouldn't have written a patch. And usually, the customers I support want that, too. Now, somebody else may not want it, and that is fine. But IMHO, posting a patch should be considered prima facie evidence of non-zero demand for the associated feature. +1 I'd much rather hear what you are trying to accomplish is much better done this other way. rather than, why would you want to do this? or as you said no one wants that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CF3+4 (was Re: Parallel query execution)
On Mon, Jan 21, 2013 at 8:47 PM, Josh Berkus j...@agliodbs.com wrote: My own experience is different from yours, I guess. I actually like it when I post a patch, or suggest a concept, and Tom fires back with a laundry list of reasons it won't work. This can be a problem with new submitters, though. If you're not used to the current community dialog, that email can be taken as your idea is stupid because rather than what it actually means, which is fix these issues and resubmit, please. That's often not clearly communicated, and is important with new submitters. +1 to this as well. I definitely felt that way when submitting my first patch. Robert might even recall a convo at a conference that he and I had about it. If not for that I might have given up long ago. Now I can say I am used to it though, and I appreciate the honest and constructive criticism I receive because over time I have seen that Tom and others are even critical of themselves and it's really for the betterment of the final product. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Jan 20, 2013 at 8:40 AM, Robert Haas robertmh...@gmail.com wrote:
On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber p...@omniti.com wrote:
Updated patch is rebased against current master and copyright year is
updated.
I took a look at this. According to the documentation for
PQpingParams: It accepts connection parameters identical to those of
PQconnectdbParams, described above. It is not, however, necessary to
supply correct user name, password, or database name values to obtain
the server status. The -U option therefore seems quite useless
except as a source of user confusion, and -d is only useful in that
you could instead pass a connections string. That is definitely a
useful thing to be able to do, but calling the option -d for database
might be viewed as confusing. Or, it might be viewed as consistent
with other commands, if you tilt your head just the right way.
I would be tempted to change the syntax synopsis of the command to
pg_isready [OPTIONS]... [CONNSTR] and delete -d and -U from the list
of options, along the way that psql already works, but making
allowance for the fact that database and username are apparently not
relevant.
This was done to silence useless error messages in the logs. If you
attempt to connect as some user that does not exist, or to some
database that does not exist, it throws an error in the logs, even
with PQping. You could fix it with env vars, but since the point is to
change the user/database that we were connecting as, I figured it
should be consistent with all the other methods to do that.
I am also a little bit baffled by the loop that begins here:
+ while (conn_opt_ptr-keyword)
It appears to me that what this loop does is iterate through all of
the possible connection options and then, when we get to the host,
port, user, or dbname options, add them to the keywords and values
arrays. But... what do we get out of iterating through all of the
other options, then? It seems to me that you could dispense with the
loop and just keep the stuff that actually adds the non-default values
to the arrays:
+ if (pghost != NULL)
+ {
+ keywords[opt_index] = host;
+ values[opt_index] = pghost;
+ opt_index++;
+ }
+ if (pgport != NULL)
+ {
+ keywords[opt_index] = port;
+ values[opt_index] = pgport;
+ opt_index++;
+ }
+ if (pgconnstr != NULL)
+ {
+ keywords[opt_index] = dbname;
+ values[opt_index] = pgconnstr;
+ opt_index++;
+ }
Maybe there's some purpose to this I'm not seeing, but from here the
loop looks like an unnecessary frammish.
I use this to find the defaults if they don't pass anything in, so I
know what to put in the status message at the end. I could devise my
own way to come up with those values as I have seen in some other
code, but I thought it was better to ask libpq directly what defaults
it was going to use.
Finally, I think there should be a check that the user hasn't supplied
more command-line arguments than we know what to do with, similar to
this:
[rhaas pgsql]$ vacuumdb foo bar baz
vacuumdb: too many command-line arguments (first is bar)
Try vacuumdb --help for more information.
That error message text seems like it might not have been given
sufficient thought, but for purposes of this patch it's probably
better to copy it than to invent something new.
I had not considered this. I will take a look and provide an updated patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Jan 20, 2013 at 11:38 AM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber p...@omniti.com wrote: This was done to silence useless error messages in the logs. If you attempt to connect as some user that does not exist, or to some database that does not exist, it throws an error in the logs, even with PQping. You could fix it with env vars, but since the point is to change the user/database that we were connecting as, I figured it should be consistent with all the other methods to do that. Uh, OK. Well, in that case, I'm inclined to think that a documentation mention is in order, and perhaps an update to the PQpingParams documentation as well. Because that's hardly obvious. :-( Ok. I can add something to the notes section of the docs. I can also add some code comments for this and for grabbing the default params. I use this to find the defaults if they don't pass anything in, so I know what to put in the status message at the end. I could devise my own way to come up with those values as I have seen in some other code, but I thought it was better to ask libpq directly what defaults it was going to use. Oh, I see. Is it really important to have the host and port in the output, or should we trim that down to just e.g. accepting connections? It seems useful to have that if a human is looking at the output, but maybe not if a machine is looking at the output. And if somebody doesn't want it, getting rid of it with sed or awk is nontrivial - imagine: pg_isready -d /tmp:5432 - accepting connections If you are scripting I'd assume you would use the return code value. It might be reasonable to make adding the host and port the verbose method and have just accepting connections as the default output, but my concern there is a misdiagnosis because someone doesn't realize what server they are connecting to. This way they can't miss it and they don't have to add another command line option to get that output. The other thing I thought about when you mentioned this is not doing the default lookups if it's in quiet mode. I could move things around to accomplish this, but not sure it is worth the effort and complexity. Thoughts? I had not considered this. I will take a look and provide an updated patch. Sounds good. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] My first patch! (to \df output)
On Jan 19, 2013 10:55 AM, Jon Erdman postgre...@thewickedtribe.net wrote: I did realize that since I moved it to + the doc should change, but I didn't address that. I'll get on it this weekend. As far as the column name and displayed values go, they're taken from the CREATE FUNCTION syntax, and were recommended by Magnus, Bruce, and Fetter, who were all sitting next to me day after pgconf.eu Prague. I personally have no strong feelings either way, I just want to be able to see the info without having to directly query pg_proc. Whatever you all agree on is fine by me. Sounds like you have a +4/-1 on the names then. I'd keep them as is.
Re: [HACKERS] [WIP] pg_ping utility
On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber p...@omniti.com wrote: Updated patch attached. Thanks. I am marking this patch as ready for committer. -- Michael Paquier http://michael.otacoo.com Updated patch is rebased against current master and copyright year is updated. pg_isready_bin_v10.diff Description: Binary data pg_isready_docs_v10.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] My first patch! (to \df output)
On Sat, Dec 29, 2012 at 1:56 PM, Stephen Frost sfr...@snowman.net wrote: * Jon Erdman (postgre...@thewickedtribe.net) wrote: Oops! Here it is in the proper diff format. I didn't have my env set up correctly :( No biggie, and to get the bike-shedding started, I don't really like the column name or the values.. :) I feel like something clearer would be Runs_As with caller or owner.. Saying Security makes me think of ACLs more than what user ID the function runs as, to be honest. Looking at the actual patch itself, it looks like you have some unecessary whitespace changes included..? Thanks! Stephen Stephen, I think Jon's column name and values make a lot of sense. That being said, I do agree with your point of making it clearer for the person viewing the output, I just don't know if it would be confusing when they wanted to change it or were trying to understand how it related. Agree on the extra spaces in the docs. Jon, I think you inserted your changes improperly in the docs. The classifications apply to the type, not to security. Also, you need to use the %s place holder and the gettext_noop() call for your values as well as your column name. Compiles and tests ok. Results look as expected. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_ctl idempotent option
On Tue, Jan 15, 2013 at 11:06 AM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: On 1/14/13 10:22 AM, Tom Lane wrote: Also it appears to me that the hunk at lines 812ff is changing the default behavior, which is not what the patch is advertised to do. True, I had forgotten to mention that. Since it's already the behavior for start, another option would be to just make it the default for stop as well and forget about the extra options. I'm not sure whether there is a big use case for getting an error code on stop if the server is already stopped. Actually, I seem to recall having had to hack Red Hat's initscript because the LSB standard requires that stopping a not-running server *not* be an error. So +1 for forgetting about the option entirely and just making it idempotent all the time. +1 regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Teaching pg_receivexlog to follow timeline switches
On Tue, Jan 15, 2013 at 9:05 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Now that a standby server can follow timeline switches through streaming replication, we should do teach pg_receivexlog to do the same. Patch attached. Is it possible to re-use walreceiver code from the backend? I was thinking that it would actually be very useful to have the whole replication functionality modularized and in a standalone binary that could act as a replication proxy and WAL archiver that could run without all the overhead of an entire PG instance. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_retainxlog for inclusion in 9.3?
On Tue, Jan 1, 2013 at 10:10 AM, Magnus Hagander mag...@hagander.net wrote: So, it turns out the reason I got no feedback on this tool, was that I forgot both to email about and to actually push the code to github :O So this is actually code that's almost half a year old and that I was supposed to submit for the first or second commitfest. Oops. So, the tool and a README for it right now can be found at https://github.com/mhagander/pg_retainxlog for the time being. The idea behind the tool is to plug a hole in the case when pg_receivexlog is used for log archiving, which is that you still need a blocking archive_command in order to make sure that files aren't recycled on the master. So for 9.2 you can do this with an archive_command that checks if the file has appeared properly on the slave - but that usually means you're back at requiring ssh connectivity between the machines, for example. Even though this information is actually avialable on the master... This can also be of use to pure replication scenarios, where you don't know how to tune wal_keep_segments, but using actual live feedback instead of guessing. When pg_retainxlog is used as an archive_command, it will check the pg_stat_replication view instead of checking the slave. It will then only return ok once the requested logfile has been replicated to the slave. By default it will look for a replication client named pg_receivexlog, but it supports overriding the query with anything - so you can say things like needs to have arrived on at least two replication slaves before we consider it archived. Or if used instead of wal_keep_segmnets, needs to have arrived at *all* replication slaves. Is this a tool that people would like to see included in the general toolchain? If so, I'll reformat it to work in the general build environment and submit it for the last commitfest. (comments on the code itself are of course also welcome) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers +1 to this concept, however it may be implemented. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Dec 23, 2012 at 9:29 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber p...@omniti.com wrote: Added new version with default verbose and quiet option. Also updated docs to reflect changes. Thanks for the updated patches. Here is the status about the binary patch: - Code compiles without any warnings - After testing the patch, it behaves as expected, default option is now verbose, the output can be hidden using -q or --quiet However, I still have the following comments: - in pg_isready.c, the function help needs to be static. I have no objection to making this static, but curious what is your reason for it here? - the list of options called with getopt_long should be classified in alphabetical order (the option q is not at the correct position) d:h:p:U:qV = d:h:p:qU:V Then, about the doc patch: - docs compile correctly (I did a manual check) - I am not a native English speaker, but the docs look correct to me. There are enough examples and description is enough. No problems either with the sgml format. Once the 2 small things I noticed are fixed, this patch can be marked as ready for committer. Ok, I will get an updated version later today. Thanks, -- Michael Paquier http://michael.otacoo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers e...@xs4all.nl wrote: On Sun, December 23, 2012 15:29, Michael Paquier wrote: Once the 2 small things I noticed are fixed, this patch can be marked as ready for committer. I wasn't going to complain about it, but if we're going for small things anyway... The output is now capitalised: /tmp:6543 - Accepting Connections /tmp:6000 - No Response which looks unusual to me, could we please make it all lower-case? I'm not apposed to it in principal. Is that how it is done elsewhere in the code? If there are no objections from anyone else I will roll that change in with the others. TYhanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Dec 23, 2012 at 10:07 AM, Phil Sorber p...@omniti.com wrote: On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers e...@xs4all.nl wrote: On Sun, December 23, 2012 15:29, Michael Paquier wrote: Once the 2 small things I noticed are fixed, this patch can be marked as ready for committer. I wasn't going to complain about it, but if we're going for small things anyway... The output is now capitalised: /tmp:6543 - Accepting Connections /tmp:6000 - No Response which looks unusual to me, could we please make it all lower-case? I'm not apposed to it in principal. Is that how it is done elsewhere in the code? If there are no objections from anyone else I will roll that change in with the others. TYhanks, Erik Rijkers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Updated patch attached. pg_isready_bin_v9.diff Description: Binary data pg_isready_docs_v9.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Wed, Dec 19, 2012 at 8:28 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian br...@momjian.us wrote: On Sat, Dec 8, 2012 at 08:59:00AM -0500, Phil Sorber wrote: On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier michael.paqu...@gmail.com wrote: Bruce mentionned that pg_isready could be used directly by pg_ctl -w. Default as being non-verbose would make sense. What are the other tools you are thinking about? Some utilities in core? I think Bruce meant that PQPing() is used by pg_ctl -w, not that he would use pg_isready. Right. OK cool. If you have some spare room to write a new version with verbose option as default, I'll be pleased to review it and then write it as ready for committer. Added new version with default verbose and quiet option. Also updated docs to reflect changes. -- Michael Paquier http://michael.otacoo.com pg_isready_bin_v8.diff Description: Binary data pg_isready_docs_v8.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber p...@omniti.com wrote: Something I was just thinking about while testing this again. I mentioned the issue before about someone meaning to put -v and putting -V instead and it being a potential source of problems. What about making verbose the default and removing -v and adding -q to make it quiet? This would also match other tools behavior. I want to get this wrapped up and I am fine with it as is, but just wanted to ask what others thought. Bruce mentionned that pg_isready could be used directly by pg_ctl -w. Default as being non-verbose would make sense. What are the other tools you are thinking about? Some utilities in core? I think Bruce meant that PQPing() is used by pg_ctl -w, not that he would use pg_isready. I was just thinking that if someone is going to use it in a script adding -q won't be a big deal. If someone wants to use it by hand adding -v could become cumbersome. Except if you change the default behavior, let's change this patch status as ready to committer. Thanks, -- Michael Paquier http://michael.otacoo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Thu, Dec 6, 2012 at 8:54 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber p...@omniti.com wrote: On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: No, I think it is the reference docs on the returned value that must be fixed. That is, instead of saying that the return value correspond to the enum values, you should be saying that it will return literal0/literal if it's okay, 1 in another case and 2 in yet another case. And then next to the PQping() enum, add a comment that the values must not be messed around with because pg_isready exposes them to users and shell scripts. +1 I'm on board with this. OK. Let's do that and then mark this patch as ready for committer. Thanks, Those changes have been made. -- Michael Paquier http://michael.otacoo.com Something I was just thinking about while testing this again. I mentioned the issue before about someone meaning to put -v and putting -V instead and it being a potential source of problems. What about making verbose the default and removing -v and adding -q to make it quiet? This would also match other tools behavior. I want to get this wrapped up and I am fine with it as is, but just wanted to ask what others thought. Thanks. pg_isready_bin_v7.diff Description: Binary data pg_isready_docs_v7.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber escribió: On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier michael.paqu...@gmail.com wrote: - Same thing with this example: + para +Standard Usage: +screen + prompt$/prompt userinputpg_isready/userinput + prompt$/prompt userinputecho $?/userinput + computeroutput0/computeroutput +/screen + /para For the time being PQPING_OK returns 0 because it is on top of the enum PGPing, but this might change if for a reason or another the order of outputs is changed. So I understand what you mean by the ordering might change, but this is actual output from the shell. I'm not sure how to convey that sentiment properly here and still have a real example. Perhaps just remove the example? No, I think it is the reference docs on the returned value that must be fixed. That is, instead of saying that the return value correspond to the enum values, you should be saying that it will return literal0/literal if it's okay, 1 in another case and 2 in yet another case. And then next to the PQping() enum, add a comment that the values must not be messed around with because pg_isready exposes them to users and shell scripts. +1 I'm on board with this. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber p...@omniti.com wrote: Here is the updated patch. I renamed it, but using v5 to stay consistent. After looking at this patch, I found the following problems: - There are a couple of whitespaces still in the code, particularly at the end of those lines + const char *pguser = NULL; + const char *pgdbname = NULL; Mystery how those got in there. Fixed. - When describing the values that are returned by pg_isready, it is awkward to refer to the returning values as plain integers as those values are part of an enum. You should add references to PQPING_OK, PQPING_REJECT, PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also add to reference links in the docs redirecting to them, with things of the type xref linkend=libpq-pqpingparams-pqping-ok or related. Fixed. I changed the wording a little too. - Same thing with this example: + para +Standard Usage: +screen + prompt$/prompt userinputpg_isready/userinput + prompt$/prompt userinputecho $?/userinput + computeroutput0/computeroutput +/screen + /para For the time being PQPING_OK returns 0 because it is on top of the enum PGPing, but this might change if for a reason or another the order of outputs is changed. So I understand what you mean by the ordering might change, but this is actual output from the shell. I'm not sure how to convey that sentiment properly here and still have a real example. Perhaps just remove the example? Once those things are fixed, I think this will be ready for committer review as everybody here seem to agree with your approach. -- Michael Paquier http://michael.otacoo.com pg_isready_bin_v6.diff Description: Binary data pg_isready_docs_v6.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Tue, Nov 27, 2012 at 9:43 AM, Phil Sorber p...@omniti.com wrote: On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Peter Eisentraut pete...@gmx.net writes: Sure, PQping is useful for this very specific use case of seeing whether the server has finished starting up. If someone came with a plausible Rename the utility to pg_isready? +1, the current version of the patch is already fitted for that and would not need extra options like the number of packages sent. I am 100% fine with this. I can make this change tomorrow. -- Michael Paquier http://michael.otacoo.com Sorry I haven't jumped in on this thread more, I have limited connectivity where I am. Here is the updated patch. I renamed it, but using v5 to stay consistent. pg_isready_bin_v5.diff Description: Binary data pg_isready_docs_v5.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Peter Eisentraut pete...@gmx.net writes: Sure, PQping is useful for this very specific use case of seeing whether the server has finished starting up. If someone came with a plausible Rename the utility to pg_isready? +1, the current version of the patch is already fitted for that and would not need extra options like the number of packages sent. I am 100% fine with this. I can make this change tomorrow. -- Michael Paquier http://michael.otacoo.com Sorry I haven't jumped in on this thread more, I have limited connectivity where I am. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
I am going to be unavailable until Wednesday, so maybe gives us a few more days for feedback. On Fri, Nov 23, 2012 at 9:48 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber p...@omniti.com wrote: On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote: On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier michael.paqu...@gmail.com wrote: 3) Having an output close to what ping actually does would also be nice, the current output like Accepting/Rejecting Connections are not that Could you be more specific? Are you saying you don't want to see accepting/rejecting info output? OK sorry. I meant something like that for an accessible server: $ pg_ping -c 3 -h server.com PING server.com (192.168.1.3) accept from 192.168.1.3: icmp_seq=0 time=0.241 ms accept from 192.168.1.3: icmp_seq=0 time=0.240 ms accept from 192.168.1.3: icmp_seq=0 time=0.242 ms Like that for a rejected connection: reject from 192.168.1.3: icmp_seq=0 time=0.241 ms Like that for a timeout: timeout from 192.168.1.3: icmp_seq=0 Then print 1 line for each ping taken to stdout. How does icmp_seq fit into this? Or was that an oversight? Also, in standard ping if you don't pass -c it will continue to loop until interrupted. Would you suggest that pg_ping mimic that, or that we add an additional flag for that behavior? FWIW, I would use 'watch' with the existing output for cases that I would need something like that. We waited a couple of days for feedback for this feature. So based on all the comments provided by everybody on this thread, perhaps we should move on and implement the options that would make pg_ping a better wrapper for PQPing. Comments? -- Michael Paquier http://michael.otacoo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
Attached is updated patch v4 with the changes Michael pointed out. On Fri, Nov 16, 2012 at 12:28 AM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier michael.paqu...@gmail.com wrote: Hum, it is not really consistent to use a magic number here, particularly in the case where an additional state would be added in the enum PGPing. So why not simply return PQPING_NO_ATTEMPT when there are incorrect options or you show the help and exit? Looks like the best option here. Good point. I will make that change as well. Maybe I missed something here, but I believe it's standard that program --help should result in exit(0), no matter what the program's exitcode conventions are for live-fire exercises. (See handle_help_version_opts() in the bin/scripts/ programs, for example.) Okay to use NO_ATTEMPT for erroneous arguments, though. This seems unfortunate. If someone were to put 'pg_ping -V' instead of 'pg_ping -v' into a monitoring script, however misguided, it would make it appear as though the server were accepting connections even if it were not. Doesn't really seem to follow our goal of least surprise. Since this is the standard I have updated the patch to use this behavior, though I'm not really happy with this. Not sure if I'd rather break convention, or perhaps leave 0 sacred and add 1 to all the other return codes to offset them. Thoughts? regards, tom lane pg_ping_bin_v4.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote: On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier michael.paqu...@gmail.com wrote: 3) Having an output close to what ping actually does would also be nice, the current output like Accepting/Rejecting Connections are not that Could you be more specific? Are you saying you don't want to see accepting/rejecting info output? OK sorry. I meant something like that for an accessible server: $ pg_ping -c 3 -h server.com PING server.com (192.168.1.3) accept from 192.168.1.3: icmp_seq=0 time=0.241 ms accept from 192.168.1.3: icmp_seq=0 time=0.240 ms accept from 192.168.1.3: icmp_seq=0 time=0.242 ms Like that for a rejected connection: reject from 192.168.1.3: icmp_seq=0 time=0.241 ms Like that for a timeout: timeout from 192.168.1.3: icmp_seq=0 Then print 1 line for each ping taken to stdout. How does icmp_seq fit into this? Or was that an oversight? Also, in standard ping if you don't pass -c it will continue to loop until interrupted. Would you suggest that pg_ping mimic that, or that we add an additional flag for that behavior? FWIW, I would use 'watch' with the existing output for cases that I would need something like that. -- Michael Paquier http://michael.otacoo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
Thanks for the review. On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier michael.paqu...@gmail.com wrote: Hi Phil, I am currently looking at your patch. A lot of people already had a look at at, but I hope I will be helpful in finalizing it and hand it over to a committer. Strangely I got the following error when using git apply: $ git apply ~/download/pg_ping_v3.patch error: src/bin/scripts/.gitignore: already exists in working directory error: src/bin/scripts/Makefile: already exists in working directory I don't really get from where this error comes from, but using patch -p1 made the trick. That is strange. I will take a look to make sure I didn't do something silly. So, regarding the review of the patch (v3): 1) pg_ping.c uses 4 spaces instead of 4-space tabs, which is a PostgreSQL convention. You need to normalize your code respecting that.Take care of not changing the help output which needs 4 spaces at some places though. Ah yes, good catch. Will fix. 2) As mentionned by Christopher and Peter, pg_ping should perhaps not be seen as a simple wrapper calling only once PQPing, but as something that really enhance the libpq calls by incorporating options that could wrap it more efficiently and give the users a tool to customize the ping to a server easily. Hence, the following options make sense: - c for the number of ping packets - i for the interval between pings - W for the time to wait for a response - D output a timestamp at the beginning of ping line - q quiet the output Please also not that at the current state, we could do the same thing with a simple psql -c 'SELECT 1' postgres, so those additional things look really necessary. Ok, so now 3 votes for this. I guess this is a desired feature. I'm still not clear on the use case for this. I could see something like wanting to run the command repeatedly so you could see when a server was out of recovery and accepting connections, but couldn't that be achieved with watch? I'm also not clear what the return code would be if it has mixed results. We could return the last result, or perhaps a new return code for the mixed case. Since more people want it, I will make a version with this and see how others feel. 3) Having an output close to what ping actually does would also be nice, the current output like Accepting/Rejecting Connections are not that Could you be more specific? Are you saying you don't want to see accepting/rejecting info output? 4) I have also some security concerns about pg_ping. I noticed that even a user who is rejected by pg_hba.conf can actually ping the server using pg_ping or PQPing. Is this a wanted behavior? Some input here? This is desired and important behavior. It's not specific to pg_ping, but written into libpq. Also, it's not a special part of the protocol, it just detects how far in the connection process it was able to get to decide if the server is accepting connections. It's not really leaking any extra information that someone couldn't figure out already with the regular connection facilities. This is also why I feel pg_ping is more useful than psql -c 'SELECT 1' postgres. 5) You should not use comments like that: /* Return UNKNOWN*/ Please add a space at the end of comment for clarity like this: /* Return UNKNOWN */ Will fix. 6) Please use exit(1) instead of exit(3) like the other script utilities. We can't actually do this. It is important that it uses exit(3) (UNKNOWN). What this really says to me is the comment from the previous item should be expanded to explain this further. If we exit(1) it will imply some other state (rejecting connections) that is not known for the cases where we exit(3). The return code of pg_ping is an important feature. Or are you suggesting that we merely reorder these so that it aligns with the return code of other utilities and is not aligned with the return value of PQping? Thanks, -- Michael Paquier http://michael.otacoo.com Thanks again for the review. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier michael.paqu...@gmail.com wrote: Hum, it is not really consistent to use a magic number here, particularly in the case where an additional state would be added in the enum PGPing. So why not simply return PQPING_NO_ATTEMPT when there are incorrect options or you show the help and exit? Looks like the best option here. Good point. I will make that change as well. -- Michael Paquier http://michael.otacoo.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal - assign result of query to psql variable
On Tue, Oct 16, 2012 at 12:24 PM, Pavel Stehule pavel.steh...@gmail.com wrote:
2012/10/16 Shigeru HANADA shigeru.han...@gmail.com:
Hi Pavel,
On Tue, Oct 16, 2012 at 6:59 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
here is updated patch, I moved lot of code from lexer to command.com,
and now more \gset doesn't disable other backslash commands on same
line.
* lexer changes
IIUC, new function psql_scan_varlist_varname scans input and returns a
variable name or a comma at each call, and command.c handles the error
such as invalid # of variables. This new design seems better than old one.
However, IMHO the name psql_scan_varlist_varname sounds redundant and
unintuitive. I'd prefer psql_scan_slash_varlist, because it indicates
that that function is expected to be used for arguments of backslash
commands, like psql_scan_slash_command and psql_scan_slash_option.
Thoughts?
* multiple meta command
Now both of the command sequences
$ SELECT 1, 2 \gset var1, var2 \g foo.txt
$ SELECT 1, 2 \g foo.txt \gset var1, var2
set var1 and v2 to 1 and 2 respectively, and also write the result
into foo.txt. This would be what users expected.
* Duplication of variables
I found an issue we have not discussed. Currently \gset accepts same
variable names in the list, and stores last SELECT item in duplicated
variables. For instance,
$ SELECT 1, 2 \gset var, var
stores 2 into var. I think this behavior is acceptable, but it might
be worth mentioning in document.
* extra fixes
I fixed some minor issues below. Please see attached v10 patch for details.
* remove unused macro OT_VARLIST
* remove unnecessary #include directive for common.h
* fill comment within 80 columns
* indent short variable name with tab
* add regression test case for combination of \g and \gset
* bug on FETCH_COUNT = 1
When FETCH_COUNT is set to 1, and the number of rows returned is 1 too,
\gset shows extra (1 row). This would be a bug in
ExecQueryUsingCursor. Please see the last test case in regression test
psql_cmd.
I fixed this bug
Regards
Pavel
I'll mark this patch as waiting author.
Regards,
--
Shigeru HANADA
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
My first review...
Patch applied cleanly to master and make check was fine.
I tested it out and it operates as advertised. There were a couple
things that stood out to me though.
1) NULL values are not displayed properly after \pset null is run.
postgres=# \pset null '(null)'
Null display is (null).
postgres=# select NULL \gset var1
postgres=# \echo :var1
postgres=# select NULL;
?column?
--
(null)
(1 row)
I know this doesn't come back from the server like this, but you
should be able to pull this from the options and display
appropriately. Not sure if it should be in variable display code, or
when you store it into the variable.
2) The error messages seemed kind of terse. Other error messages are
capitalized and have punctuation.
3) We don't find out about incorrect number of columns until after
query returns. I know this is hard/impossible to fix, but it might be
nice to print out the result normally if you can't store it in the
variables.
3b) You throw an error on too many variables, but still store the data
since you have fewer columns than variables. This makes sense, but you
don't inform the user at all.
On to the code:
1) Introduction of random newlines:
*** cleanup:
*** 1254,1259
--- 1383,1389
PQclear(results);
}
+
if (pset.timing)
{
INSTR_TIME_SET_CURRENT(after);
I saw that in a couple places, but that was the most obvious.
2) TargetList - Why not use the built in linked list operations rather
than creating your own? Are they not accessible to client binaries
like this?
Overall I think this is a useful feature and I think you integrated it
well within the existing infrastructure, ie combining concepts of \set
and \g.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut pete...@gmx.net wrote: Quick review ... Code: *** install: all installdirs *** 54,59 --- 55,61 $(INSTALL_PROGRAM) clusterdb$(X) '$(DESTDIR)$(bindir)'/clusterdb$(X) $(INSTALL_PROGRAM) vacuumdb$(X) '$(DESTDIR)$(bindir)'/vacuumdb$(X) $(INSTALL_PROGRAM) reindexdb$(X) '$(DESTDIR)$(bindir)'/reindexdb$(X) + $(INSTALL_PROGRAM) pg_ping$(X) '$(DESTDIR)$(bindir)'/pg_ping$(X) installdirs: $(MKDIR_P) '$(DESTDIR)$(bindir)' Whitespace misaligned Fixed. + exit(3); // Return UNKNOWN No // comments. Changed. + while (NULL != conn_opt_ptr-keyword) Better writte as while (conn_opt_ptr-keyword != NULL) or while (conn_opt_ptr-keyword) Changed to the latter. Also, it seems that about 75% of the patch is connection options processing. How about we get rid of all that and just have them specify a connection string? It would be a break with tradition, but maybe it's time for something new. I don't think that should be a part of this patch. If we think that we should remove connection params and just pass a connection string we should probably deprecate connection params and remove them everywhere together over a period of time like with other features. I don't think we should introduce a new binary that doesn't work the same way as all the other binaries. I went back and checked, and realized I didn't do it correctly, but this patch now does allow a connection string to be passed to -d. This seems to be the accepted way to do this. Functionality: I'm missing the typical ping functionality to ping continuously. If we're going to call it pg_ping, it ought to do something similar to ping, I think. It's not named after the network utility but after the libpq function PQping. Since this doesn't output latencies or really much of anything else like the network ping, I'm not sure it makes sense to do that, but I could be convinced otherwise. Attaching an updated patch. pg_ping_bin_v3.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Tue, Oct 23, 2012 at 6:22 PM, Christopher Browne cbbro...@gmail.com wrote: On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut pete...@gmx.net wrote: On 10/22/12 11:47 AM, Phil Sorber wrote: Also, it seems that about 75% of the patch is connection options processing. How about we get rid of all that and just have them specify a connection string? It would be a break with tradition, but maybe it's time for something new. I'd be pretty pleased if it had just two ways to get configured: a) A connection string (which might, in the new order of things, be a JDBC-like URI), or b) Environment values drawn in from PGHOST/PGPORT/... When I first wrote this for my own purposes it was basically 'return PQping();' with the necessary glue around it and I used the env var's exactly as you describe. I ended up adding connection parameters to satisfy the ops guy who was having trouble integrating it how we wanted to use it. I figured that to go in core it would need that anyway. I'm not sure why we would want to prevent people from using command line options like that. That is often the most intuitive way to use a tool. Either way I think this is probably a separate debate entirely. That's pretty much enough configurability, I'd think. Functionality: I'm missing the typical ping functionality to ping continuously. If we're going to call it pg_ping, it ought to do something similar to ping, I think. Yep, should have equivalents to: -i, an interval between pings, -c, a count -w/-W, a timeout interval Like I replied to Peter above, I'm not sure it fits the mold of the ping network utility. If you think it needs a different name please propose one. I'm not totally closed off to this idea, it's just not what I had in mind when I put it together. If people find it useful, I can add it. Might be nice to have analogues to: -D printing timestamp before each line -q quiets output -v verbose output (got it, check!) -V version (got it, check!) Right now it outputs nothing by default. I suppose I could change it to output Accepting/Rejecting Connections by default, and verbose can add the connection info. Thoughts? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: Here is the new patch. I renamed the utility from pg_ping to pingdb to go along with the naming convention of src/bin/scripts. Uh, no, that's not a step forward. Leaving out a pg prefix from those script names is universally agreed to have been a mistake. We've not felt that changing the legacy names is worth the amount of pain it'd cause, but that doesn't mean that we should propagate the mistake into brand new executable names. regards, tom lane Ok. I asked about this and got no response so I assume there were no strong opinions. I have reverted to the pg_ping name. Patches attached. pg_ping_bin_v2.diff Description: Binary data pg_ping_docs_v2.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah, I know a whole new executable is kind of a pain, and the amount of infrastructure and added maintenance seems a bit high compared to what this does. But a lot of the programs in src/bin/scripts are not much bigger. (In fact that might be the best place for this.) I considered src/bin/scripts but all those are for maintenance tasks on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the bits in common.h/common.c, nor does it need some of the includes that the build process has. Well, we classify all those programs as client-side tools in the documentation, so I don't see that pg_ping doesn't belong there. The alternative is to give it its very own subdirectory under src/bin/; which increases the infrastructure burden *significantly* (eg, now it needs its own NLS message catalog) for not a lot of value IMO. I would also like it to have a regression test which none of those seem to have. [ shrug... ] There is nothing in the current regression infrastructure that would work for this, so that desire is pie-in-the-sky regardless of where you put it in the source tree. Also, PQping itself is exercised in every buildfarm run as part of pg_ctl start, so I don't feel a real strong need to test pg_ping separately. regards, tom lane Here is the new patch. I renamed the utility from pg_ping to pingdb to go along with the naming convention of src/bin/scripts. Updated docs and made some other minor improvements. pingdb-bin.diff Description: Binary data pingdb-doc.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Mon, Oct 15, 2012 at 5:32 PM, Andres Freund and...@2ndquadrant.com wrote: On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote: On 13 October 2012 22:19, Phil Sorber p...@omniti.com wrote: Based on a previous thread (http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I have put together a first attempt of a pg_ping utility. I am attaching two patches. One for the executable and one for the docs. I would also like to make a regression tests and translations, but wanted to get feedback on what I had so far. pg_pong: 1 packets transmitted, 1 received, 0% packet loss, time 2 days Well this works for me, and I raised a couple typos directly to Phil. The advantage of this over pg_ctl status is that it doesn't have to be run on the machine local to the database, and access to the data directory isn't required if it is run locally. The advantage over connecting using a regular connection is that authentication and authorisation isn't necessary, and if all connections are in use, it will still return the desired result. And it does what it says on the tin. So +1 from me. Why not add a pg_ctl subcommand for that? For me that sounds like a good place for it... Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services We discussed that in the other thread. pg_ctl is often only (always?) packaged with the server binaries and not client. Discussed adding it to psql, but Tom said he'd prefer to see it as a standalone binary anyway. I don't have any real strong opinion about it going into an existing binary like psql (I have a patch for this too) or being standalone, I just think we should have some way to do this from the command line on a client. It seems trivial, but I think it's very useful and if our libpq already supports this, why not? FWIW pg_ctl does use the same API (PQping), but it doesn't expose it as an option you can use exclusively. It just uses it to make sure the server is up/down depending on what it is trying to do. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: Why not add a pg_ctl subcommand for that? For me that sounds like a good place for it... I think that's a bad fit, because every other pg_ctl subcommand requires access to the data directory. It would be very confusing if this one subcommand worked remotely when the others didn't. There was also some discussion of wedging it into psql, which would at least have the advantage that it'd typically be installed on the right side of the client/server divide. But I still think wedging into is the appropriate verb there: psql is a tool for making a connection and executing some SQL commands, and ping is not that. Yeah, I know a whole new executable is kind of a pain, and the amount of infrastructure and added maintenance seems a bit high compared to what this does. But a lot of the programs in src/bin/scripts are not much bigger. (In fact that might be the best place for this.) regards, tom lane I considered src/bin/scripts but all those are for maintenance tasks on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the bits in common.h/common.c, nor does it need some of the includes that the build process has. I would also like it to have a regression test which none of those seem to have. Seems like it would be a bit of a wedge there as well, but if we did, maybe we call it pingdb instead? If there is consensus that we want it to live there, I can write a patch for that as well. Though just to be clear my preference at this point is still for a standalone binary. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: I would also like it to have a regression test which none of those seem to have. [ shrug... ] There is nothing in the current regression infrastructure that would work for this, so that desire is pie-in-the-sky regardless of where you put it in the source tree. Also, PQping itself is exercised in every buildfarm run as part of pg_ctl start, so I don't feel a real strong need to test pg_ping separately. My plan was to borrow heavily from the pg_upgrade test. I want to verify the exit status based on known database state as presumably people would be using this for monitoring/load balancing, etc. Hoping to prevent silly breakage like the help output from returning an 'Accepting Connections' exit status. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] getopt() and strdup()
On Wed, Oct 10, 2012 at 7:54 PM, Bruce Momjian br...@momjian.us wrote: On Mon, Oct 8, 2012 at 09:03:37PM -0400, Bruce Momjian wrote: On Mon, Oct 8, 2012 at 04:33:29PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: A while ago I noticed that in some places we strdup/pg_strdup() optarg strings from getopt(), and in some places we don't. If we needed the strdup(), the missing cases should generate errors. If we don't need them, the strdup() is unnecessary, and research confirms they are unnecessary. Should we remove the extra strdup/pg_strdup() calls, for consistency. What research? Given the number of different ways argv[] is handled on different platforms (cf ps_status.c), I am very unwilling to trust that it's safe to hang onto an argv string for long without strdup'ing it. I think we might have had old platforms that required it, but none are still supported today. And what's your grounds for stating that? All the alternatives in ps_status.c are still live code AFAICS. My feeling is it's more likely to be a good idea to be adding strdup's than removing them. Well, what we have now is either wrong or over-kill --- I don't know for sure which. OK, I have developed the attached patch to add strdup/pg_strdup() calls to all saving of getopt optarg arguments. Also, do we want to centralize the definition of pg_strdup() in /port, or leave each module to define it on its own? I see pg_strdup() defined in these modules: /pgtop/contrib/oid2name /pgtop/contrib/pgbench /pgtop/contrib/pg_upgrade /pgtop/src/bin/initdb /pgtop/src/bin/pg_basebackup /pgtop/src/bin/pg_ctl /pgtop/src/bin/pg_dump /pgtop/src/bin/psql /pgtop/src/bin/scripts +1 for a centralized definition. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] getopt() and strdup()
On Sat, Oct 13, 2012 at 3:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Phil Sorber p...@omniti.com writes: On Wed, Oct 10, 2012 at 7:54 PM, Bruce Momjian br...@momjian.us wrote: Also, do we want to centralize the definition of pg_strdup() in /port, or leave each module to define it on its own? +1 for a centralized definition. The difficulty with a centralized definition is that it's not clear that the error path is or should be *exactly* the same for all these usages. I see at least six variants right now. While some are gratuitous, some of them are tied into local conventions of each program. regards, tom lane Is it possible to at least standardize on one for the front-end and one for the back-end? Then we can use those two going forward and sort out all these other usages and get them to conform to one of the aforementioned definitions over time. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [WIP] pg_ping utility
Based on a previous thread (http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I have put together a first attempt of a pg_ping utility. I am attaching two patches. One for the executable and one for the docs. I would also like to make a regression tests and translations, but wanted to get feedback on what I had so far. Thanks. pg_ping_bin.diff Description: Binary data pg_ping_docs.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PQping command line tool
On Wed, Oct 3, 2012 at 11:35 AM, Bruce Momjian br...@momjian.us wrote: On Tue, Oct 2, 2012 at 11:01:36PM -0400, Phil Sorber wrote: I was wondering recently if there was any command line tool that utilized PQping() or PQpingParams(). I searched the code and couldn't find anything and was wondering if there was any interest to have something like this included? I wrote something for my purposes of performing a health check that also supports nagios style status output. It's probably convenient for scripting purposes as well. It's not currently ready for submission to a commitfest, but if there was an interest I would clean it up so that it would be. I don't see any tool using PQping except pg_ctl. Perhaps we should modify pg_ctl status to use PQping. Right now is only checks the postmaster.pid file, and checks to see that the pid is a running postmaster. What it currently doesn't do is to check if the server is accepting connections with PQping(), like we do for pg_ctl -w start. Comments? I was thinking that maybe this should be a new feature in an existing tool, however I don't think pg_ctl would satisfy my use case as it's normally bundled with the server. This would need to be something that I could install just a client package. It's not a deal breaker, but it makes things more complex. How about adding it as an option to psql? That's not to say that I think we shouldn't also add it to 'pg_ctl status'. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PQping command line tool
On Wed, Oct 3, 2012 at 11:42 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/10/3 Phil Sorber p...@omniti.com: On Wed, Oct 3, 2012 at 11:35 AM, Bruce Momjian br...@momjian.us wrote: On Tue, Oct 2, 2012 at 11:01:36PM -0400, Phil Sorber wrote: I was wondering recently if there was any command line tool that utilized PQping() or PQpingParams(). I searched the code and couldn't find anything and was wondering if there was any interest to have something like this included? I wrote something for my purposes of performing a health check that also supports nagios style status output. It's probably convenient for scripting purposes as well. It's not currently ready for submission to a commitfest, but if there was an interest I would clean it up so that it would be. I don't see any tool using PQping except pg_ctl. Perhaps we should modify pg_ctl status to use PQping. Right now is only checks the postmaster.pid file, and checks to see that the pid is a running postmaster. What it currently doesn't do is to check if the server is accepting connections with PQping(), like we do for pg_ctl -w start. Comments? I was thinking that maybe this should be a new feature in an existing tool, however I don't think pg_ctl would satisfy my use case as it's normally bundled with the server. This would need to be something that I could install just a client package. It's not a deal breaker, but it makes things more complex. How about adding it as an option to psql? That's not to say that I think we shouldn't also add it to 'pg_ctl status'. I was looking at the code and originally I was using return code to signify what the status was and some text output when quiet wasn't set, but psql has it's own set of established return codes. How does everyone feel about using different return codes when psql is in the PQping mode? Also was just printing out terse text forms of the enums. OK, NO_RESPONSE, etc. I was thinking they could be used in shell scripts that way, but we could do that with return codes as well. Would people like to see something more human friendly and descriptive? Also -C, --check was available and I went with that. Most of the other stuff I could think of already had the short option taken. +1 Pavel -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Missing OID define
Thom Brown and I were doing some hacking the other day and came across this missing define. We argued over who was going to send the patch in and I lost. So here it is. pg_type_uuid_oid.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xmalloc = pg_malloc
On Tue, Oct 2, 2012 at 12:30 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: pg_calloc(randomly different API for pg_malloc0) Do we need this? I thought about getting rid of it, but there are some dozens of calls scattered across several files, so I wasn't sure it was worth it. Anybody else have an opinion? I think having more than 1 function that does the same thing is generally a bad idea. It sounds like it is going to cause confusion and provide no real benefit. I wonder whether the same set of functions should also be available in the backend with ereport(EC_OUT_OF_MEMORY, ...) behaviour as well. In the backend, you almost always ought to be using palloc instead. The only places where it's really appropriate to be using malloc directly are where you don't want an error thrown for out-of-memory. So I think providing these in the backend would do little except to encourage bad programming. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PQping command line tool
I was wondering recently if there was any command line tool that utilized PQping() or PQpingParams(). I searched the code and couldn't find anything and was wondering if there was any interest to have something like this included? I wrote something for my purposes of performing a health check that also supports nagios style status output. It's probably convenient for scripting purposes as well. It's not currently ready for submission to a commitfest, but if there was an interest I would clean it up so that it would be. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fwd: PATCH: psql boolean display
On Sun, Sep 2, 2012 at 1:13 AM, Pavel Stehule pavel.steh...@gmail.com wrote: -- Forwarded message -- From: Pavel Stehule pavel.steh...@gmail.com Date: 2012/9/1 Subject: PATCH: psql boolean display To: Phil Sorber p...@omniti.com Hello I am looking to your patch: I have only one note. I don't think so using any text for values true and false is good idea. I don't see a sense of any special texts like foo, bar from your example. More strict design is better - I wrote simple modification - it is based on psql psets - and it add new configuration settings boolstyle [char|word]. A advantage of this design is consistency and possible autocomplete support. Regards Pavel postgres= select true, false; bool │ bool ──┼── t│ f (1 row) postgres= \pset boolstyle word Bool output style is word. postgres= select true, false; bool │ bool ──┼─── true │ false (1 row) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers What you are proposing sounds like it would be better suited to a server side GUC. Based on the discussion in the thread that said true/false was the SQL standard and we were doing it incorrectly as t/f but could not change for legacy reasons. We could even change the default but give users a way to revert to the old behavior. Similar to the bytea_output GUC. Maybe add the GUC for 9.3 but not change the default behavior until 10.0. What my patch was intended to do was let the end user set boolean output to any arbitrary values. While foo/bar is pretty useless, it was meant to reinforce that it was capable of any arbitrary value. I can think of a decent list of other output an end user might want, such as: true/false yes/no y/n on/off 1/0 enabled/disabled Plus the different capitalized forms. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: psql boolean display
On Aug 20, 2012 5:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/8/20 Robert Haas robertmh...@gmail.com: On Sun, Aug 19, 2012 at 12:02 PM, Phil Sorber p...@omniti.com wrote: I am providing a patch to allow you to change the output of a boolean value in psql much like you can do with NULL. A client requested this feature and we thought it may appeal to someone else in the community. The patch includes updated docs and a regression test. The code changes themselves are pretty simple and straightforward. Example from the regression test: SELECT true, false; bool | bool --+-- t| f (1 row) \pset booltrue 'foo' \pset boolfalse 'bar' SELECT true, false; bool | bool --+-- foo | bar (1 row) \pset booltrue 't' \pset boolfalse 'f' SELECT true, false; bool | bool --+-- t| f (1 row) As always, comments welcome. Why not just do it in the SQL? SELECT CASE WHEN whatever THEN 'foo' ELSE 'bar' END AS whatever; I understand this motivation - although I was more happy with server side solution. Was a server side implementation submitted before? I can change it, but I did it on the client side like the null display was done. Regards Pavel Stehule -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: psql boolean display
On Aug 20, 2012 5:19 PM, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/8/20 Robert Haas robertmh...@gmail.com: On Sun, Aug 19, 2012 at 12:02 PM, Phil Sorber p...@omniti.com wrote: I am providing a patch to allow you to change the output of a boolean value in psql much like you can do with NULL. A client requested this feature and we thought it may appeal to someone else in the community. The patch includes updated docs and a regression test. The code changes themselves are pretty simple and straightforward. Example from the regression test: SELECT true, false; bool | bool --+-- t| f (1 row) \pset booltrue 'foo' \pset boolfalse 'bar' SELECT true, false; bool | bool --+-- foo | bar (1 row) \pset booltrue 't' \pset boolfalse 'f' SELECT true, false; bool | bool --+-- t| f (1 row) As always, comments welcome. Why not just do it in the SQL? SELECT CASE WHEN whatever THEN 'foo' ELSE 'bar' END AS whatever; I understand this motivation - although I was more happy with server side solution. Was a server side implementation submitted before? I can change it, but I did it on the client side like the null display was done. Or how about both? Regards Pavel Stehule -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: psql boolean display
On Aug 20, 2012 5:56 PM, Thom Brown t...@linux.com wrote: On 20 August 2012 22:31, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:19 PM, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/8/20 Robert Haas robertmh...@gmail.com: On Sun, Aug 19, 2012 at 12:02 PM, Phil Sorber p...@omniti.com wrote: I am providing a patch to allow you to change the output of a boolean value in psql much like you can do with NULL. A client requested this feature and we thought it may appeal to someone else in the community. The patch includes updated docs and a regression test. The code changes themselves are pretty simple and straightforward. Example from the regression test: SELECT true, false; bool | bool --+-- t| f (1 row) \pset booltrue 'foo' \pset boolfalse 'bar' SELECT true, false; bool | bool --+-- foo | bar (1 row) \pset booltrue 't' \pset boolfalse 'f' SELECT true, false; bool | bool --+-- t| f (1 row) As always, comments welcome. Why not just do it in the SQL? SELECT CASE WHEN whatever THEN 'foo' ELSE 'bar' END AS whatever; I understand this motivation - although I was more happy with server side solution. Was a server side implementation submitted before? I can change it, but I did it on the client side like the null display was done. Or how about both? Surely one would break the other? If using both. -- Thom
Re: [HACKERS] PATCH: psql boolean display
On Aug 20, 2012 6:08 PM, Thom Brown t...@linux.com wrote: On 20 August 2012 23:06, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:56 PM, Thom Brown t...@linux.com wrote: On 20 August 2012 22:31, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:19 PM, Phil Sorber p...@omniti.com wrote: On Aug 20, 2012 5:11 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2012/8/20 Robert Haas robertmh...@gmail.com: On Sun, Aug 19, 2012 at 12:02 PM, Phil Sorber p...@omniti.com wrote: I am providing a patch to allow you to change the output of a boolean value in psql much like you can do with NULL. A client requested this feature and we thought it may appeal to someone else in the community. The patch includes updated docs and a regression test. The code changes themselves are pretty simple and straightforward. Example from the regression test: SELECT true, false; bool | bool --+-- t| f (1 row) \pset booltrue 'foo' \pset boolfalse 'bar' SELECT true, false; bool | bool --+-- foo | bar (1 row) \pset booltrue 't' \pset boolfalse 'f' SELECT true, false; bool | bool --+-- t| f (1 row) As always, comments welcome. Why not just do it in the SQL? SELECT CASE WHEN whatever THEN 'foo' ELSE 'bar' END AS whatever; I understand this motivation - although I was more happy with server side solution. Was a server side implementation submitted before? I can change it, but I did it on the client side like the null display was done. Or how about both? Surely one would break the other? If using both. Yes. :) Really server would override client. -- Thom
Re: [HACKERS] PATCH: psql boolean display
On Aug 20, 2012 6:28 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Gurjeet Singh singh.gurj...@gmail.com wrote: On occasions I have wanted psql to emit the full 'True'/'False' words instead of cryptic one-letter t/f, which can get lost on long rows that get wrapped around on screen. Writing long-winded CASE expressions to get the effect is too much for small ad-hoc queries. I thought of inventing a data type whose out-function would emit these strings, and tack a ::mybool to the expression I want modified. But that would break the applications if somebody pasted the same query in an application (JDBC or some such that understands boolean) and expected a boolean data type instead of a text output of an expression. The type itself does output true/false; it's just psql that uses t/f. test=# select 'true'::boolean::text; text -- true (1 row) test=# select 'false'::boolean::text; text --- false (1 row) That has always seemed quite odd (and occasionally inconvenient) to me. I think that may be from the cast. I didn't see any transformation in psql. Looked like it was raw output from the server. -Kevin
