Thread: pg_file_settings view vs. Windows
I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings
view doesn't act as its author presumably intended. Specifically, it
reads as empty until/unless the current session processes a SIGHUP event.
This is because before that happens, it's depending on having inherited
the state data from the postmaster via fork(), which of course does not
happen with EXEC_BACKEND. This applies to both the committed version and
my proposed rewrite.
AFAICS the only "correct" fix would be to add the pg_file_settings
state data to the set of data dumped and reloaded through
write_nondefault_variables/read_nondefault_variables. That would add
a fair amount of code, and it might hurt backend startup time more than
the feature is worth. In any case, I'm not volunteering to do it.
More or less bad alternative answers include:
1. Just document the current behavior.
2. On Windows, force a SIGHUP processing cycle if we're asked to execute
the view and we see that no state data exists yet. This would have the
result that the current backend might adopt settings that no other session
has yet, which is not so great but might be tolerable.
3. Force a SIGHUP processing cycle but don't actually apply any of the
values. This would be pretty messy though, especially if you wanted it
to produce results exactly like the normal case so far as detection of
incorrect values goes. I don't think this is a good idea; it's going
to be too prone to corner-case bugs.
4. Revert the pg_file_settings patch altogether until the author comes
up with a more portable implementation.
Thoughts? As I said, I'm not volunteering to fix this.
regards, tom lane
> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings > view doesn't act as its author presumably intended. Specifically, it > reads as empty until/unless the current session processes a SIGHUP event. > This is because before that happens, it's depending on having inherited > the state data from the postmaster via fork(), which of course does not > happen with EXEC_BACKEND. This applies to both the committed version and > my proposed rewrite. > > AFAICS the only "correct" fix would be to add the pg_file_settings > state data to the set of data dumped and reloaded through > write_nondefault_variables/read_nondefault_variables. That would add > a fair amount of code, and it might hurt backend startup time more than > the feature is worth. In any case, I'm not volunteering to do it. > > More or less bad alternative answers include: > > 1. Just document the current behavior. > > 2. On Windows, force a SIGHUP processing cycle if we're asked to execute > the view and we see that no state data exists yet. This would have the > result that the current backend might adopt settings that no other session > has yet, which is not so great but might be tolerable. > > 3. Force a SIGHUP processing cycle but don't actually apply any of the > values. This would be pretty messy though, especially if you wanted it > to produce results exactly like the normal case so far as detection of > incorrect values goes. I don't think this is a good idea; it's going > to be too prone to corner-case bugs. > > 4. Revert the pg_file_settings patch altogether until the author comes > up with a more portable implementation. > > Thoughts? As I said, I'm not volunteering to fix this. I'm just wondering why we did not catch this earlier. If this is because threre's no regression test case for pg_file_settings view, we should add along with any of solutions above (of course except #4) IMO. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Tatsuo Ishii <ishii@postgresql.org> writes:
>> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings
>> view doesn't act as its author presumably intended. Specifically, it
>> reads as empty until/unless the current session processes a SIGHUP event.
> I'm just wondering why we did not catch this earlier. If this is
> because threre's no regression test case for pg_file_settings view,
Yeah, exactly. Unfortunately I see no way to add a useful test, at least
not one that will work in installcheck mode. There's no way to predict
what will be in the view in that case. Even for "make check", the output
would be pretty darn environment-dependent.
regards, tom lane
On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Tatsuo Ishii <ishii@postgresql.org> writes: >>> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings >>> view doesn't act as its author presumably intended. Specifically, it >>> reads as empty until/unless the current session processes a SIGHUP event. > >> I'm just wondering why we did not catch this earlier. If this is >> because threre's no regression test case for pg_file_settings view, > > Yeah, exactly. Unfortunately I see no way to add a useful test, at least > not one that will work in installcheck mode. There's no way to predict > what will be in the view in that case. Even for "make check", the output > would be pretty darn environment-dependent. And also because this patch had no review input regarding Windows and EXEC_BACKEND. I would suggest pinging the author (just did so), waiting for a fix a bit, and move on with 4. if nothing happens. We usually require that a patch includes support for Windows as a requirement (see for example discussions about why pg_fincore in not a contrib module even if it overlaps a bit with pg_prewarm), why would this patch have a different treatment? -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, exactly. Unfortunately I see no way to add a useful test, at least
>> not one that will work in installcheck mode. There's no way to predict
>> what will be in the view in that case. Even for "make check", the output
>> would be pretty darn environment-dependent.
> And also because this patch had no review input regarding Windows and
> EXEC_BACKEND. I would suggest pinging the author (just did so),
> waiting for a fix a bit, and move on with 4. if nothing happens.
Well, mumble. After playing with this for a bit, I'm fairly convinced
that it offers useful functionality, especially with the error-reporting
additions I've proposed. Right now, there is no easy way to tell whether
a SIGHUP has worked, or why not if not, unless you have access to the
postmaster log. So I think there's definite usefulness here for
remote-administration scenarios.
So I kinda think that alternative 1 (document the Windows deficiency)
is better than having no such functionality at all. Obviously a proper
fix would be better yet, but that's something that could be rolled in
later.
> We usually require that a patch includes support for Windows as a
> requirement (see for example discussions about why pg_fincore in not a
> contrib module even if it overlaps a bit with pg_prewarm), why would
> this patch have a different treatment?
Agreed, but it was evidently not obvious to anyone that there was a
portability issue in this code, else we'd have resolved the issue
before it got committed. As a thought experiment, what would happen
if we'd not noticed this issue till post-release, which is certainly
not implausible?
Also, there are multiple pre-existing minor bugs (the leakage problem
I mentioned earlier, and some other things I've found while hacking
on the view patch) that we would have to deal with in some other
way if we revert now. I'd just as soon not detangle that.
regards, tom lane
On Sun, Jun 28, 2015 at 10:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Yeah, exactly. Unfortunately I see no way to add a useful test, at least >>> not one that will work in installcheck mode. There's no way to predict >>> what will be in the view in that case. Even for "make check", the output >>> would be pretty darn environment-dependent. > >> And also because this patch had no review input regarding Windows and >> EXEC_BACKEND. I would suggest pinging the author (just did so), >> waiting for a fix a bit, and move on with 4. if nothing happens. > > Well, mumble. After playing with this for a bit, I'm fairly convinced > that it offers useful functionality, especially with the error-reporting > additions I've proposed. Right now, there is no easy way to tell whether > a SIGHUP has worked, or why not if not, unless you have access to the > postmaster log. So I think there's definite usefulness here for > remote-administration scenarios. > > So I kinda think that alternative 1 (document the Windows deficiency) > is better than having no such functionality at all. Obviously a proper > fix would be better yet, but that's something that could be rolled in > later. > >> We usually require that a patch includes support for Windows as a >> requirement (see for example discussions about why pg_fincore in not a >> contrib module even if it overlaps a bit with pg_prewarm), why would >> this patch have a different treatment? > > Agreed, but it was evidently not obvious to anyone that there was a > portability issue in this code, else we'd have resolved the issue > before it got committed. As a thought experiment, what would happen > if we'd not noticed this issue till post-release, which is certainly > not implausible? > > Also, there are multiple pre-existing minor bugs (the leakage problem > I mentioned earlier, and some other things I've found while hacking > on the view patch) that we would have to deal with in some other > way if we revert now. I'd just as soon not detangle that. > Thank you for bug report. I have not came up with portable idea yet, but I will deal with this problem immediately. If I couldn't come up with better solution, I'd like to propose #1 idea. But it would be unavoidable to be revert it if there are any reason for Windows support. Regards, -- Sawada Masahiko
>> I'm just wondering why we did not catch this earlier. If this is >> because threre's no regression test case for pg_file_settings view, > > Yeah, exactly. Unfortunately I see no way to add a useful test, at least > not one that will work in installcheck mode. There's no way to predict > what will be in the view in that case. Even for "make check", the output > would be pretty darn environment-dependent. Is there anything like this (9.5 features not tested on Windows) other than pg_file_settings? Maybe SRA OSS could help in testing such features on Windows. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
I wrote:
> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings
> view doesn't act as its author presumably intended. Specifically, it
> reads as empty until/unless the current session processes a SIGHUP event.
> ...
> More or less bad alternative answers include:
> ...
> 3. Force a SIGHUP processing cycle but don't actually apply any of the
> values. This would be pretty messy though, especially if you wanted it
> to produce results exactly like the normal case so far as detection of
> incorrect values goes. I don't think this is a good idea; it's going
> to be too prone to corner-case bugs.
I had second thoughts about how difficult this might be. I had forgotten
that set_config_option already has a changeVal argument that does more or
less exactly what we need here: if false, it makes all the checks but
doesn't actually apply the value.
So let me make a radical proposal that both gets rid of the portability
problem and, arguably, makes the view more useful than it is today.
I think we should define the view as showing, not what was in the config
file(s) at the last SIGHUP, but what is in the files *now*. That means
you could use it to validate edits before you attempt to apply them,
rather than having to pull the trigger and then ask if it worked. And yet
it would remain just about as useful as it is now for post-mortem checks
when a SIGHUP didn't do what you expected.
This could be implemented by doing essentially a SIGHUP cycle but passing
changeVal = false to set_config_option. Other details would remain mostly
as in my WIP patch of yesterday. The temporary context for doing SIGHUP
work still seems like a good idea, but we could flush it at the end of
the view's execution rather than needing to hang onto it indefinitely.
The main bug risk here is that there might be GUCs whose apply_hook is
making validity checks that should have been in a check_hook. However,
any such coding is wrong already; and the symptom here would only be that
the view might fail to point out values that can't be applied, which would
be annoying but it's hardly catastrophic.
Comments?
regards, tom lane
On Mon, Jun 29, 2015 at 12:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings >> view doesn't act as its author presumably intended. Specifically, it >> reads as empty until/unless the current session processes a SIGHUP event. >> ... >> More or less bad alternative answers include: >> ... >> 3. Force a SIGHUP processing cycle but don't actually apply any of the >> values. This would be pretty messy though, especially if you wanted it >> to produce results exactly like the normal case so far as detection of >> incorrect values goes. I don't think this is a good idea; it's going >> to be too prone to corner-case bugs. > > I had second thoughts about how difficult this might be. I had forgotten > that set_config_option already has a changeVal argument that does more or > less exactly what we need here: if false, it makes all the checks but > doesn't actually apply the value. > > So let me make a radical proposal that both gets rid of the portability > problem and, arguably, makes the view more useful than it is today. > I think we should define the view as showing, not what was in the config > file(s) at the last SIGHUP, but what is in the files *now*. That means > you could use it to validate edits before you attempt to apply them, > rather than having to pull the trigger and then ask if it worked. And yet > it would remain just about as useful as it is now for post-mortem checks > when a SIGHUP didn't do what you expected. > > This could be implemented by doing essentially a SIGHUP cycle but passing > changeVal = false to set_config_option. Other details would remain mostly > as in my WIP patch of yesterday. The temporary context for doing SIGHUP > work still seems like a good idea, but we could flush it at the end of > the view's execution rather than needing to hang onto it indefinitely. > > The main bug risk here is that there might be GUCs whose apply_hook is > making validity checks that should have been in a check_hook. However, > any such coding is wrong already; and the symptom here would only be that > the view might fail to point out values that can't be applied, which would > be annoying but it's hardly catastrophic. > > Comments? > You meant that we parse each GUC parameter in configration file, and then pass changeVal=false to set_config_option whenever pg_file_settings is refered. So this view would be used for checking whether currently configuration file is applied successfully or not, right? The such a feature would be also good, but the main purpose of pg_file_settings was to resolve where each GUC parameter came from, especially in case of using ALTER SYSTEM command. I'm not sure that it would be adequate for our originally purpose. Regards, -- Sawada Masahiko
Sawada Masahiko <sawada.mshk@gmail.com> writes:
> On Mon, Jun 29, 2015 at 12:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So let me make a radical proposal that both gets rid of the portability
>> problem and, arguably, makes the view more useful than it is today.
>> I think we should define the view as showing, not what was in the config
>> file(s) at the last SIGHUP, but what is in the files *now*. That means
>> you could use it to validate edits before you attempt to apply them,
>> rather than having to pull the trigger and then ask if it worked. And yet
>> it would remain just about as useful as it is now for post-mortem checks
>> when a SIGHUP didn't do what you expected.
> You meant that we parse each GUC parameter in configration file, and
> then pass changeVal=false to set_config_option whenever
> pg_file_settings is refered.
> So this view would be used for checking whether currently
> configuration file is applied successfully or not, right?
Well, it would check whether the current contents of the file could be
applied successfully.
> The such a feature would be also good, but the main purpose of
> pg_file_settings was to resolve where each GUC parameter came from,
> especially in case of using ALTER SYSTEM command.
> I'm not sure that it would be adequate for our originally purpose.
I'm not following. Surely pg_settings itself is enough to tell you
where the currently-applied GUC value came from.
regards, tom lane
On Mon, Jun 29, 2015 at 11:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Sawada Masahiko <sawada.mshk@gmail.com> writes: >> On Mon, Jun 29, 2015 at 12:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> So let me make a radical proposal that both gets rid of the portability >>> problem and, arguably, makes the view more useful than it is today. >>> I think we should define the view as showing, not what was in the config >>> file(s) at the last SIGHUP, but what is in the files *now*. That means >>> you could use it to validate edits before you attempt to apply them, >>> rather than having to pull the trigger and then ask if it worked. And yet >>> it would remain just about as useful as it is now for post-mortem checks >>> when a SIGHUP didn't do what you expected. > >> You meant that we parse each GUC parameter in configration file, and >> then pass changeVal=false to set_config_option whenever >> pg_file_settings is refered. >> So this view would be used for checking whether currently >> configuration file is applied successfully or not, right? > > Well, it would check whether the current contents of the file could be > applied successfully. > >> The such a feature would be also good, but the main purpose of >> pg_file_settings was to resolve where each GUC parameter came from, >> especially in case of using ALTER SYSTEM command. >> I'm not sure that it would be adequate for our originally purpose. > > I'm not following. Surely pg_settings itself is enough to tell you > where the currently-applied GUC value came from. > Ah yes, it would be enough to accomplish originally purpose. In order to see current contents of each configuration file, we need read them whenever pg_file_settings is referred, right? Regards, -- Sawada Masahiko
On Sat, Jun 27, 2015 at 07:20:43PM -0400, Tom Lane wrote: > Tatsuo Ishii <ishii@postgresql.org> writes: > >> I noticed that in EXEC_BACKEND builds (ie, Windows) the pg_file_settings > >> view doesn't act as its author presumably intended. Specifically, it > >> reads as empty until/unless the current session processes a SIGHUP event. > > > I'm just wondering why we did not catch this earlier. If this is > > because threre's no regression test case for pg_file_settings view, > > Yeah, exactly. Unfortunately I see no way to add a useful test, at least > not one that will work in installcheck mode. There's no way to predict > what will be in the view in that case. Even for "make check", the output > would be pretty darn environment-dependent. A TAP test case is the way to test this thoroughly. It could feed any number of specific postgresql.conf variations to a postmaster. See src/bin/pg_ctl/t/002_status.pl for a test doing something similar. (Granted, I would not have thought to write such a test for this feature.)