Thread: PQconninfo function for libpq
I'm breaking this out into it's own thread, for my own sanity if nothing else :) And it's an isolated feature after all. I still agree with the previous review at http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net about keeping the data in more than one place. Based on that, I've created a different version of this patch, attached. This way we keep all the data in one struct. At this point, the patch is untested beyond compiling and a trivial psql check, because I ran out of time :) But I figured I'd throw it out there for comments on which version people prefer. (And yes, it's quite possible I've made a big think-mistake in it somewhere, but again, better to get some eyes on it early) My version also contains a fixed version of the docs that should be moved back into Zoltans version if that's the one we end up preferring. Also, a question was buried in the other review which is - are we OK to remove the requiressl parameter. Both these patches do so, because the code becomes much simpler if we can do that. It has been deprecated since 7.2. Is it OK to remove it, or do we need to put back in the more complex code to deal with both? Attached is both Zoltans latest patch (v16) and my slightly different approach. Comments on which approach is best? Test results from somebody who has the time to look at it? :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
Hi, 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: > I'm breaking this out into it's own thread, for my own sanity if > nothing else :) And it's an isolated feature after all. > > I still agree with the previous review at > http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net > about keeping the data in more than one place. OK, it seems I completely missed that comment. (Or forgot about it if I happened to answer it.) > Based on that, I've created a different version of this patch, > attached. This way we keep all the data in one struct. I like this single structure but not the way you handle the options' classes. In your patch, each option belongs to only one class. These classes are: PG_CONNINFO_REPLICATION = "replication" only PG_CONNINFO_PASSWORD = "password" only PG_CONNINFO_NORMAL = everything else How does it help pg_basebackup to filter out e.g. dbname and replication? These are added by the walreceiver module anyway and adding them to the primary_conninfo line should even be discouraged by the documentation. In my view, the classes should be inclusive: PG_CONNINFO_NORMAL = Everything that's usable for a regular client connection. This mean everything, maybe including "password" but excluding "replication". PG_CONNINFO_PASSWORD = "password" only. PG_CONNINFO_REPLICATION = Everything usable for a replication client not added by walreceiver. Maybe including/excluding "password". Maybe there should be two flags for replication usage: PG_CONNINFO_WALRECEIVER = everything except those not added by walreceiver (and maybe "password" too) PG_CONNINFO_REPLICATION = "replication" only And every option can belong to more than one class, just as in my patch. > > At this point, the patch is untested beyond compiling and a trivial > psql check, because I ran out of time :) But I figured I'd throw it > out there for comments on which version people prefer. (And yes, it's > quite possible I've made a big think-mistake in it somewhere, but > again, better to get some eyes on it early) > > My version also contains a fixed version of the docs that should be > moved back into Zoltans version if that's the one we end up > preferring. I also liked your version of the documentation better, I am not too good at writing docs. > Also, a question was buried in the other review which is - are we OK > to remove the requiressl parameter. Both these patches do so, because > the code becomes much simpler if we can do that. It has been > deprecated since 7.2. Is it OK to remove it, or do we need to put back > in the more complex code to deal with both? > > Attached is both Zoltans latest patch (v16) and my slightly different approach. > > Comments on which approach is best? > > Test results from somebody who has the time to look at it? :) > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ Thanks for four work on this. Best regards, Zoltán Böszörményi -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: > Hi, > > 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: > >> I'm breaking this out into it's own thread, for my own sanity if >> nothing else :) And it's an isolated feature after all. >> >> I still agree with the previous review at >> >> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net >> about keeping the data in more than one place. > > > OK, it seems I completely missed that comment. > (Or forgot about it if I happened to answer it.) > > >> Based on that, I've created a different version of this patch, >> attached. This way we keep all the data in one struct. > > > I like this single structure but not the way you handle the > options' classes. In your patch, each option belongs to only > one class. These classes are: > > PG_CONNINFO_REPLICATION = "replication" only > PG_CONNINFO_PASSWORD = "password" only > PG_CONNINFO_NORMAL = everything else > > How does it help pg_basebackup to filter out e.g. dbname and replication? PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or actually, it shouldn't have the replication=1 part, right? So it should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD? > These are added by the walreceiver module anyway and adding them > to the primary_conninfo line should even be discouraged by the > documentation. Hmm. I wasn't actually thinking about the dbname part here, I admit that. > In my view, the classes should be inclusive: > > PG_CONNINFO_NORMAL = Everything that's usable for a regular client > connection. This mean everything, maybe including "password" but > excluding "replication". > > PG_CONNINFO_PASSWORD = "password" only. > > PG_CONNINFO_REPLICATION = Everything usable for a replication client > not added by walreceiver. Maybe including/excluding "password". > > Maybe there should be two flags for replication usage: > > PG_CONNINFO_WALRECEIVER = everything except those not added > by walreceiver (and maybe "password" too) > > PG_CONNINFO_REPLICATION = "replication" only > > And every option can belong to more than one class, just as in my patch. Hmm. I kind of liked having each option in just one class, but I see the problem. Looking at the ones you suggest, all the non-password ones *have* to be without password, otherwise there i no way to get the conninfo without password - which is the whole reason for that parameter to exist. So the PASSWORD one has to be additional - which means that not making the other ones additional makes them inconsistent. But maybe we don't really have a choice there. >> At this point, the patch is untested beyond compiling and a trivial >> psql check, because I ran out of time :) But I figured I'd throw it >> out there for comments on which version people prefer. (And yes, it's >> quite possible I've made a big think-mistake in it somewhere, but >> again, better to get some eyes on it early) >> >> My version also contains a fixed version of the docs that should be >> moved back into Zoltans version if that's the one we end up >> preferring. > > > I also liked your version of the documentation better, > I am not too good at writing docs. np. >> Also, a question was buried in the other review which is - are we OK >> to remove the requiressl parameter. Both these patches do so, because >> the code becomes much simpler if we can do that. It has been >> deprecated since 7.2. Is it OK to remove it, or do we need to put back >> in the more complex code to deal with both? Just going to highlight that we're looking for at least one third party to comment on this :) >> Attached is both Zoltans latest patch (v16) and my slightly different >> approach. >> >> Comments on which approach is best? >> >> Test results from somebody who has the time to look at it? :) Do you happen to have a set of tests you've been running on your patches? Can you try them again this one? --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
2012-11-21 22:15 keltezéssel, Magnus Hagander írta: > On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: >> Hi, >> >> 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: >> >>> I'm breaking this out into it's own thread, for my own sanity if >>> nothing else :) And it's an isolated feature after all. >>> >>> I still agree with the previous review at >>> >>> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net >>> about keeping the data in more than one place. >> >> OK, it seems I completely missed that comment. >> (Or forgot about it if I happened to answer it.) >> >> >>> Based on that, I've created a different version of this patch, >>> attached. This way we keep all the data in one struct. >> >> I like this single structure but not the way you handle the >> options' classes. In your patch, each option belongs to only >> one class. These classes are: >> >> PG_CONNINFO_REPLICATION = "replication" only >> PG_CONNINFO_PASSWORD = "password" only >> PG_CONNINFO_NORMAL = everything else >> >> How does it help pg_basebackup to filter out e.g. dbname and replication? > PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or > actually, it shouldn't have the replication=1 part, right? So it > should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD? > > >> These are added by the walreceiver module anyway and adding them >> to the primary_conninfo line should even be discouraged by the >> documentation. > Hmm. I wasn't actually thinking about the dbname part here, I admit that. And not only the dbname, libpqwalreceiver adds these three: [zozo@localhost libpqwalreceiver]$ grep dbname * libpqwalreceiver.c: "%s dbname=replication replication=true fallback_application_name=walreceiver", I also excluded "application_name" from PG_CONNINFO_REPLICATION by this reasoning: - for async replication or single standby, it doesn't matter, the connection will show up as "walreceiver" - for sync replication, the administrator has to add the node name manually via application_name. > >> In my view, the classes should be inclusive: >> >> PG_CONNINFO_NORMAL = Everything that's usable for a regular client >> connection. This mean everything, maybe including "password" but >> excluding "replication". >> >> PG_CONNINFO_PASSWORD = "password" only. >> >> PG_CONNINFO_REPLICATION = Everything usable for a replication client >> not added by walreceiver. Maybe including/excluding "password". >> >> Maybe there should be two flags for replication usage: >> >> PG_CONNINFO_WALRECEIVER = everything except those not added >> by walreceiver (and maybe "password" too) >> >> PG_CONNINFO_REPLICATION = "replication" only >> >> And every option can belong to more than one class, just as in my patch. > Hmm. I kind of liked having each option in just one class, but I see > the problem. Looking at the ones you suggest, all the non-password > ones *have* to be without password, otherwise there i no way to get > the conninfo without password - which is the whole reason for that > parameter to exist. So the PASSWORD one has to be additional - which > means that not making the other ones additional makes them > inconsistent. But maybe we don't really have a choice there. Yes, the PASSWORD part can be on its own, this is what I meant above but wanted a different opinion about having it completely separate is better or not. But the NORMAL and REPLICATION (or WALRECEIVER) classes need to overlap. >>> At this point, the patch is untested beyond compiling and a trivial >>> psql check, because I ran out of time :) But I figured I'd throw it >>> out there for comments on which version people prefer. (And yes, it's >>> quite possible I've made a big think-mistake in it somewhere, but >>> again, better to get some eyes on it early) >>> >>> My version also contains a fixed version of the docs that should be >>> moved back into Zoltans version if that's the one we end up >>> preferring. >> >> I also liked your version of the documentation better, >> I am not too good at writing docs. > np. > > >>> Also, a question was buried in the other review which is - are we OK >>> to remove the requiressl parameter. Both these patches do so, because >>> the code becomes much simpler if we can do that. It has been >>> deprecated since 7.2. Is it OK to remove it, or do we need to put back >>> in the more complex code to deal with both? > Just going to highlight that we're looking for at least one third > party to comment on this :) Yes, me too. A +1 for removing wouldn't count from me. ;-) > >>> Attached is both Zoltans latest patch (v16) and my slightly different >>> approach. >>> >>> Comments on which approach is best? >>> >>> Test results from somebody who has the time to look at it? :) > Do you happen to have a set of tests you've been running on your > patches? Can you try them again this one? My set of tests are: 1. initdb the master 2. pg_basebackup -R the first standby from the master 3. pg_basebackup -R the second standby from the master 4. pg_basebackup -R the third standby from the first standby and diff -durpN the different data directories while there is no load on either. I will test it today after fixing the classes in your patch. ;-) > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ > > -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta: > 2012-11-21 22:15 keltezéssel, Magnus Hagander írta: >> On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: >>> Hi, >>> >>> 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: >>> >>>> I'm breaking this out into it's own thread, for my own sanity if >>>> nothing else :) And it's an isolated feature after all. >>>> >>>> I still agree with the previous review at >>>> >>>> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net >>>> about keeping the data in more than one place. >>> >>> OK, it seems I completely missed that comment. >>> (Or forgot about it if I happened to answer it.) >>> >>> >>>> Based on that, I've created a different version of this patch, >>>> attached. This way we keep all the data in one struct. >>> >>> I like this single structure but not the way you handle the >>> options' classes. In your patch, each option belongs to only >>> one class. These classes are: >>> >>> PG_CONNINFO_REPLICATION = "replication" only >>> PG_CONNINFO_PASSWORD = "password" only >>> PG_CONNINFO_NORMAL = everything else >>> >>> How does it help pg_basebackup to filter out e.g. dbname and replication? >> PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or >> actually, it shouldn't have the replication=1 part, right? So it >> should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD? >> >> >>> These are added by the walreceiver module anyway and adding them >>> to the primary_conninfo line should even be discouraged by the >>> documentation. >> Hmm. I wasn't actually thinking about the dbname part here, I admit that. > > And not only the dbname, libpqwalreceiver adds these three: > > [zozo@localhost libpqwalreceiver]$ grep dbname * > libpqwalreceiver.c: "%s dbname=replication replication=true > fallback_application_name=walreceiver", > > I also excluded "application_name" from PG_CONNINFO_REPLICATION > by this reasoning: > > - for async replication or single standby, it doesn't matter, > the connection will show up as "walreceiver" > - for sync replication, the administrator has to add the node name > manually via application_name. > >> >>> In my view, the classes should be inclusive: >>> >>> PG_CONNINFO_NORMAL = Everything that's usable for a regular client >>> connection. This mean everything, maybe including "password" but >>> excluding "replication". >>> >>> PG_CONNINFO_PASSWORD = "password" only. >>> >>> PG_CONNINFO_REPLICATION = Everything usable for a replication client >>> not added by walreceiver. Maybe including/excluding "password". >>> >>> Maybe there should be two flags for replication usage: >>> >>> PG_CONNINFO_WALRECEIVER = everything except those not added >>> by walreceiver (and maybe "password" too) >>> >>> PG_CONNINFO_REPLICATION = "replication" only >>> >>> And every option can belong to more than one class, just as in my patch. >> Hmm. I kind of liked having each option in just one class, but I see >> the problem. Looking at the ones you suggest, all the non-password >> ones *have* to be without password, otherwise there i no way to get >> the conninfo without password - which is the whole reason for that >> parameter to exist. So the PASSWORD one has to be additional - which >> means that not making the other ones additional makes them >> inconsistent. But maybe we don't really have a choice there. > > Yes, the PASSWORD part can be on its own, this is what I meant above > but wanted a different opinion about having it completely separate > is better or not. > > But the NORMAL and REPLICATION (or WALRECEIVER) classes > need to overlap. > >>>> At this point, the patch is untested beyond compiling and a trivial >>>> psql check, because I ran out of time :) But I figured I'd throw it >>>> out there for comments on which version people prefer. (And yes, it's >>>> quite possible I've made a big think-mistake in it somewhere, but >>>> again, better to get some eyes on it early) >>>> >>>> My version also contains a fixed version of the docs that should be >>>> moved back into Zoltans version if that's the one we end up >>>> preferring. >>> >>> I also liked your version of the documentation better, >>> I am not too good at writing docs. >> np. >> >> >>>> Also, a question was buried in the other review which is - are we OK >>>> to remove the requiressl parameter. Both these patches do so, because >>>> the code becomes much simpler if we can do that. It has been >>>> deprecated since 7.2. Is it OK to remove it, or do we need to put back >>>> in the more complex code to deal with both? >> Just going to highlight that we're looking for at least one third >> party to comment on this :) > > Yes, me too. A +1 for removing wouldn't count from me. ;-) > >> >>>> Attached is both Zoltans latest patch (v16) and my slightly different >>>> approach. >>>> >>>> Comments on which approach is best? >>>> >>>> Test results from somebody who has the time to look at it? :) >> Do you happen to have a set of tests you've been running on your >> patches? Can you try them again this one? > > My set of tests are: > > 1. initdb the master > 2. pg_basebackup -R the first standby from the master > 3. pg_basebackup -R the second standby from the master > 4. pg_basebackup -R the third standby from the first standby > > and diff -durpN the different data directories while there is no load on either. > > I will test it today after fixing the classes in your patch. ;-) Attached is my v17 patch based on your version with the classes fixed. I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN which contains "dbname" and "fallback_application_name, and PG_CONNINFO_REPLICATION_USER which contains everything else except "replication". Since pg_basebackup sets fallback_application_name and not application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER. The REPLICATION and REPLICATION_HIDDEN options may be united, I don't have a strong opinion on it. I also flipped the "ignoreMissing" argument to conninfo_storeval() from false to true in PQconninfo() since we don't want error reporting there. This patch works with my pg_basebackup patch after fixing the flags value. Best regards, Zoltán Böszörményi -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Attachment
On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote: > 2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta: > >> 2012-11-21 22:15 keltezéssel, Magnus Hagander írta: >>> >>> On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> >>> wrote: >>>> >>>> Hi, >>>> >>>> 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: >>>> >>>>> I'm breaking this out into it's own thread, for my own sanity if >>>>> nothing else :) And it's an isolated feature after all. >>>>> >>>>> I still agree with the previous review at >>>>> >>>>> >>>>> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net >>>>> about keeping the data in more than one place. >>>> >>>> >>>> OK, it seems I completely missed that comment. >>>> (Or forgot about it if I happened to answer it.) >>>> >>>> >>>>> Based on that, I've created a different version of this patch, >>>>> attached. This way we keep all the data in one struct. >>>> >>>> >>>> I like this single structure but not the way you handle the >>>> options' classes. In your patch, each option belongs to only >>>> one class. These classes are: >>>> >>>> PG_CONNINFO_REPLICATION = "replication" only >>>> PG_CONNINFO_PASSWORD = "password" only >>>> PG_CONNINFO_NORMAL = everything else >>>> >>>> How does it help pg_basebackup to filter out e.g. dbname and >>>> replication? >>> >>> PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or >>> actually, it shouldn't have the replication=1 part, right? So it >>> should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD? >>> >>> >>>> These are added by the walreceiver module anyway and adding them >>>> to the primary_conninfo line should even be discouraged by the >>>> documentation. >>> >>> Hmm. I wasn't actually thinking about the dbname part here, I admit that. >> >> >> And not only the dbname, libpqwalreceiver adds these three: >> >> [zozo@localhost libpqwalreceiver]$ grep dbname * >> libpqwalreceiver.c: "%s dbname=replication replication=true >> fallback_application_name=walreceiver", >> >> I also excluded "application_name" from PG_CONNINFO_REPLICATION >> by this reasoning: >> >> - for async replication or single standby, it doesn't matter, >> the connection will show up as "walreceiver" >> - for sync replication, the administrator has to add the node name >> manually via application_name. >> >>> >>>> In my view, the classes should be inclusive: >>>> >>>> PG_CONNINFO_NORMAL = Everything that's usable for a regular client >>>> connection. This mean everything, maybe including "password" but >>>> excluding "replication". >>>> >>>> PG_CONNINFO_PASSWORD = "password" only. >>>> >>>> PG_CONNINFO_REPLICATION = Everything usable for a replication client >>>> not added by walreceiver. Maybe including/excluding "password". >>>> >>>> Maybe there should be two flags for replication usage: >>>> >>>> PG_CONNINFO_WALRECEIVER = everything except those not added >>>> by walreceiver (and maybe "password" too) >>>> >>>> PG_CONNINFO_REPLICATION = "replication" only >>>> >>>> And every option can belong to more than one class, just as in my patch. >>> >>> Hmm. I kind of liked having each option in just one class, but I see >>> the problem. Looking at the ones you suggest, all the non-password >>> ones *have* to be without password, otherwise there i no way to get >>> the conninfo without password - which is the whole reason for that >>> parameter to exist. So the PASSWORD one has to be additional - which >>> means that not making the other ones additional makes them >>> inconsistent. But maybe we don't really have a choice there. >> >> >> Yes, the PASSWORD part can be on its own, this is what I meant above >> but wanted a different opinion about having it completely separate >> is better or not. >> >> But the NORMAL and REPLICATION (or WALRECEIVER) classes >> need to overlap. >> >>>>> At this point, the patch is untested beyond compiling and a trivial >>>>> psql check, because I ran out of time :) But I figured I'd throw it >>>>> out there for comments on which version people prefer. (And yes, it's >>>>> quite possible I've made a big think-mistake in it somewhere, but >>>>> again, better to get some eyes on it early) >>>>> >>>>> My version also contains a fixed version of the docs that should be >>>>> moved back into Zoltans version if that's the one we end up >>>>> preferring. >>>> >>>> >>>> I also liked your version of the documentation better, >>>> I am not too good at writing docs. >>> >>> np. >>> >>> >>>>> Also, a question was buried in the other review which is - are we OK >>>>> to remove the requiressl parameter. Both these patches do so, because >>>>> the code becomes much simpler if we can do that. It has been >>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put back >>>>> in the more complex code to deal with both? >>> >>> Just going to highlight that we're looking for at least one third >>> party to comment on this :) >> >> >> Yes, me too. A +1 for removing wouldn't count from me. ;-) >> >>> >>>>> Attached is both Zoltans latest patch (v16) and my slightly different >>>>> approach. >>>>> >>>>> Comments on which approach is best? >>>>> >>>>> Test results from somebody who has the time to look at it? :) >>> >>> Do you happen to have a set of tests you've been running on your >>> patches? Can you try them again this one? >> >> >> My set of tests are: >> >> 1. initdb the master >> 2. pg_basebackup -R the first standby from the master >> 3. pg_basebackup -R the second standby from the master >> 4. pg_basebackup -R the third standby from the first standby >> >> and diff -durpN the different data directories while there is no load on >> either. >> >> I will test it today after fixing the classes in your patch. ;-) > > > Attached is my v17 patch based on your version with the classes fixed. > > I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN > which contains "dbname" and "fallback_application_name, and > PG_CONNINFO_REPLICATION_USER which contains everything else > except "replication". > > Since pg_basebackup sets fallback_application_name and not > application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER. I'm still not sure I like this. The "since pg_basebackup sets" part really points to the problem - we're designing a public API for just internal options here, aren't we? Is there *any* other application that would ever use this? And if we do it like this, changing how pg_basebackup works will make a change to our public libpq APIs in worst case. fallback_application_name certainly has *nothing* to do with replication, in principle. I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL and PG_CONNINFO_PASSWORD (which still make sense to separate), and then plug in the exclusion of specific parameters in pg_basebackup, in the CreateRecoveryConf() function. pg_basebackup will of course always know what pg_basebackup is doing with these things. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
2012-11-22 12:44 keltezéssel, Magnus Hagander írta: > On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote: >> 2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta: >> >>> 2012-11-21 22:15 keltezéssel, Magnus Hagander írta: >>>> On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb@cybertec.at> >>>> wrote: >>>>> Hi, >>>>> >>>>> 2012-11-21 19:19 keltezéssel, Magnus Hagander írta: >>>>> >>>>>> I'm breaking this out into it's own thread, for my own sanity if >>>>>> nothing else :) And it's an isolated feature after all. >>>>>> >>>>>> I still agree with the previous review at >>>>>> >>>>>> >>>>>> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net >>>>>> about keeping the data in more than one place. >>>>> >>>>> OK, it seems I completely missed that comment. >>>>> (Or forgot about it if I happened to answer it.) >>>>> >>>>> >>>>>> Based on that, I've created a different version of this patch, >>>>>> attached. This way we keep all the data in one struct. >>>>> >>>>> I like this single structure but not the way you handle the >>>>> options' classes. In your patch, each option belongs to only >>>>> one class. These classes are: >>>>> >>>>> PG_CONNINFO_REPLICATION = "replication" only >>>>> PG_CONNINFO_PASSWORD = "password" only >>>>> PG_CONNINFO_NORMAL = everything else >>>>> >>>>> How does it help pg_basebackup to filter out e.g. dbname and >>>>> replication? >>>> PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or >>>> actually, it shouldn't have the replication=1 part, right? So it >>>> should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD? >>>> >>>> >>>>> These are added by the walreceiver module anyway and adding them >>>>> to the primary_conninfo line should even be discouraged by the >>>>> documentation. >>>> Hmm. I wasn't actually thinking about the dbname part here, I admit that. >>> >>> And not only the dbname, libpqwalreceiver adds these three: >>> >>> [zozo@localhost libpqwalreceiver]$ grep dbname * >>> libpqwalreceiver.c: "%s dbname=replication replication=true >>> fallback_application_name=walreceiver", >>> >>> I also excluded "application_name" from PG_CONNINFO_REPLICATION >>> by this reasoning: >>> >>> - for async replication or single standby, it doesn't matter, >>> the connection will show up as "walreceiver" >>> - for sync replication, the administrator has to add the node name >>> manually via application_name. >>> >>>>> In my view, the classes should be inclusive: >>>>> >>>>> PG_CONNINFO_NORMAL = Everything that's usable for a regular client >>>>> connection. This mean everything, maybe including "password" but >>>>> excluding "replication". >>>>> >>>>> PG_CONNINFO_PASSWORD = "password" only. >>>>> >>>>> PG_CONNINFO_REPLICATION = Everything usable for a replication client >>>>> not added by walreceiver. Maybe including/excluding "password". >>>>> >>>>> Maybe there should be two flags for replication usage: >>>>> >>>>> PG_CONNINFO_WALRECEIVER = everything except those not added >>>>> by walreceiver (and maybe "password" too) >>>>> >>>>> PG_CONNINFO_REPLICATION = "replication" only >>>>> >>>>> And every option can belong to more than one class, just as in my patch. >>>> Hmm. I kind of liked having each option in just one class, but I see >>>> the problem. Looking at the ones you suggest, all the non-password >>>> ones *have* to be without password, otherwise there i no way to get >>>> the conninfo without password - which is the whole reason for that >>>> parameter to exist. So the PASSWORD one has to be additional - which >>>> means that not making the other ones additional makes them >>>> inconsistent. But maybe we don't really have a choice there. >>> >>> Yes, the PASSWORD part can be on its own, this is what I meant above >>> but wanted a different opinion about having it completely separate >>> is better or not. >>> >>> But the NORMAL and REPLICATION (or WALRECEIVER) classes >>> need to overlap. >>> >>>>>> At this point, the patch is untested beyond compiling and a trivial >>>>>> psql check, because I ran out of time :) But I figured I'd throw it >>>>>> out there for comments on which version people prefer. (And yes, it's >>>>>> quite possible I've made a big think-mistake in it somewhere, but >>>>>> again, better to get some eyes on it early) >>>>>> >>>>>> My version also contains a fixed version of the docs that should be >>>>>> moved back into Zoltans version if that's the one we end up >>>>>> preferring. >>>>> >>>>> I also liked your version of the documentation better, >>>>> I am not too good at writing docs. >>>> np. >>>> >>>> >>>>>> Also, a question was buried in the other review which is - are we OK >>>>>> to remove the requiressl parameter. Both these patches do so, because >>>>>> the code becomes much simpler if we can do that. It has been >>>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put back >>>>>> in the more complex code to deal with both? >>>> Just going to highlight that we're looking for at least one third >>>> party to comment on this :) >>> >>> Yes, me too. A +1 for removing wouldn't count from me. ;-) >>> >>>>>> Attached is both Zoltans latest patch (v16) and my slightly different >>>>>> approach. >>>>>> >>>>>> Comments on which approach is best? >>>>>> >>>>>> Test results from somebody who has the time to look at it? :) >>>> Do you happen to have a set of tests you've been running on your >>>> patches? Can you try them again this one? >>> >>> My set of tests are: >>> >>> 1. initdb the master >>> 2. pg_basebackup -R the first standby from the master >>> 3. pg_basebackup -R the second standby from the master >>> 4. pg_basebackup -R the third standby from the first standby >>> >>> and diff -durpN the different data directories while there is no load on >>> either. >>> >>> I will test it today after fixing the classes in your patch. ;-) >> >> Attached is my v17 patch based on your version with the classes fixed. >> >> I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN >> which contains "dbname" and "fallback_application_name, and >> PG_CONNINFO_REPLICATION_USER which contains everything else >> except "replication". >> >> Since pg_basebackup sets fallback_application_name and not >> application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER. > I'm still not sure I like this. The "since pg_basebackup sets" part > really points to the problem - we're designing a public API for just > internal options here, aren't we? Is there *any* other application > that would ever use this? I was thinking about this pg_receivexlog application but PQconninfo is not needed there. It would be a nice libpq extension to be able to login using a PQconninfoOption array instead of the keyword/value arrays. But the replication/normal distinction is not needed there either. > And if we do it like this, changing how > pg_basebackup works will make a change to our public libpq APIs in > worst case. > > fallback_application_name certainly has *nothing* to do with > replication, in principle. > > > I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL > and PG_CONNINFO_PASSWORD (which still make sense to separate), and > then plug in the exclusion of specific parameters in pg_basebackup, in > the CreateRecoveryConf() function. pg_basebackup will of course always > know what pg_basebackup is doing with these things. OK, I will post a new pg_basebackup patch with this in mind in its own thread. Anyway, here are two small patches, the first is against your previous patch to fix a typo and and the ignoreMissing argument to conninfo_storeval(). The second one is the product of what caught my attention while I was looking at pg_receivexlog. The current coding may write beyond the end of the allocated arrays and the password may overwrite a previously set keyword/value pair. Best regards, Zoltán Böszörményi -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Attachment
When I was testing pg_resetxlog with relative database path,
The pg_resetxlog is doing the transaction log reset even when the server is
running.
Steps to reproduce:1. ./pg_ctl -D ../../data start2. ./pg_resetxlog -f ../../data -- is not able to detect as server
is already running.
Please find the patch for the same:
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
***************
*** 254,260 **** main(int argc, char *argv[]) */ snprintf(path, MAXPGPATH, "%s/postmaster.pid",
DataDir);
! if ((fd = open(path, O_RDONLY, 0)) < 0) { if (errno != ENOENT) {
--- 254,260 ---- */ snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);
! if ((fd = open("postmaster.pid", O_RDONLY, 0)) < 0) { if (errno != ENOENT)
{
Any suggestions/comments?
Regards,
Hari babu.
On Thu, Nov 22, 2012 at 10:22 PM, Hari Babu <haribabu.kommi@huawei.com> wrote:
>
> When I was testing pg_resetxlog with relative database path,
> The pg_resetxlog is doing the transaction log reset even when the server is
> running.
>
> Steps to reproduce:
> 1. ./pg_ctl -D ../../data start
> 2. ./pg_resetxlog -f ../../data -- is not able to detect as server
> is already running.
>
>
> Please find the patch for the same:
>
> *** a/src/bin/pg_resetxlog/pg_resetxlog.c
> --- b/src/bin/pg_resetxlog/pg_resetxlog.c
> ***************
> *** 254,260 **** main(int argc, char *argv[])
> */
> snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);
>
> ! if ((fd = open(path, O_RDONLY, 0)) < 0)
> {
> if (errno != ENOENT)
> {
> --- 254,260 ----
> */
> snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);
>
> ! if ((fd = open("postmaster.pid", O_RDONLY, 0)) < 0)
> {
> if (errno != ENOENT)
> {
>
> Any suggestions/comments?
Looks good to me.
Regards,
--
Fujii Masao
Hari Babu <haribabu.kommi@huawei.com> writes:
> When I was testing pg_resetxlog with relative database path,
> The pg_resetxlog is doing the transaction log reset even when the server is
> running.
Ooops. Fixed, thanks for the report!
regards, tom lane
On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: > 2012-11-22 12:44 keltezéssel, Magnus Hagander írta: >>>>>>> Also, a question was buried in the other review which is - are we OK >>>>>>> to remove the requiressl parameter. Both these patches do so, because >>>>>>> the code becomes much simpler if we can do that. It has been >>>>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put >>>>>>> back >>>>>>> in the more complex code to deal with both? >>>>> >>>>> Just going to highlight that we're looking for at least one third >>>>> party to comment on this :) >>>> >>>> >>>> Yes, me too. A +1 for removing wouldn't count from me. ;-) +1 > The second one is the product of what caught my attention while > I was looking at pg_receivexlog. The current coding may write > beyond the end of the allocated arrays and the password may > overwrite a previously set keyword/value pair. ISTM that such problem doesn't happen at all because argcount is incremented as follows. if (dbhost) argcount++;if (dbuser) argcount++;if (dbport) argcount++; Regards, -- Fujii Masao
2012-11-23 06:30 keltezéssel, Fujii Masao írta: > On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: >> 2012-11-22 12:44 keltezéssel, Magnus Hagander írta: >>>>>>>> Also, a question was buried in the other review which is - are we OK >>>>>>>> to remove the requiressl parameter. Both these patches do so, because >>>>>>>> the code becomes much simpler if we can do that. It has been >>>>>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put >>>>>>>> back >>>>>>>> in the more complex code to deal with both? >>>>>> Just going to highlight that we're looking for at least one third >>>>>> party to comment on this :) >>>>> >>>>> Yes, me too. A +1 for removing wouldn't count from me. ;-) > +1 Thanks. :-) > >> The second one is the product of what caught my attention while >> I was looking at pg_receivexlog. The current coding may write >> beyond the end of the allocated arrays and the password may >> overwrite a previously set keyword/value pair. > ISTM that such problem doesn't happen at all because argcount is > incremented as follows. > > if (dbhost) > argcount++; > if (dbuser) > argcount++; > if (dbport) > argcount++; Right, forget about the second patch. > > Regards, > -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
On Fri, Nov 23, 2012 at 6:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Thu, Nov 22, 2012 at 10:05 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote: >> 2012-11-22 12:44 keltezéssel, Magnus Hagander írta: >>>>>>>> Also, a question was buried in the other review which is - are we OK >>>>>>>> to remove the requiressl parameter. Both these patches do so, because >>>>>>>> the code becomes much simpler if we can do that. It has been >>>>>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put >>>>>>>> back >>>>>>>> in the more complex code to deal with both? >>>>>> >>>>>> Just going to highlight that we're looking for at least one third >>>>>> party to comment on this :) >>>>> >>>>> >>>>> Yes, me too. A +1 for removing wouldn't count from me. ;-) > > +1 Actually, with the cleaner code that resulted from the rewrite, reintroducing it turned out to be pretty darn simple - if I'm not missing something. PFA a patch that comes *with* requiressl=<n> support, without making the code that much more complex. It also fixes the fact that pg_service.conf was broken by the previous one. It also includes the small fixes from Zoltans latest round (the one that was for libpq, not the one for pg_receivexlog that turned out to be wrong). And a pgindent run :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
On 11/22/12 6:44 AM, Magnus Hagander wrote: > I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL > and PG_CONNINFO_PASSWORD (which still make sense to separate), What is the use case for separating them? > and > then plug in the exclusion of specific parameters in pg_basebackup, in > the CreateRecoveryConf() function. pg_basebackup will of course always > know what pg_basebackup is doing with these things. Agreed on that in principle.
On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 11/22/12 6:44 AM, Magnus Hagander wrote: >> I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL >> and PG_CONNINFO_PASSWORD (which still make sense to separate), > > What is the use case for separating them? Getting a connection string that doesn't contain sensitive information like a password. In order to show it in a dialog box, for example. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On 11/27/12 4:20 PM, Magnus Hagander wrote: > On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On 11/22/12 6:44 AM, Magnus Hagander wrote: >>> I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL >>> and PG_CONNINFO_PASSWORD (which still make sense to separate), >> >> What is the use case for separating them? > > Getting a connection string that doesn't contain sensitive information > like a password. In order to show it in a dialog box, for example. There is already the PQconninfoOption.dispchar field for this purpose.
Peter Eisentraut wrote: > On 11/27/12 4:20 PM, Magnus Hagander wrote: > > On Tue, Nov 27, 2012 at 10:13 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > >> On 11/22/12 6:44 AM, Magnus Hagander wrote: > >>> I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL > >>> and PG_CONNINFO_PASSWORD (which still make sense to separate), > >> > >> What is the use case for separating them? > > > > Getting a connection string that doesn't contain sensitive information > > like a password. In order to show it in a dialog box, for example. > > There is already the PQconninfoOption.dispchar field for this purpose. I had the impression that that field would go away with this patch, but then again it might not be worth the compatibility break. I find the dispchar thingy somewhat unsightly. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Peter Eisentraut wrote:
>> There is already the PQconninfoOption.dispchar field for this purpose.
> I had the impression that that field would go away with this patch, but
> then again it might not be worth the compatibility break. I find the
> dispchar thingy somewhat unsightly.
It is that, without a doubt, but that API has been that way longer than
any of us have been working on the code. I'm not excited about changing
it just to have an allegedly-cleaner API. And we cannot have the field
simply "go away", because it's been exposed to applications for lo these
many years, and surely some of them are using it. So in practice we'd
be maintaining both the old API and the new one.
I think we should leave it as-is until there are more reasons to change
it than seem to be provided in this thread.
regards, tom lane
<p dir="ltr"><br /> On Nov 28, 2012 1:54 AM, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br /> ><br /> > Alvaro Herrera <<a href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>writes:<br /> > > Peter Eisentraut wrote:<br/> > >> There is already the PQconninfoOption.dispchar field for this purpose.<br /> ><br /> > >I had the impression that that field would go away with this patch, but<br /> > > then again it might not be worththe compatibility break. I find the<br /> > > dispchar thingy somewhat unsightly.<br /> ><br /> > It isthat, without a doubt, but that API has been that way longer than<br /> > any of us have been working on the code. I'mnot excited about changing<br /> > it just to have an allegedly-cleaner API. And we cannot have the field<br /> >simply "go away", because it's been exposed to applications for lo these<br /> > many years, and surely some of themare using it. So in practice we'd<br /> > be maintaining both the old API and the new one.<br /> ><br /> >I think we should leave it as-is until there are more reasons to change<br /> > it than seem to be provided in thisthread.<br /><p dir="ltr">I think removing that would be a really bad idea. I'm not sure anybody is actually relyingon it, but it would also change the size of the struct and thus break things for anybody using those functions. <pdir="ltr">If people prefer we remove the password classifier for the new function since it at least partially duplicatesthat field we can certainly do that, but I think leaving it in allows those who write new code to use a slightlyneater api, at pretty much no cost in maintenance for us. <p dir="ltr">/Magnus
On Wed, Nov 28, 2012 at 7:01 AM, Magnus Hagander <magnus@hagander.net> wrote: > > On Nov 28, 2012 1:54 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> >> Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> > Peter Eisentraut wrote: >> >> There is already the PQconninfoOption.dispchar field for this purpose. >> >> > I had the impression that that field would go away with this patch, but >> > then again it might not be worth the compatibility break. I find the >> > dispchar thingy somewhat unsightly. >> >> It is that, without a doubt, but that API has been that way longer than >> any of us have been working on the code. I'm not excited about changing >> it just to have an allegedly-cleaner API. And we cannot have the field >> simply "go away", because it's been exposed to applications for lo these >> many years, and surely some of them are using it. So in practice we'd >> be maintaining both the old API and the new one. >> >> I think we should leave it as-is until there are more reasons to change >> it than seem to be provided in this thread. > > I think removing that would be a really bad idea. I'm not sure anybody is > actually relying on it, but it would also change the size of the struct and > thus break things for anybody using those functions. > > If people prefer we remove the password classifier for the new function > since it at least partially duplicates that field we can certainly do that, > but I think leaving it in allows those who write new code to use a slightly > neater api, at pretty much no cost in maintenance for us. In the interest of moving things along, I'll remove these for now at least, and commit the rest of the patch, so we can keep working on the basebacku part of things. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Fri, Nov 30, 2012 at 1:02 AM, Magnus Hagander <magnus@hagander.net> wrote: > In the interest of moving things along, I'll remove these for now at > least, and commit the rest of the patch, so we can keep working on the > basebacku part of things. I see you committed this - does this possibly provide infrastructure that could be used to lift the restriction imposed by the following commit? commit fe21fcaf8d91a71c15ff25276f9fa81e0cd1dba9 Author: Bruce Momjian <bruce@momjian.us> Date: Wed Aug 15 19:04:52 2012 -0400 In psql, if the is no connection object, e.g. due to a server crash, require all parameters for \c, rather than usingthe defaults, which might be wrong. Because personally, I find the new behavior no end of annoying. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Dec 3, 2012 at 3:20 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Nov 30, 2012 at 1:02 AM, Magnus Hagander <magnus@hagander.net> wrote: >> In the interest of moving things along, I'll remove these for now at >> least, and commit the rest of the patch, so we can keep working on the >> basebacku part of things. > > I see you committed this - does this possibly provide infrastructure > that could be used to lift the restriction imposed by the following > commit? > > commit fe21fcaf8d91a71c15ff25276f9fa81e0cd1dba9 > Author: Bruce Momjian <bruce@momjian.us> > Date: Wed Aug 15 19:04:52 2012 -0400 > > In psql, if the is no connection object, e.g. due to a server crash, > require all parameters for \c, rather than using the defaults, which > might be wrong. > > Because personally, I find the new behavior no end of annoying. It certainly sounds like it could do that. Though it might be useful to actually add a connection funciton to libpq that takes such an array of options directly - AFAICS, right now you'd have to deconstruct the return value into a string, and then pass it into a connection function that immediately parses it right back out as conninfooptions. --Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/