Re: [HACKERS] pg_file_settings view vs. Windows
On Sat, Jun 27, 2015 at 07:20:43PM -0400, Tom Lane wrote: Tatsuo Ishii is...@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.) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
Sawada Masahiko sawada.m...@gmail.com writes: On Mon, Jun 29, 2015 at 12:20 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
On Mon, Jun 29, 2015 at 12:20 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
Michael Paquier michael.paqu...@gmail.com writes: On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
Tatsuo Ishii is...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@sss.pgh.pa.us wrote: Tatsuo Ishii is...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_file_settings view vs. Windows
On Sun, Jun 28, 2015 at 10:40 AM, Tom Lane t...@sss.pgh.pa.us wrote: Michael Paquier michael.paqu...@gmail.com writes: On Sun, Jun 28, 2015 at 8:20 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
