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. 3+ messages / 2 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; 3+ 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] 3+ messages in thread
* pg_tablespace_location() failure with allow_in_place_tablespaces @ 2022-03-04 06:44 Michael Paquier <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Michael Paquier @ 2022-03-04 06:44 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, While playing with tablespaces and recovery in a TAP test, I have noticed that retrieving the location of a tablespace created with allow_in_place_tablespaces enabled fails in pg_tablespace_location(), because readlink() sees a directory in this case. The use may be limited to any automated testing and allow_in_place_tablespaces is a developer GUC, still it seems to me that there is an argument to allow the case rather than tweak any tests to hardcode a path with the tablespace OID. And any other code paths are able to handle such tablespaces, be they in recovery or in tablespace create/drop. A junction point is a directory on WIN32 as far as I recall, but pgreadlink() is here to ensure that we get the correct path on a source found as pgwin32_is_junction(), so we can rely on that. This stuff has led me to the attached. Thoughts? -- Michael Attachments: [text/x-diff] tbspace-inplace-location.patch (4.3K, ../../[email protected]/2-tbspace-inplace-location.patch) download | inline diff: diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c index e79eb6b478..59b8f8196c 100644 --- a/src/backend/utils/adt/misc.c +++ b/src/backend/utils/adt/misc.c @@ -15,6 +15,7 @@ #include "postgres.h" #include <sys/file.h> +#include <sys/stat.h> #include <dirent.h> #include <fcntl.h> #include <math.h> @@ -307,8 +308,7 @@ pg_tablespace_location(PG_FUNCTION_ARGS) { Oid tablespaceOid = PG_GETARG_OID(0); char sourcepath[MAXPGPATH]; - char targetpath[MAXPGPATH]; - int rllen; + struct stat st; /* * It's useful to apply this function to pg_class.reltablespace, wherein @@ -333,20 +333,57 @@ pg_tablespace_location(PG_FUNCTION_ARGS) */ snprintf(sourcepath, sizeof(sourcepath), "pg_tblspc/%u", tablespaceOid); - rllen = readlink(sourcepath, targetpath, sizeof(targetpath)); - if (rllen < 0) + /* + * Before reading the link, check if it is a link or a directory. + * A directory is possible for a tablespace created with + * allow_in_place_tablespaces enabled. On Windows, junction points + * are directories, which is why this is checked first. + */ + if (lstat(sourcepath, &st) < 0) + { ereport(ERROR, (errcode_for_file_access(), - errmsg("could not read symbolic link \"%s\": %m", + errmsg("could not stat file \"%s\": %m", sourcepath))); - if (rllen >= sizeof(targetpath)) - ereport(ERROR, - (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), - errmsg("symbolic link \"%s\" target is too long", - sourcepath))); - targetpath[rllen] = '\0'; + } + else if ( +#ifndef WIN32 + S_ISLNK(st.st_mode) +#else + pgwin32_is_junction(pathbuf) +#endif + ) + { + char targetpath[MAXPGPATH]; + int rllen; - PG_RETURN_TEXT_P(cstring_to_text(targetpath)); + rllen = readlink(sourcepath, targetpath, sizeof(targetpath)); + if (rllen < 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read symbolic link \"%s\": %m", + sourcepath))); + if (rllen >= sizeof(targetpath)) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("symbolic link \"%s\" target is too long", + sourcepath))); + targetpath[rllen] = '\0'; + + PG_RETURN_TEXT_P(cstring_to_text(targetpath)); + } + else if (S_ISDIR(st.st_mode)) + { + /* + * For a directory, return the relative path of the source, + * as created by allow_in_place_tablespaces. This is useful + * for regression tests. + */ + PG_RETURN_TEXT_P(cstring_to_text(sourcepath)); + } + else + elog(ERROR, "\"%s\" is not a directory or symbolic link", + sourcepath); #else ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out index 2dfbcfdebe..473fe8c28e 100644 --- a/src/test/regress/expected/tablespace.out +++ b/src/test/regress/expected/tablespace.out @@ -24,6 +24,15 @@ SELECT spcoptions FROM pg_tablespace WHERE spcname = 'regress_tblspacewith'; DROP TABLESPACE regress_tblspacewith; -- create a tablespace we can use CREATE TABLESPACE regress_tblspace LOCATION ''; +-- This returns a relative path as of an effect of allow_in_place_tablespaces, +-- masking the tablespace OID used in the path name. +SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN') + FROM pg_tablespace WHERE spcname = 'regress_tblspace'; + regexp_replace +---------------- + pg_tblspc/NNN +(1 row) + -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql index 896f05cea3..0949a28488 100644 --- a/src/test/regress/sql/tablespace.sql +++ b/src/test/regress/sql/tablespace.sql @@ -22,6 +22,10 @@ DROP TABLESPACE regress_tblspacewith; -- create a tablespace we can use CREATE TABLESPACE regress_tblspace LOCATION ''; +-- This returns a relative path as of an effect of allow_in_place_tablespaces, +-- masking the tablespace OID used in the path name. +SELECT regexp_replace(pg_tablespace_location(oid), '(pg_tblspc)/(\d+)', '\1/NNN') + FROM pg_tablespace WHERE spcname = 'regress_tblspace'; -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pg_tablespace_location() failure with allow_in_place_tablespaces @ 2022-03-07 11:36 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Michael Paquier @ 2022-03-07 11:36 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Thomas Munro <[email protected]> On Fri, Mar 04, 2022 at 03:44:22PM +0900, Michael Paquier wrote: > The use may be limited to any automated testing and > allow_in_place_tablespaces is a developer GUC, still it seems to me > that there is an argument to allow the case rather than tweak any > tests to hardcode a path with the tablespace OID. And any other code > paths are able to handle such tablespaces, be they in recovery or in > tablespace create/drop. > > A junction point is a directory on WIN32 as far as I recall, but > pgreadlink() is here to ensure that we get the correct path on > a source found as pgwin32_is_junction(), so we can rely on that. This > stuff has led me to the attached. Thomas, I'd rather fix this for the sake of the tests. One point is that the function returns a relative path for in-place tablespaces, but it would be easy enough to append a DataDir. What do you think? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-03-07 11:36 UTC | newest] Thread overview: 3+ 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]> 2022-03-04 06:44 pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[email protected]> 2022-03-07 11:36 ` Re: pg_tablespace_location() failure with allow_in_place_tablespaces Michael Paquier <[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