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 / 3 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
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). @ 2024-03-05 12:36 Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Alvaro Herrera @ 2024-03-05 12:36 UTC (permalink / raw) To: Dean Rasheed <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Amul Sul <[email protected]>; vignesh C <[email protected]> On 2024-Mar-04, Dean Rasheed wrote: > I don't think that this is the right fix. ISTM that the real issue is > that dropping a NOT NULL constraint should not mark the column as > nullable if it is part of a PK, whether or not that PK is deferrable > -- a deferrable PK still marks a column as not nullable. Yeah. As I said upthread, a good fix seems to require no longer relying on RelationGetIndexAttrBitmap to obtain the columns in the primary key, because that function does not include deferred primary keys. I came up with the attached POC, which seems to fix the reported problem, but of course it needs more polish, a working test case, and verifying whether the new function should be used in more places -- in particular, whether it can be used to revert the changes to RelationGetIndexList that b0e96f311985 did. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La persona que no quería pecar / estaba obligada a sentarse en duras y empinadas sillas / desprovistas, por cierto de blandos atenuantes" (Patricio Vogel) Attachments: [text/x-diff] 0001-Don-t-lose-attnotnull-if-a-deferred-PK-supports-it.patch (3.3K, ../../[email protected]/2-0001-Don-t-lose-attnotnull-if-a-deferred-PK-supports-it.patch) download | inline diff: From cc7032c3f62949d20bc98d4b3c70cb2cb8a0dd5e Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <[email protected]> Date: Tue, 5 Mar 2024 13:32:50 +0100 Subject: [PATCH] Don't lose attnotnull if a deferred PK supports it When dropping a NOT NULL constraint on a column that has a deferred primary key, we would reset attnotnull, but that's bogus. XXX this patch is nowhere near final. Reported-by: Amul Sul <[email protected]> Discussion: https://postgr.es/m/CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com --- src/backend/commands/tablecmds.c | 71 ++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index c61f9305c2..84c7871dda 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -12704,6 +12704,73 @@ ATExecDropConstraint(Relation rel, const char *constrName, table_close(conrel, RowExclusiveLock); } +/* + * dropconstraint_getpkcols -- subroutine for dropconstraint_internal + * + * This is a workaround to the fact that RelationGetIndexAttrBitmap does + * not consider a DEFERRABLE PRIMARY KEY to be a real primary key. Maybe + * this code should be elsewhere. + */ +static Bitmapset * +dropconstraint_getpkcols(Relation rel) +{ + Relation indrel; + SysScanDesc indscan; + ScanKeyData skey; + HeapTuple htup; + Bitmapset *b = NULL; + + /* + * We try first to obtain a list of PK columns from the cache; if there + * is one, we're done. + */ + b = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY); + if (b != NULL) + return b; + + /* Prepare to scan pg_index for entries having indrelid = this rel. */ + ScanKeyInit(&skey, + Anum_pg_index_indrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + + indrel = table_open(IndexRelationId, AccessShareLock); + indscan = systable_beginscan(indrel, IndexIndrelidIndexId, true, + NULL, 1, &skey); + + while (HeapTupleIsValid(htup = systable_getnext(indscan))) + { + Form_pg_index index = (Form_pg_index) GETSTRUCT(htup); + + /* + * Ignore any indexes that are currently being dropped and those that + * aren't a primary key. + */ + if (!index->indislive) + continue; + if (!index->indisprimary) + continue; + + /* + * If this primary key was IMMEDIATE, it would have been returned + * by RelationGetIndexAttrBitmap. + */ + Assert(!index->indimmediate); + + for (int i = 0; i < index->indnatts; i++) + { + int attrnum = index->indkey.values[i]; + + b = bms_add_member(b, attrnum - FirstLowInvalidHeapAttributeNumber); + } + } + + systable_endscan(indscan); + table_close(indrel, AccessShareLock); + + return b; +} + /* * Remove a constraint, using its pg_constraint tuple * @@ -12837,9 +12904,7 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha * We want to test columns for their presence in the primary key, but * only if we're not dropping it. */ - pkcols = dropping_pk ? NULL : - RelationGetIndexAttrBitmap(rel, - INDEX_ATTR_BITMAP_PRIMARY_KEY); + pkcols = dropping_pk ? NULL : dropconstraint_getpkcols(rel); ircols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY); foreach(lc, unconstrained_cols) -- 2.39.2 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). @ 2024-03-05 14:58 Dean Rasheed <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Dean Rasheed @ 2024-03-05 14:58 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Amul Sul <[email protected]>; vignesh C <[email protected]> On Tue, 5 Mar 2024 at 12:36, Alvaro Herrera <[email protected]> wrote: > > Yeah. As I said upthread, a good fix seems to require no longer relying > on RelationGetIndexAttrBitmap to obtain the columns in the primary key, > because that function does not include deferred primary keys. I came up > with the attached POC, which seems to fix the reported problem, but of > course it needs more polish, a working test case, and verifying whether > the new function should be used in more places -- in particular, whether > it can be used to revert the changes to RelationGetIndexList that > b0e96f311985 did. > Looking at the other places that call RelationGetIndexAttrBitmap() with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include deferrable PKs, since they are relying on the result to see which columns are not nullable. So there are other bugs here. For example: CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text); CREATE TABLE bar (LIKE foo); now fails to mark bar.id as not nullable, whereas prior to b0e96f311985 it would have been. So I think RelationGetIndexAttrBitmap() should include deferrable PKs, but not all the changes made to RelationGetIndexList() by b0e96f311985 need reverting. Regards, Dean ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-03-05 14:58 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]> 2024-03-05 12:36 Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Alvaro Herrera <[email protected]> 2024-03-05 14:58 ` Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s). Dean Rasheed <[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