Thread: 9.2RC1 wraps this Thursday ...
... or at least, that's what the schedule says. I don't think we can honestly produce a "release candidate" when there are still open issues listed as blockers at http://wiki.postgresql.org/wiki/PostgreSQL_9.2_Open_Items We need to either get something done about those, conclude that they're not blockers, or postpone RC1. The items currently listed as blockers are: * GiST indexes vs fuzzy comparisons used by geometric types ** Alexander proposed a patch that would support the current behavior, but should we change the behavior instead? I put this in the blocker list because I was hoping to get some conversation going about the whole issue of fuzzy comparisons in the geometric stuff. However, the time for making any basic semantic revisions in 9.2 is long past. We could perhaps look at applying Alexander's more restricted patch, but maybe even that is too destabilizing at this point. I'm inclined to move the whole thing onto the "long term issues" list. Comments? * Should we fix tuple limit handling, or redefine 9.x behavior as correct? ** The consensus seems to be to change the documentation to match the current behavior. At this point this is just a pre-existing documentation bug. Somebody ought to do something about it at some point, but it hardly seems like a release blocker. * keepalives I don't know who put this item in, or what it refers to, since it has no supporting link. Unless somebody steps forward with an explanation of what the blocker issue is here, this entry is going to disappear. * pg_ctl crashes on Win32 when neither PGDATA nor -D specified I'm not sure that this qualifies as a release blocker either --- isn't it a plain-vanilla pre-existing bug? And what does the proposed patch have to do with the stated problem? (Even if you define the problem as "make sure we're restricted" rather than the stated symptom, the patch looks rather fragile and Rube Goldbergian ... isn't there a way to actually test if we're in a restricted process?) * Checkpointer process split broke fsync'ing ** bug is fixed, but now we had better recheck earlier performance claims Is anyone actually going to do any performance testing on this? * View options are problematic for pg_dump I had hoped those who created this problem were going to fix it, but given the lack of response I guess I'll have to. regards, tom lane
Excerpts from Tom Lane's message of mar ago 21 10:47:41 -0400 2012: > * pg_ctl crashes on Win32 when neither PGDATA nor -D specified > > I'm not sure that this qualifies as a release blocker either --- isn't > it a plain-vanilla pre-existing bug? And what does the proposed patch > have to do with the stated problem? (Even if you define the problem > as "make sure we're restricted" rather than the stated symptom, the > patch looks rather fragile and Rube Goldbergian ... isn't there a way > to actually test if we're in a restricted process?) You mean, test if we're in a restricted process, and then refuse to run unless that is so? That would be a simple way out of the problem, but I'm not really sure that it "fixes" the issue because Win32 people normally expects stuff to run by dropping privs internally. Maybe that's something we should leave for later, though, and fix 9.2 by doing what you propose (which is presumably going to be a much simpler patch). Clearly having pg_ctl just crash is not a good situation. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Aug 21, 2012 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > ... or at least, that's what the schedule says. I don't think we can > honestly produce a "release candidate" when there are still open issues > listed as blockers at > http://wiki.postgresql.org/wiki/PostgreSQL_9.2_Open_Items > We need to either get something done about those, conclude that they're > not blockers, or postpone RC1. > > The items currently listed as blockers are: > > * GiST indexes vs fuzzy comparisons used by geometric types > ** Alexander proposed a patch that would support the current behavior, but should we change the behavior instead? > > I put this in the blocker list because I was hoping to get some > conversation going about the whole issue of fuzzy comparisons in the > geometric stuff. However, the time for making any basic semantic > revisions in 9.2 is long past. We could perhaps look at applying > Alexander's more restricted patch, but maybe even that is too > destabilizing at this point. I'm inclined to move the whole thing onto > the "long term issues" list. Comments? Agree. > * Should we fix tuple limit handling, or redefine 9.x behavior as correct? > ** The consensus seems to be to change the documentation to match the current behavior. > > At this point this is just a pre-existing documentation bug. Somebody > ought to do something about it at some point, but it hardly seems like > a release blocker. Agree. > * keepalives > > I don't know who put this item in, or what it refers to, since it has > no supporting link. Unless somebody steps forward with an explanation > of what the blocker issue is here, this entry is going to disappear. I don't know who added this either, but Simon addressed it, so it can be moved to resolved. It referred to some changes to the walsender/walreceiver protocol that were made for 9.2 but still a bit half-baked. > * pg_ctl crashes on Win32 when neither PGDATA nor -D specified > > I'm not sure that this qualifies as a release blocker either --- isn't > it a plain-vanilla pre-existing bug? And what does the proposed patch > have to do with the stated problem? (Even if you define the problem > as "make sure we're restricted" rather than the stated symptom, the > patch looks rather fragile and Rube Goldbergian ... isn't there a way > to actually test if we're in a restricted process?) If this isn't a regression, it's not a release blocker. > * Checkpointer process split broke fsync'ing > ** bug is fixed, but now we had better recheck earlier performance claims > > Is anyone actually going to do any performance testing on this? I am unlikely to have time between now and release. > * View options are problematic for pg_dump > > I had hoped those who created this problem were going to fix it, but > given the lack of response I guess I'll have to. This is my fault, but my hackers inbox got flooded and this got lost in the shuffle. Sorry. I can probably devote some time to it today if you don't want to be bothered with it. Do you have a sense of what the right fix is? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane > * pg_ctl crashes on Win32 when neither PGDATA nor -D specified > I'm not sure that this qualifies as a release blocker either --- isn't > it a plain-vanilla pre-existing bug? And what does the proposed patch > have to do with the stated problem? (Even if you define the problem > as "make sure we're restricted" rather than the stated symptom, the > patch looks rather fragile and Rube Goldbergian ... This is to handle one part of the overall problem. Below is text from previous mail discussion due to which new handling is introduced: " > I note that "postgres -C data_directory" will refuse to run on the > command line because I've got admin privileges in Windows, and that > pg_ctl normally starts postgres.exe using CreateRestrictedProcess. > But it does not do so for the popen call in adjust_data_dir. -- By you if that actually is a third bug, as seems likely, somebody with access to a windows environment will need to deal with it." I have tried to define the handling similar to InitDB where for administrative users, it re-forks itself in a restricted mode as it has to start postgres. > isn't there a way to actually test if we're in a restricted process? Do you mean to say that it should check if pg_ctl runs as an administrative user then do the re-fork in restricted mode. If something else, then could you please give more detail about what is exact expectation to handle the above issue. With Regards, Amit Kapila.
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 21, 2012 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * View options are problematic for pg_dump
>>
>> I had hoped those who created this problem were going to fix it, but
>> given the lack of response I guess I'll have to.
> This is my fault, but my hackers inbox got flooded and this got lost
> in the shuffle. Sorry. I can probably devote some time to it today
> if you don't want to be bothered with it. Do you have a sense of what
> the right fix is?
I can work on it if you're still swamped. I think it is probably
fixable by treating the view options as attached to the _RETURN rule
instead of the base table in pg_dump's objects. (There is an ALTER VIEW
command to set the security option, no?)
regards, tom lane
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Excerpts from Tom Lane's message of mar ago 21 10:47:41 -0400 2012:
>> * pg_ctl crashes on Win32 when neither PGDATA nor -D specified
>>
>> I'm not sure that this qualifies as a release blocker either --- isn't
>> it a plain-vanilla pre-existing bug? And what does the proposed patch
>> have to do with the stated problem? (Even if you define the problem
>> as "make sure we're restricted" rather than the stated symptom, the
>> patch looks rather fragile and Rube Goldbergian ... isn't there a way
>> to actually test if we're in a restricted process?)
> You mean, test if we're in a restricted process, and then refuse to run
> unless that is so? That would be a simple way out of the problem, but
> I'm not really sure that it "fixes" the issue because Win32 people
> normally expects stuff to run by dropping privs internally.
Well, what the proposed patch does is fix the permissions problem by
re-executing pg_ctl in a restricted process. What I was unhappy about
was the mechanism for deciding it needs to do that: I think it should
be something less easily breakable than looking for an environment
variable.
And I still don't see what that has to do with failing if the data
directory isn't specified. Surely that should just lead to
pg_ctl: no database directory specified and environment variable PGDATA unsetTry "pg_ctl --help" for more information.
If that doesn't work on Windows, isn't there something else wrong
altogether?
regards, tom lane
Amit Kapila <amit.kapila@huawei.com> writes:
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane
>> * pg_ctl crashes on Win32 when neither PGDATA nor -D specified
>> I'm not sure that this qualifies as a release blocker either --- isn't
>> it a plain-vanilla pre-existing bug?
> This is to handle one part of the overall problem. Below is text from
> previous mail discussion due to which new handling is introduced:
> "
>> I note that "postgres -C data_directory" will refuse to run on the
>> command line because I've got admin privileges in Windows, and that
>> pg_ctl normally starts postgres.exe using CreateRestrictedProcess.
>> But it does not do so for the popen call in adjust_data_dir.
Ah, okay, so that is a new bug in 9.2. I've adjusted the description
on the open-items page to reflect what still needs to be fixed.
>> isn't there a way to actually test if we're in a restricted process?
> Do you mean to say that it should check if pg_ctl runs as an administrative
> user then do the re-fork in restricted mode.
Something like that. The proposed patch depends on there not being a
conflicting environment variable, which seems rather fragile to me.
Can't we test the same condition that postgres.exe itself would test?
regards, tom lane
From: Tom Lane [tgl@sss.pgh.pa.us] Sent: Tuesday, August 21, 2012 10:31 PM Amit Kapila <amit.kapila@huawei.com> writes: > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane >>> * pg_ctl crashes on Win32 when neither PGDATA nor -D specified >>> I'm not sure that this qualifies as a release blocker either --- isn't >>> it a plain-vanilla pre-existing bug? >> This is to handle one part of the overall problem. Below is text from >> previous mail discussion due to which new handling is introduced: >> " >> I note that "postgres -C data_directory" will refuse to run on the >> command line because I've got admin privileges in Windows, and that >> pg_ctl normally starts postgres.exe using CreateRestrictedProcess. >> But it does not do so for the popen call in adjust_data_dir. >Ah, okay, so that is a new bug in 9.2. I've adjusted the description >on the open-items page to reflect what still needs to be fixed. >>> isn't there a way to actually test if we're in a restricted process? >> Do you mean to say that it should check if pg_ctl runs as an administrative >> user then do the re-fork in restricted mode. >Something like that. The proposed patch depends on there not being a >conflicting environment variable, which seems rather fragile to me. >Can't we test the same condition that postgres.exe itself would test? Yes, it should be possible. I will update the patchtommorow and will post it here. And if there will be any problem in having the similar check as postgres.exe itselfdoes, I shall find an alternative and discuss the same. With Regards, Amit Kapila.
On Tue, Aug 21, 2012 at 12:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Aug 21, 2012 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> * View options are problematic for pg_dump >>> >>> I had hoped those who created this problem were going to fix it, but >>> given the lack of response I guess I'll have to. > >> This is my fault, but my hackers inbox got flooded and this got lost >> in the shuffle. Sorry. I can probably devote some time to it today >> if you don't want to be bothered with it. Do you have a sense of what >> the right fix is? > > I can work on it if you're still swamped. I think it is probably > fixable by treating the view options as attached to the _RETURN rule > instead of the base table in pg_dump's objects. (There is an ALTER VIEW > command to set the security option, no?) Yep, we need to emit: ALTER VIEW whatever SET (security_barrier = true); ...after creating the rule that transforms it into a view. I spent a little time looking at this before lunch and it seems pretty straightforward to exclude the options from the dump of the table: I think we can just have repairViewRuleMultiLoop() to clear ((TableInfo *) table)->reloptions. However, that by itself would result in them not getting dumped anywhere, so then I guess we need to add a reloptions field to RuleInfo. repairViewMultiLoop() can then detach the options from the TableInfo object and attach them to the RuleInfo object. Then we can adjust dumpRule() to print an ALTER VIEW command for any attached reloptions. That seems pretty grotty because it kind of flies in the face of the idea that the table and the rule are separate objects, but I don't have a better idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 21, 2012 at 12:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I can work on it if you're still swamped. I think it is probably
>> fixable by treating the view options as attached to the _RETURN rule
>> instead of the base table in pg_dump's objects. (There is an ALTER VIEW
>> command to set the security option, no?)
> Yep, we need to emit:
> ALTER VIEW whatever SET (security_barrier = true);
> ...after creating the rule that transforms it into a view. I spent a
> little time looking at this before lunch and it seems pretty
> straightforward to exclude the options from the dump of the table: I
> think we can just have repairViewRuleMultiLoop() to clear ((TableInfo
> *) table)->reloptions.
> However, that by itself would result in them not getting dumped
> anywhere, so then I guess we need to add a reloptions field to
> RuleInfo. repairViewMultiLoop() can then detach the options from the
> TableInfo object and attach them to the RuleInfo object. Then we can
> adjust dumpRule() to print an ALTER VIEW command for any attached
> reloptions. That seems pretty grotty because it kind of flies in the
> face of the idea that the table and the rule are separate objects, but
> I don't have a better idea.
Yeah, that sounds about right. You want to do it, or shall I?
regards, tom lane
On Tue, Aug 21, 2012 at 2:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Aug 21, 2012 at 12:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I can work on it if you're still swamped. I think it is probably >>> fixable by treating the view options as attached to the _RETURN rule >>> instead of the base table in pg_dump's objects. (There is an ALTER VIEW >>> command to set the security option, no?) > >> Yep, we need to emit: > >> ALTER VIEW whatever SET (security_barrier = true); > >> ...after creating the rule that transforms it into a view. I spent a >> little time looking at this before lunch and it seems pretty >> straightforward to exclude the options from the dump of the table: I >> think we can just have repairViewRuleMultiLoop() to clear ((TableInfo >> *) table)->reloptions. > >> However, that by itself would result in them not getting dumped >> anywhere, so then I guess we need to add a reloptions field to >> RuleInfo. repairViewMultiLoop() can then detach the options from the >> TableInfo object and attach them to the RuleInfo object. Then we can >> adjust dumpRule() to print an ALTER VIEW command for any attached >> reloptions. That seems pretty grotty because it kind of flies in the >> face of the idea that the table and the rule are separate objects, but >> I don't have a better idea. > > Yeah, that sounds about right. You want to do it, or shall I? If you don't mind dealing with it, that's great. If you'd prefer that I cleaned up my own mess, I'll take care of it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 21, 2012 at 2:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, that sounds about right. You want to do it, or shall I?
> If you don't mind dealing with it, that's great. If you'd prefer that
> I cleaned up my own mess, I'll take care of it.
I can do it. I have nothing on my plate today except "get RC1 ready".
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 21, 2012 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * Checkpointer process split broke fsync'ing
>> ** bug is fixed, but now we had better recheck earlier performance claims
>>
>> Is anyone actually going to do any performance testing on this?
> I am unlikely to have time between now and release.
Me either, and I didn't hear any other volunteers.
Even if testing showed that there was some performance regression,
I doubt that we would either revert the checkpointer process split or
hold up the release to look for another solution. So realistically this
is not a blocker issue. I'll move it to the "not blockers" section.
regards, tom lane
From: Tom Lane [tgl@sss.pgh.pa.us] Sent: Tuesday, August 21, 2012 10:31 PM Amit Kapila <amit.kapila@huawei.com> writes: > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane >> * pg_ctl crashes on Win32 when neither PGDATA nor -D specified >>> isn't there a way to actually test if we're in a restricted process? >> Do you mean to say that it should check if pg_ctl runs as an administrative >> user then do the re-fork in restricted mode. > Something like that. The proposed patch depends on there not being a > conflicting environment variable, which seems rather fragile to me. > Can't we test the same condition that postgres.exe itself would test? To implement the postgre.exe way we have following options: 1. Duplicate the function pgwin32_is_admin and related function to pg_ctl, as currently it is not exposed. 2. Make that visible to pg_ctl, but for that it need to link with postgre lib. 3. Move the functions to some common place may be src/port. 4. any other better way? Curretly I have implemented the patch with Approach-1, but I believe Approach-3 would have been better. However I was not sure which is the best place to move functions, so I have implemented with Approach-1. Please let me know if the attached patch is acceptable. I shall wait today night for your confirmation and shall let youknow before I leave my work place in which case I shall complete tommorow morning but not sure whether that much delay is acceptable. With Regards, Amit Kapila.
Attachment
Amit kapila <amit.kapila@huawei.com> writes:
>> Can't we test the same condition that postgres.exe itself would test?
> To implement the postgre.exe way we have following options:
> 1. Duplicate the function pgwin32_is_admin and related function to pg_ctl, as currently it is not exposed.
> 2. Make that visible to pg_ctl, but for that it need to link with postgre lib.
> 3. Move the functions to some common place may be src/port.
> 4. any other better way?
> Curretly I have implemented the patch with Approach-1, but I believe Approach-3 would have been better.
After poking around a bit I realized that you'd copied the
environment-variable hack from initdb.c, which has got basically the
same problem of needing to drop admin privileges. I think it is just
as ugly and dangerous there as here. So I would be in favor of approach
#3 and merging initdb's copy of the code too. In fact, given that
GetCommandLine() appears to be OS-provided, it seems to me that *all*
of the functionality needed could be wrapped up in a utility subroutine
with the semantics of "re-exec myself in a restricted process if
needed".
On the other hand, that's kind of a big chunk of work to take on at the
last minute for what is admittedly a rather hypothetical risk. Maybe
it'd be best to just duplicate initdb's code into pg_ctl for the moment
and plan on cleaning it up later when there's more time.
However, I really can't take responsibility for any of this since
I don't have a Windows development environment. One of the Windows-
hacking committers needs to pick this issue up. Anyone?
regards, tom lane
I wrote:
> ... I really can't take responsibility for any of this since
> I don't have a Windows development environment. One of the Windows-
> hacking committers needs to pick this issue up. Anyone?
[ crickets ]
I guess everybody who might take an interest in this is out sailing...
After further reflection I've realized that, while this is a new bug in
9.2, it is not really a regression from 9.1. The failure only occurs if
pg_ctl is pointed at a configuration-only directory (one that contains
postgresql.conf but is not the real data directory). But that is a case
that did not work at all in any previous release, so no users will be
relying on it.
Accordingly, I don't think this is a release-blocker, so I'm going to
move it to the non-blocker section of the open-items page.
Anybody who wants to fix it is surely welcome to, but I'm not going
to consider this item as a reason to postpone RC1.
regards, tom lane
On 08/23/2012 12:40 PM, Tom Lane wrote: > I wrote: >> ... I really can't take responsibility for any of this since >> I don't have a Windows development environment. One of the Windows- >> hacking committers needs to pick this issue up. Anyone? > > [ crickets ] > > I guess everybody who might take an interest in this is out sailing... If it's critical I can do some test builds, but I burned my Windows dev env down during a recent upgrade and (thankfully) haven't had a reason to re-create it yet so it'd take me a little while. > Accordingly, I don't think this is a release-blocker, so I'm going to > move it to the non-blocker section of the open-items page. Sounds sensible to me. It won't hurt anyone or damage data, so there's little reason not to fix it in a point release. -- Craig Ringer
From: Tom Lane [mailto:tgl@sss.pgh.pa.us] Sent: Thursday, August 23, 2012 10:10 AM I wrote: >> ... I really can't take responsibility for any of this since >> I don't have a Windows development environment. One of the Windows- >> hacking committers needs to pick this issue up. Anyone? > [ crickets ] > Accordingly, I don't think this is a release-blocker, so I'm going to > move it to the non-blocker section of the open-items page. > Anybody who wants to fix it is surely welcome to, but I'm not going > to consider this item as a reason to postpone RC1. Let me know if anything is expected from my side. With Regards, Amit Kapila.
On 08/23/2012 12:40 AM, Tom Lane wrote: > I wrote: >> ... I really can't take responsibility for any of this since >> I don't have a Windows development environment. One of the Windows- >> hacking committers needs to pick this issue up. Anyone? > [ crickets ] > > I guess everybody who might take an interest in this is out sailing... > > After further reflection I've realized that, while this is a new bug in > 9.2, it is not really a regression from 9.1. The failure only occurs if > pg_ctl is pointed at a configuration-only directory (one that contains > postgresql.conf but is not the real data directory). But that is a case > that did not work at all in any previous release, so no users will be > relying on it. > > Accordingly, I don't think this is a release-blocker, so I'm going to > move it to the non-blocker section of the open-items page. > > Anybody who wants to fix it is surely welcome to, but I'm not going > to consider this item as a reason to postpone RC1. > > I'm not sure what you want done. I can test Amit's patch in a couple of Windows environments (say XP+mingw and W7+MSVC) if that's what you need. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 08/23/2012 12:40 AM, Tom Lane wrote:
>> Anybody who wants to fix it is surely welcome to, but I'm not going
>> to consider this item as a reason to postpone RC1.
> I'm not sure what you want done. I can test Amit's patch in a couple of
> Windows environments (say XP+mingw and W7+MSVC) if that's what you need.
Well, the patch as-is just adds another copy of code that really needs
to be refactored into some new file in src/port/ or some such. That's
not work I care to do while being unable to test the result ...
regards, tom lane
On 08/23/2012 01:58 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 08/23/2012 12:40 AM, Tom Lane wrote: >>> Anybody who wants to fix it is surely welcome to, but I'm not going >>> to consider this item as a reason to postpone RC1. >> I'm not sure what you want done. I can test Amit's patch in a couple of >> Windows environments (say XP+mingw and W7+MSVC) if that's what you need. > Well, the patch as-is just adds another copy of code that really needs > to be refactored into some new file in src/port/ or some such. That's > not work I care to do while being unable to test the result ... > > OK, I'll see if I can carve out a bit of time. cheers andrew
On 08/23/2012 02:44 PM, Andrew Dunstan wrote: > > On 08/23/2012 01:58 PM, Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >>> On 08/23/2012 12:40 AM, Tom Lane wrote: >>>> Anybody who wants to fix it is surely welcome to, but I'm not going >>>> to consider this item as a reason to postpone RC1. >>> I'm not sure what you want done. I can test Amit's patch in a couple of >>> Windows environments (say XP+mingw and W7+MSVC) if that's what you >>> need. >> Well, the patch as-is just adds another copy of code that really needs >> to be refactored into some new file in src/port/ or some such. That's >> not work I care to do while being unable to test the result ... >> >> > > > OK, I'll see if I can carve out a bit of time. > > I have spent a couple of hours on this, and I'm sufficiently nervous about it that I'm not going to do anything in a hurry. I will see what can be done over the weekend, possibly, but no promises. TBH I'd rather stick with the less invasive approach of the original patch at this stage, and see about refactoring for 9.3. cheers andrew
On Fri, Aug 24, 2012 at 1:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote: > > On 08/23/2012 02:44 PM, Andrew Dunstan wrote: >> >> >> On 08/23/2012 01:58 PM, Tom Lane wrote: >>> >>> Andrew Dunstan <andrew@dunslane.net> writes: >>>> >>>> On 08/23/2012 12:40 AM, Tom Lane wrote: >>>>> >>>>> Anybody who wants to fix it is surely welcome to, but I'm not going >>>>> to consider this item as a reason to postpone RC1. >>>> >>>> I'm not sure what you want done. I can test Amit's patch in a couple of >>>> Windows environments (say XP+mingw and W7+MSVC) if that's what you need. >>> >>> Well, the patch as-is just adds another copy of code that really needs >>> to be refactored into some new file in src/port/ or some such. That's >>> not work I care to do while being unable to test the result ... >>> >>> >> >> >> OK, I'll see if I can carve out a bit of time. >> >> > > I have spent a couple of hours on this, and I'm sufficiently nervous about > it that I'm not going to do anything in a hurry. I will see what can be done > over the weekend, possibly, but no promises. > > TBH I'd rather stick with the less invasive approach of the original patch > at this stage, and see about refactoring for 9.3. +1. While I haven't looked at the code specifically, these areas can be quite fragile and very environment-dependent. I'd rather not upset it this close to release - especially not after RC wrap. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Fri, Aug 24, 2012 at 1:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> TBH I'd rather stick with the less invasive approach of the original patch
>> at this stage, and see about refactoring for 9.3.
> +1.
> While I haven't looked at the code specifically, these areas can be
> quite fragile and very environment-dependent. I'd rather not upset it
> this close to release - especially not after RC wrap.
Fair enough. Will one of you deal with the patch as-is, then?
regards, tom lane
On 08/24/2012 10:10 AM, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Fri, Aug 24, 2012 at 1:06 AM, Andrew Dunstan <andrew@dunslane.net> wrote: >>> TBH I'd rather stick with the less invasive approach of the original patch >>> at this stage, and see about refactoring for 9.3. >> +1. >> While I haven't looked at the code specifically, these areas can be >> quite fragile and very environment-dependent. I'd rather not upset it >> this close to release - especially not after RC wrap. > Fair enough. Will one of you deal with the patch as-is, then? > > I had a brief talk with Magnus the other day, and I have just spent more time looking over this. This is a fairly narrow failure case, with a not so narrow proposed solution. Making pg_ctl re-exec itself whenever we see that we're running as an admin user is a very broad brush approach, since the problem is restricted to cases where we have a config-only data directory. I'm particularly concerned about the possible effect that might have on pg_ctl when it's running as a service controller, and I'm not prepared to commit anything like the current patch without a great deal more testing. A temporary bandaid might be to do the detection of admin privileges and go back to doing what we did there before we got adjust_data_dir() for that case. That at least should work no worse than what we have now. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I had a brief talk with Magnus the other day, and I have just spent more
> time looking over this. This is a fairly narrow failure case, with a not
> so narrow proposed solution. Making pg_ctl re-exec itself whenever we
> see that we're running as an admin user is a very broad brush approach,
> since the problem is restricted to cases where we have a config-only
> data directory. I'm particularly concerned about the possible effect
> that might have on pg_ctl when it's running as a service controller, and
> I'm not prepared to commit anything like the current patch without a
> great deal more testing.
Good point.
> A temporary bandaid might be to do the
> detection of admin privileges and go back to doing what we did there
> before we got adjust_data_dir() for that case. That at least should work
> no worse than what we have now.
Unless I'm missing something, pg_ctl basically doesn't work with
config-only directory setups before 9.2: since it has no way to find the
postmaster.pid file, any case that waits for the postmaster to start or
stop will fail in a confusing fashion. So the fact that the case still
doesn't work on Windows doesn't constitute a regression; in fact, it
might be *more* user-friendly this way, since you'll get an error rather
than obscure misbehavior. Rather than applying a hasty band-aid,
I think it's probably better to sit back and think about a solution
for 9.3.
BTW, one idea that occurs to me is to bypass the problem by skipping
the server's no-root-privileges check when the postmaster is given the
-C switch. (This shouldn't pose a security hazard, since reading the
config files is something a root-privileged caller could do anyway.)
I don't immediately see a non-ugly way to do that in the current server
code structure, but maybe somebody else will have an idea.
regards, tom lane
On 08/26/2012 03:15 PM, Tom Lane wrote: > BTW, one idea that occurs to me is to bypass the problem by skipping > the server's no-root-privileges check when the postmaster is given the > -C switch. (This shouldn't pose a security hazard, since reading the > config files is something a root-privileged caller could do anyway.) > I don't immediately see a non-ugly way to do that in the current server > code structure, but maybe somebody else will have an idea. > I had the same idea, and couldn't see a simple way to do it either. The trouble is that the check_root() call comes too early for us to do it cleanly. But I'll have another look. cheers andrew