public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
3+ messages / 3 participants
[nested] [flat]
* [PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..de2b56c2fa 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..e3da0622d7 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--wac7ysb48OaltWcw--
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: not null constraints, again
@ 2024-09-26 12:19 jian he <[email protected]>
2024-09-26 16:53 ` Re: not null constraints, again Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: jian he @ 2024-09-26 12:19 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Tender Wang <[email protected]>; Pg Hackers <[email protected]>
+-- a PK in parent must have a not-null in child that it can mark inherited
+create table inh_parent (a int primary key);
+create table inh_child (a int primary key);
+alter table inh_child inherit inh_parent; -- nope
+alter table inh_child alter a set not null;
+alter table inh_child inherit inh_parent; -- now it works
+ERROR: relation "inh_parent" would be inherited from more than once
in src/test/regress/sql/inherit.sql, the comments at the end of the
command, seem to conflict with the output?
-------------------------------------------------------------------------------
ALTER TABLE ALTER COLUMN SET NOT NULL
implicitly means
ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT.
So in ATExecSetNotNull
if (conForm->connoinherit && recurse)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change NO INHERIT status of NOT
NULL constraint \"%s\" on relation \"%s\"",
NameStr(conForm->conname),
RelationGetRelationName(rel)));
should be
if (conForm->connoinherit)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change NO INHERIT status of NOT
NULL constraint \"%s\" on relation \"%s\"",
NameStr(conForm->conname),
RelationGetRelationName(rel)));
then we can avoid the weird case like below:
drop table if exists pp1;
create table pp1 (f1 int not null no inherit);
ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
-------------------------------------------------------------------------------
+ else if (rel->rd_rel->relhassubclass &&
+ find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
+ {
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("not-null constraint on column \"%s\" must be removed in
child tables too",
+ colName),
+ errhint("Do not specify the ONLY keyword."));
+ }
this part in ATExecDropNotNull is not necessary?
per alter_table.sql
<<<<<<---------->>>>>>
-- make sure we can drop a constraint on the parent but it remains on the child
CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT
"test_drop_constr_parent_c_check";
<<<<<<---------->>>>>>
by the same way, we can drop a not-null constraint ONLY on the parent,
but it remains on the child.
if we not remove the above part then
ALTER TABLE ONLY DROP CONSTRAINT
will behave differently from
ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL.
example:
drop table pp1,cc1, cc2;
create table pp1 (f1 int not null);
create table cc1 (f2 text, f3 int) inherits (pp1);
create table cc2(f4 float) inherits(pp1,cc1);
alter table only pp1 drop constraint pp1_f1_not_null; --works.
alter table only pp1 alter column f1 drop not null; --- error, should also work.
-------------------------------------------------------------------------------
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: not null constraints, again
2024-09-26 12:19 Re: not null constraints, again jian he <[email protected]>
@ 2024-09-26 16:53 ` Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Alvaro Herrera @ 2024-09-26 16:53 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Tender Wang <[email protected]>; Pg Hackers <[email protected]>
On 2024-Sep-26, jian he wrote:
> +-- a PK in parent must have a not-null in child that it can mark inherited
> +create table inh_parent (a int primary key);
> +create table inh_child (a int primary key);
> +alter table inh_child inherit inh_parent; -- nope
> +alter table inh_child alter a set not null;
> +alter table inh_child inherit inh_parent; -- now it works
> +ERROR: relation "inh_parent" would be inherited from more than once
> in src/test/regress/sql/inherit.sql, the comments at the end of the
> command, seem to conflict with the output?
Outdated, useless -- removed.
> -------------------------------------------------------------------------------
>
> ALTER TABLE ALTER COLUMN SET NOT NULL
> implicitly means
> ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT.
>
> So in ATExecSetNotNull
> if (conForm->connoinherit && recurse)
> ereport(ERROR,
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
> NameStr(conForm->conname),
> RelationGetRelationName(rel)));
> should be
> if (conForm->connoinherit)
> ereport(ERROR,
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
> NameStr(conForm->conname),
> RelationGetRelationName(rel)));
>
> then we can avoid the weird case like below:
>
> drop table if exists pp1;
> create table pp1 (f1 int not null no inherit);
> ALTER TABLE pp1 ALTER f1 SET NOT NULL;
> ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
Hmm, I don't understand why you say SET NOT NULL implicitly means SET
NOT NULL NO INHERIT. That's definitely not the intention. As I
explained earlier, the normal state is that a constraint is inheritable,
so if you do SET NOT NULL you want that constraint to be INHERIT.
Anyway, I don't see what you see as weird in the commands you list. To
me it reacts like this:
=# create table pp1 (f1 int not null no inherit);
CREATE TABLE
=# ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ERROR: cannot change NO INHERIT status of NOT NULL constraint "pp1_f1_not_null" on relation "pp1"
=# ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
ALTER TABLE
=# \d+ pp1
Tabla «public.pp1»
Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento │ Compresión │ Estadísticas │ Descripción
─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼────────────┼──────────────┼─────────────
f1 │ integer │ │ not null │ │ plain │ │ │
Not-null constraints:
"pp1_f1_not_null" NOT NULL "f1" NO INHERIT
Método de acceso: heap
which seems to be exactly what we want.
> -------------------------------------------------------------------------------
>
> + else if (rel->rd_rel->relhassubclass &&
> + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
> + {
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("not-null constraint on column \"%s\" must be removed in
> child tables too",
> + colName),
> + errhint("Do not specify the ONLY keyword."));
> + }
> this part in ATExecDropNotNull is not necessary?
>
> per alter_table.sql
> <<<<<<---------->>>>>>
> -- make sure we can drop a constraint on the parent but it remains on the child
> CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
> CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
> ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT
> "test_drop_constr_parent_c_check";
> <<<<<<---------->>>>>>
> by the same way, we can drop a not-null constraint ONLY on the parent,
> but it remains on the child.
> if we not remove the above part then
> ALTER TABLE ONLY DROP CONSTRAINT
> will behave differently from
> ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL.
>
> example:
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int not null);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
>
> alter table only pp1 drop constraint pp1_f1_not_null; --works.
> alter table only pp1 alter column f1 drop not null; --- error, should also work.
> -------------------------------------------------------------------------------
Hmm. I'm not sure I like this behavior, but there is precedent in
CHECK, and since DROP CONSTRAINT also already works that way, I suppose
DROP NOT NULL should do that too. I'll get it changed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code. Too bad I can't do this at work (Oracle 8/9)." (Tom Allison)
http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-09-26 16:53 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 17:44 [PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2024-09-26 12:19 Re: not null constraints, again jian he <[email protected]>
2024-09-26 16:53 ` Re: not null constraints, again Alvaro Herrera <[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