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 claimsIs 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 unset
Try "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 patch tommorow and will post it here.
And if there will be any problem in having the similar check as postgres.exe itself does, 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 claimsIs 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 you know 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.
Attachments:
defect_pg_ctl_admin_win32.patchtext/plain; name=defect_pg_ctl_admin_win32.patchDownload+156-0
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
