public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v2] windows: Only consider us to be running as service if stderr is invalid. 4+ messages / 4 participants [nested] [flat]
* [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. @ 2021-03-03 05:24 Andres Freund <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Andres Freund @ 2021-03-03 05:24 UTC (permalink / raw) Previously pgwin32_is_service() would falsely return true when postgres is started from somewhere within a service, but not as a service. That is e.g. always the case with windows docker containers, which some CI services use to run windows tests in. In addition to this change, it likely would be a good idea to have pg_ctl runservice pass down a flag indicating that postgres is running as a service. Author: Andres Freund <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/port/win32security.c | 13 +++++++++++-- src/bin/pg_ctl/pg_ctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/port/win32security.c b/src/port/win32security.c index 4a673fde19a..b57ce61d752 100644 --- a/src/port/win32security.c +++ b/src/port/win32security.c @@ -95,8 +95,9 @@ pgwin32_is_admin(void) * We consider ourselves running as a service if one of the following is * true: * - * 1) We are running as LocalSystem (only used by services) - * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the + * 1) Standard error is not valid (always the case for services) + * 2) We are running as LocalSystem (only used by services) + * 3) Our token contains SECURITY_SERVICE_RID (automatically added to the * process token by the SCM when starting a service) * * The check for LocalSystem is needed, because surprisingly, if a service @@ -121,11 +122,19 @@ pgwin32_is_service(void) PSID ServiceSid; PSID LocalSystemSid; SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY}; + HANDLE stderr_handle; /* Only check the first time */ if (_is_service != -1) return _is_service; + stderr_handle = GetStdHandle(STD_ERROR_HANDLE); + if (stderr_handle != INVALID_HANDLE_VALUE && stderr_handle != NULL) + { + _is_service = 0; + return _is_service; + } + /* First check for LocalSystem */ if (!AllocateAndInitializeSid(&NtAuthority, 1, SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0, diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 7985da0a943..c99e3c507de 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -1737,6 +1737,31 @@ typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, L typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE); typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD); +/* + * Set up STARTUPINFO for the new process to inherit this process' handles. + * + * Process started as services appear to have "empty" handles (GetStdHandle() + * returns NULL) rather than invalid ones. But passing down NULL ourselves + * doesn't work, it's interpreted as STARTUPINFO->hStd* not being set. But we + * can pass down INVALID_HANDLE_VALUE - which makes GetStdHandle() in the new + * process (and its child processes!) return INVALID_HANDLE_VALUE. Which + * achieves the goal of postmaster running in a similar environment as pg_ctl. + */ +static void +InheritStdHandles(STARTUPINFO* si) +{ + si->dwFlags |= STARTF_USESTDHANDLES; + si->hStdInput = GetStdHandle(STD_INPUT_HANDLE); + if (si->hStdInput == NULL) + si->hStdInput = INVALID_HANDLE_VALUE; + si->hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE); + if (si->hStdOutput == NULL) + si->hStdOutput = INVALID_HANDLE_VALUE; + si->hStdError = GetStdHandle(STD_ERROR_HANDLE); + if (si->hStdError == NULL) + si->hStdError = INVALID_HANDLE_VALUE; +} + /* * Create a restricted token, a job object sandbox, and execute the specified * process with it. @@ -1774,6 +1799,14 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser ZeroMemory(&si, sizeof(si)); si.cb = sizeof(si); + /* + * Set stdin/stdout/stderr handles to be inherited in the child + * process. That allows postmaster and the processes it starts to perform + * additional checks to see if running in a service (otherwise they get + * the default console handles - which point to "somewhere"). + */ + InheritStdHandles(&si); + Advapi32Handle = LoadLibrary("ADVAPI32.DLL"); if (Advapi32Handle != NULL) { -- 2.29.2.540.g3cf59784d4 --pm3lfdw7knjptu4k-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Contributing test cases to improve coverage @ 2024-06-12 18:22 Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2024-06-12 18:22 UTC (permalink / raw) To: J F <[email protected]>; +Cc: [email protected] J F <[email protected]> writes: > For postgres, I am looking at adding test cases to test suite in > test/src/regress/. I have gone through (a)-(e), and managed to produced > some test cases. As an example, I claim the test case > ``` > CREATE RECURSIVE VIEW a(b) AS SELECT'' ; > SELECT FROM a WHERE NULL; > ``` > could kill the following mutation at optimizer/plan/setrefs.c, 502:5--502:33 > Original binary operator expression: > ``` > rte->rtekind == RTE_SUBQUERY > ```` > Replacement expression: > ``` > (rte->rtekind) >= RTE_SUBQUERY > ``` I am quite confused about what is the point of this. You have not found any actual bug, nor have you demonstrated that this test case could discover a likely future bug that wouldn't be detected another way. Moreover, it seems like the process would lead to some very large number of equally marginal test cases. We aren't likely to accept such a patch, because we are concerned about keeping down the runtime of the test suite. regards, tom lane ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Contributing test cases to improve coverage @ 2024-06-12 19:51 J F <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: J F @ 2024-06-12 19:51 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] > I am quite confused about what is the point of this. You have not > found any actual bug, nor have you demonstrated that this test case > could discover a likely future bug that wouldn't be detected another > way. Moreover, it seems like the process would lead to some very > large number of equally marginal test cases. We aren't likely to > accept such a patch, because we are concerned about keeping down the > runtime of the test suite. > > regards, tom lane The point of this project is to improve the coverage of PostgreSQL’s preexisting test suite. Writing a test suite to achieve close to 100% coverage is challenging, but I have proposed a workflow to automate this process. I assert that no test case in the regression test suite currently covers the comparator in the expression rte->rtekind == RTE_SUBQUERY. I propose adding a new test case that addresses exactly this. In the future, if someone accidentally modifies the operator to become >=, it will trigger incorrect behavior when certain queries are executed. This test case will catch that issue. I get that the test cases in /regress are likely reserved for actual bugs found and are designed to run quickly. Would it be a good idea to have a separate, more rigorous test suite that runs longer but provides better code coverage? Regards, Jon ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Contributing test cases to improve coverage @ 2024-06-13 13:59 Ashutosh Bapat <[email protected]> parent: J F <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Ashutosh Bapat @ 2024-06-13 13:59 UTC (permalink / raw) To: J F <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected] On Thu, Jun 13, 2024 at 1:22 AM J F <[email protected]> wrote: > > I am quite confused about what is the point of this. You have not > > found any actual bug, nor have you demonstrated that this test case > > could discover a likely future bug that wouldn't be detected another > > way. Moreover, it seems like the process would lead to some very > > large number of equally marginal test cases. We aren't likely to > > accept such a patch, because we are concerned about keeping down the > > runtime of the test suite. > > > > regards, tom lane > > > The point of this project is to improve the coverage of PostgreSQL’s > preexisting test suite. Writing a test suite to achieve close to 100% > coverage is challenging, but I have proposed a workflow to automate this > process. > We monitor code coverage. But this is doing more than that. It will find out the places in code, which if changed, will cause bugs. That seems useful to avoid refactoring mistakes, esp. when people rely on regression tests to tell whether their code changes are sane. But in PostgreSQL, we rely heavily on reviewers and committers to do that instead of tests. Still, the tests produced by this tool will help catch bugs that human eyes can not. As Tom said, managing that battery of tests may not be worth it. Basically, I am flip-flopping on the usefulness of this effort. > > I assert that no test case in the regression test suite currently covers > the comparator in the expression rte->rtekind == RTE_SUBQUERY. I propose > adding a new test case that addresses exactly this. In the future, if > someone accidentally modifies the operator to become >=, it will trigger > incorrect behavior when certain queries are executed. This test case will > catch that issue. > Usually PostgreSQL developers know that rtekind is an enum so they are very very unlikely to use anything other than == and !=. Such a change will be caught by a reviewer. I think many of the tests that this tool will produce will fall in this category. > > I get that the test cases in /regress are likely reserved for actual bugs > found and are designed to run quickly. Would it be a good idea to have a > separate, more rigorous test suite that runs longer but provides better > code coverage? > > There are practical difficulties like maintaining the expected outputs for such a large battery of tests. But maybe some external project could. BTW, have you considered perl tests, isolation tests etc. Tests in regress/ do not cover many subsystems e.g. replication. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-06-13 13:59 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-03 05:24 [PATCH v2] windows: Only consider us to be running as service if stderr is invalid. Andres Freund <[email protected]> 2024-06-12 18:22 Re: Contributing test cases to improve coverage Tom Lane <[email protected]> 2024-06-12 19:51 ` Re: Contributing test cases to improve coverage J F <[email protected]> 2024-06-13 13:59 ` Re: Contributing test cases to improve coverage Ashutosh Bapat <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox