public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v46 3/7] Make archiver process an auxiliary process 5+ messages / 4 participants [nested] [flat]
* [PATCH v46 3/7] Make archiver process an auxiliary process @ 2020-03-13 07:59 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:59 UTC (permalink / raw) This is a preliminary patch for shared-memory based stats collector. Archiver process must be a auxiliary process since it uses shared memory after stats data was moved into shared-memory. Make the process an auxiliary process in order to make it work. --- src/backend/access/transam/xlogarchive.c | 6 +- src/backend/bootstrap/bootstrap.c | 22 ++-- src/backend/postmaster/pgarch.c | 130 +++-------------------- src/backend/postmaster/postmaster.c | 50 +++++---- src/backend/storage/lmgr/proc.c | 1 + src/include/access/xlog.h | 3 + src/include/access/xlogarchive.h | 1 + src/include/miscadmin.h | 2 + src/include/postmaster/pgarch.h | 4 +- src/include/storage/pmsignal.h | 1 - src/include/storage/proc.h | 3 + 11 files changed, 69 insertions(+), 154 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 1c5a4f8b5a..d01859bde5 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -29,7 +29,9 @@ #include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" +#include "storage/latch.h" #include "storage/pmsignal.h" +#include "storage/proc.h" /* * Attempt to retrieve the specified file from off-line archival storage. @@ -490,8 +492,8 @@ XLogArchiveNotify(const char *xlog) } /* Notify archiver that it's got something to do */ - if (IsUnderPostmaster) - SendPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER); + if (IsUnderPostmaster && ProcGlobal->archiverLatch) + SetLatch(ProcGlobal->archiverLatch); } /* diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..41da0c5059 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -317,6 +317,9 @@ AuxiliaryProcessMain(int argc, char *argv[]) case StartupProcess: MyBackendType = B_STARTUP; break; + case ArchiverProcess: + MyBackendType = B_ARCHIVER; + break; case BgWriterProcess: MyBackendType = B_BG_WRITER; break; @@ -437,30 +440,29 @@ AuxiliaryProcessMain(int argc, char *argv[]) proc_exit(1); /* should never return */ case StartupProcess: - /* don't set signals, startup process has its own agenda */ StartupProcessMain(); - proc_exit(1); /* should never return */ + proc_exit(1); + + case ArchiverProcess: + PgArchiverMain(); + proc_exit(1); case BgWriterProcess: - /* don't set signals, bgwriter has its own agenda */ BackgroundWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case CheckpointerProcess: - /* don't set signals, checkpointer has its own agenda */ CheckpointerMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalWriterProcess: - /* don't set signals, walwriter has its own agenda */ InitXLOGAccess(); WalWriterMain(); - proc_exit(1); /* should never return */ + proc_exit(1); case WalReceiverProcess: - /* don't set signals, walreceiver has its own agenda */ WalReceiverMain(); - proc_exit(1); /* should never return */ + proc_exit(1); default: elog(PANIC, "unrecognized process type: %d", (int) MyAuxProcType); diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index edec311f12..9a2e21bf86 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -48,6 +48,7 @@ #include "storage/latch.h" #include "storage/pg_shmem.h" #include "storage/pmsignal.h" +#include "storage/procsignal.h" #include "utils/guc.h" #include "utils/ps_status.h" @@ -78,13 +79,11 @@ * Local data * ---------- */ -static time_t last_pgarch_start_time; static time_t last_sigterm_time = 0; /* * Flags set by interrupt handlers for later service in the main loop. */ -static volatile sig_atomic_t wakened = false; static volatile sig_atomic_t ready_to_stop = false; /* ---------- @@ -95,8 +94,6 @@ static volatile sig_atomic_t ready_to_stop = false; static pid_t pgarch_forkexec(void); #endif -NON_EXEC_STATIC void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -static void pgarch_waken(SIGNAL_ARGS); static void pgarch_waken_stop(SIGNAL_ARGS); static void pgarch_MainLoop(void); static void pgarch_ArchiverCopyLoop(void); @@ -110,75 +107,6 @@ static void pgarch_archiveDone(char *xlog); * ------------------------------------------------------------ */ -/* - * pgarch_start - * - * Called from postmaster at startup or after an existing archiver - * died. Attempt to fire up a fresh archiver process. - * - * Returns PID of child process, or 0 if fail. - * - * Note: if fail, we will be called again from the postmaster main loop. - */ -int -pgarch_start(void) -{ - time_t curtime; - pid_t pgArchPid; - - /* - * Do nothing if no archiver needed - */ - if (!XLogArchivingActive()) - return 0; - - /* - * Do nothing if too soon since last archiver start. This is a safety - * valve to protect against continuous respawn attempts if the archiver is - * dying immediately at launch. Note that since we will be re-called from - * the postmaster main loop, we will get another chance later. - */ - curtime = time(NULL); - if ((unsigned int) (curtime - last_pgarch_start_time) < - (unsigned int) PGARCH_RESTART_INTERVAL) - return 0; - last_pgarch_start_time = curtime; - -#ifdef EXEC_BACKEND - switch ((pgArchPid = pgarch_forkexec())) -#else - switch ((pgArchPid = fork_process())) -#endif - { - case -1: - ereport(LOG, - (errmsg("could not fork archiver: %m"))); - return 0; - -#ifndef EXEC_BACKEND - case 0: - /* in postmaster child ... */ - InitPostmasterChild(); - - /* Close the postmaster's sockets */ - ClosePostmasterPorts(false); - - /* Drop our connection to postmaster's shared memory, as well */ - dsm_detach_all(); - PGSharedMemoryDetach(); - - PgArchiverMain(0, NULL); - break; -#endif - - default: - return (int) pgArchPid; - } - - /* shouldn't get here */ - return 0; -} - /* ------------------------------------------------------------ * Local functions called by archiver follow * ------------------------------------------------------------ @@ -212,14 +140,9 @@ pgarch_forkexec(void) #endif /* EXEC_BACKEND */ -/* - * PgArchiverMain - * - * The argc/argv parameters are valid only in EXEC_BACKEND case. However, - * since we don't use 'em, it hardly matters... - */ -NON_EXEC_STATIC void -PgArchiverMain(int argc, char *argv[]) +/* Main entry point for archiver process */ +void +PgArchiverMain(void) { /* * Ignore all signals usually bound to some action in the postmaster, @@ -231,33 +154,26 @@ PgArchiverMain(int argc, char *argv[]) /* SIGQUIT handler was already set up by InitPostmasterChild */ pqsignal(SIGALRM, SIG_IGN); pqsignal(SIGPIPE, SIG_IGN); - pqsignal(SIGUSR1, pgarch_waken); + pqsignal(SIGUSR1, procsignal_sigusr1_handler); pqsignal(SIGUSR2, pgarch_waken_stop); + /* Reset some signals that are accepted by postmaster but not here */ pqsignal(SIGCHLD, SIG_DFL); + + /* Unblock signals (they were blocked when the postmaster forked us) */ PG_SETMASK(&UnBlockSig); - MyBackendType = B_ARCHIVER; - init_ps_display(NULL); + /* + * Advertise our latch that backends can use to wake us up while we're + * sleeping. + */ + ProcGlobal->archiverLatch = &MyProc->procLatch; pgarch_MainLoop(); exit(0); } -/* SIGUSR1 signal handler for archiver process */ -static void -pgarch_waken(SIGNAL_ARGS) -{ - int save_errno = errno; - - /* set flag that there is work to be done */ - wakened = true; - SetLatch(MyLatch); - - errno = save_errno; -} - /* SIGUSR2 signal handler for archiver process */ static void pgarch_waken_stop(SIGNAL_ARGS) @@ -282,14 +198,6 @@ pgarch_MainLoop(void) pg_time_t last_copy_time = 0; bool time_to_stop; - /* - * We run the copy loop immediately upon entry, in case there are - * unarchived files left over from a previous database run (or maybe the - * archiver died unexpectedly). After that we wait for a signal or - * timeout before doing more. - */ - wakened = true; - /* * There shouldn't be anything for the archiver to do except to wait for a * signal ... however, the archiver exists to protect our data, so she @@ -328,12 +236,8 @@ pgarch_MainLoop(void) } /* Do what we're here for */ - if (wakened || time_to_stop) - { - wakened = false; - pgarch_ArchiverCopyLoop(); - last_copy_time = time(NULL); - } + pgarch_ArchiverCopyLoop(); + last_copy_time = time(NULL); /* * Sleep until a signal is received, or until a poll is forced by @@ -354,13 +258,9 @@ pgarch_MainLoop(void) WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, timeout * 1000L, WAIT_EVENT_ARCHIVER_MAIN); - if (rc & WL_TIMEOUT) - wakened = true; if (rc & WL_POSTMASTER_DEATH) time_to_stop = true; } - else - wakened = true; } /* diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 7de27ee4e0..af91c313e2 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -548,6 +548,7 @@ static void ShmemBackendArrayRemove(Backend *bn); #endif /* EXEC_BACKEND */ #define StartupDataBase() StartChildProcess(StartupProcess) +#define StartArchiver() StartChildProcess(ArchiverProcess) #define StartBackgroundWriter() StartChildProcess(BgWriterProcess) #define StartCheckpointer() StartChildProcess(CheckpointerProcess) #define StartWalWriter() StartChildProcess(WalWriterProcess) @@ -1788,7 +1789,7 @@ ServerLoop(void) /* If we have lost the archiver, try to start a new one. */ if (PgArchPID == 0 && PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* If we need to signal the autovacuum launcher, do so now */ if (avlauncher_needs_signal) @@ -3027,7 +3028,7 @@ reaper(SIGNAL_ARGS) if (!IsBinaryUpgrade && AutoVacuumingActive() && AutoVacPID == 0) AutoVacPID = StartAutoVacLauncher(); if (PgArchStartupAllowed() && PgArchPID == 0) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); if (PgStatPID == 0) PgStatPID = pgstat_start(); @@ -3162,20 +3163,16 @@ reaper(SIGNAL_ARGS) } /* - * Was it the archiver? If so, just try to start a new one; no need - * to force reset of the rest of the system. (If fail, we'll try - * again in future cycles of the main loop.). Unless we were waiting - * for it to shut down; don't restart it in that case, and - * PostmasterStateMachine() will advance to the next shutdown step. + * Was it the archiver? Normal exit can be ignored; we'll start a new + * one at the next iteration of the postmaster's main loop, if + * necessary. Any other exit condition is treated as a crash. */ if (pid == PgArchPID) { PgArchPID = 0; if (!EXIT_STATUS_0(exitstatus)) - LogChildExit(LOG, _("archiver process"), - pid, exitstatus); - if (PgArchStartupAllowed()) - PgArchPID = pgarch_start(); + HandleChildCrash(pid, exitstatus, + _("archiver process")); continue; } @@ -3423,7 +3420,7 @@ CleanupBackend(int pid, /* * HandleChildCrash -- cleanup after failed backend, bgwriter, checkpointer, - * walwriter, autovacuum, or background worker. + * walwriter, autovacuum, archiver or background worker. * * The objectives here are to clean up our local state about the child * process, and to signal all other remaining children to quickdie. @@ -3629,6 +3626,18 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) signal_child(AutoVacPID, (SendStop ? SIGSTOP : SIGQUIT)); } + /* Take care of the archiver too */ + if (pid == PgArchPID) + PgArchPID = 0; + else if (PgArchPID != 0 && take_action) + { + ereport(DEBUG2, + (errmsg_internal("sending %s to process %d", + (SendStop ? "SIGSTOP" : "SIGQUIT"), + (int) PgArchPID))); + signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT)); + } + /* * Force a power-cycle of the pgarch process too. (This isn't absolutely * necessary, but it seems like a good idea for robustness, and it @@ -3932,6 +3941,7 @@ PostmasterStateMachine(void) Assert(CheckpointerPID == 0); Assert(WalWriterPID == 0); Assert(AutoVacPID == 0); + Assert(PgArchPID == 0); /* syslogger is not considered here */ pmState = PM_NO_CHILDREN; } @@ -5160,7 +5170,7 @@ sigusr1_handler(SIGNAL_ARGS) */ Assert(PgArchPID == 0); if (XLogArchivingAlways()) - PgArchPID = pgarch_start(); + PgArchPID = StartArchiver(); /* * If we aren't planning to enter hot standby mode later, treat @@ -5214,16 +5224,6 @@ sigusr1_handler(SIGNAL_ARGS) if (StartWorkerNeeded || HaveCrashedWorker) maybe_start_bgworkers(); - if (CheckPostmasterSignal(PMSIGNAL_WAKEN_ARCHIVER) && - PgArchPID != 0) - { - /* - * Send SIGUSR1 to archiver process, to wake it up and begin archiving - * next WAL file. - */ - signal_child(PgArchPID, SIGUSR1); - } - /* Tell syslogger to rotate logfile if requested */ if (SysLoggerPID != 0) { @@ -5465,6 +5465,10 @@ StartChildProcess(AuxProcType type) ereport(LOG, (errmsg("could not fork startup process: %m"))); break; + case ArchiverProcess: + ereport(LOG, + (errmsg("could not fork archiver process: %m"))); + break; case BgWriterProcess: ereport(LOG, (errmsg("could not fork background writer process: %m"))); diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index c87ffc6549..a1e51c5b99 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -182,6 +182,7 @@ InitProcGlobal(void) ProcGlobal->startupBufferPinWaitBufId = -1; ProcGlobal->walwriterLatch = NULL; ProcGlobal->checkpointerLatch = NULL; + ProcGlobal->archiverLatch = NULL; pg_atomic_init_u32(&ProcGlobal->procArrayGroupFirst, INVALID_PGPROCNO); pg_atomic_init_u32(&ProcGlobal->clogGroupFirst, INVALID_PGPROCNO); diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 75ec1073bd..551f518cc2 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -339,6 +339,9 @@ extern XLogRecPtr GetRedoRecPtr(void); extern XLogRecPtr GetInsertRecPtr(void); extern XLogRecPtr GetFlushRecPtr(void); extern XLogRecPtr GetLastImportantRecPtr(void); +extern void XLogArchiveWakeupStart(void); +extern void XLogArchiveWakeupEnd(void); +extern void XLogArchiveWakeup(void); extern void RemovePromoteSignalFiles(void); extern bool PromoteIsTriggered(void); diff --git a/src/include/access/xlogarchive.h b/src/include/access/xlogarchive.h index 3edd1a976c..1a59181cf9 100644 --- a/src/include/access/xlogarchive.h +++ b/src/include/access/xlogarchive.h @@ -25,6 +25,7 @@ extern void ExecuteRecoveryCommand(const char *command, const char *commandName, extern void KeepFileRestoredFromArchive(const char *path, const char *xlogfname); extern void XLogArchiveNotify(const char *xlog); extern void XLogArchiveNotifySeg(XLogSegNo segno); +extern void XLogArchiveWakeup(void); extern void XLogArchiveForceDone(const char *xlog); extern bool XLogArchiveCheckDone(const char *xlog); extern bool XLogArchiveIsBusy(const char *xlog); diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 1bdc97e308..adb9f819bb 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -419,6 +419,7 @@ typedef enum BootstrapProcess, StartupProcess, BgWriterProcess, + ArchiverProcess, CheckpointerProcess, WalWriterProcess, WalReceiverProcess, @@ -431,6 +432,7 @@ extern AuxProcType MyAuxProcType; #define AmBootstrapProcess() (MyAuxProcType == BootstrapProcess) #define AmStartupProcess() (MyAuxProcType == StartupProcess) #define AmBackgroundWriterProcess() (MyAuxProcType == BgWriterProcess) +#define AmArchiverProcess() (MyAuxProcType == ArchiverProcess) #define AmCheckpointerProcess() (MyAuxProcType == CheckpointerProcess) #define AmWalWriterProcess() (MyAuxProcType == WalWriterProcess) #define AmWalReceiverProcess() (MyAuxProcType == WalReceiverProcess) diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h index d102a21ab7..385b002dfe 100644 --- a/src/include/postmaster/pgarch.h +++ b/src/include/postmaster/pgarch.h @@ -32,8 +32,6 @@ */ extern int pgarch_start(void); -#ifdef EXEC_BACKEND -extern void PgArchiverMain(int argc, char *argv[]) pg_attribute_noreturn(); -#endif +extern void PgArchiverMain(void) pg_attribute_noreturn(); #endif /* _PGARCH_H */ diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h index dbbed18f61..8ed4d87ae6 100644 --- a/src/include/storage/pmsignal.h +++ b/src/include/storage/pmsignal.h @@ -34,7 +34,6 @@ typedef enum { PMSIGNAL_RECOVERY_STARTED, /* recovery has started */ PMSIGNAL_BEGIN_HOT_STANDBY, /* begin Hot Standby */ - PMSIGNAL_WAKEN_ARCHIVER, /* send a NOTIFY signal to xlog archiver */ PMSIGNAL_ROTATE_LOGFILE, /* send SIGUSR1 to syslogger to rotate logfile */ PMSIGNAL_START_AUTOVAC_LAUNCHER, /* start an autovacuum launcher */ PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 0786fcf103..430d438303 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -354,6 +354,9 @@ typedef struct PROC_HDR int startupProcPid; /* Buffer id of the buffer that Startup process waits for pin on, or -1 */ int startupBufferPinWaitBufId; + /* Archiver process's latch */ + Latch *archiverLatch; + /* Current shared estimate of appropriate spins_per_delay value */ } PROC_HDR; extern PGDLLIMPORT PROC_HDR *ProcGlobal; -- 2.27.0 ----Next_Part(Thu_Jan_14_15_14_25_2021_903)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v46-0004-Shared-memory-based-stats-collector.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: not null constraints, again @ 2025-04-15 21:12 Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2025-04-15 21:12 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tender Wang <[email protected]>; jian he <[email protected]>; Pg Hackers <[email protected]> Alvaro Herrera <[email protected]> writes: > On 2025-Apr-15, Tom Lane wrote: >> Looking at the patch itself, it doesn't seem like the got_children >> flag is accomplishing anything; > Ah yes, I forgot to set got_children when reading the children list. > This happens within the loop for columns, so the idea is to obtain that > list just once instead of once per column. Ah, got it. Makes sense as long as you actually avoid the work ;-) regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: not null constraints, again @ 2025-04-16 11:24 Alvaro Herrera <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Alvaro Herrera @ 2025-04-16 11:24 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Tender Wang <[email protected]>; jian he <[email protected]>; Pg Hackers <[email protected]> Here's another version where I do skip searching for children twice, and rewrote the comments. I also noticed that in child tables we were only looking for pg_attribute.attnotnull, and not whether the constraints had been validated or made inheritable. This seemed a wasted opportunity, so I refactored the code to instead examine the pg_constraint row and apply the same checks as for the constraint on the parent (namely, that it's valid and not NO INHERIT). We already check for these things downstream (alter table phase 2, during AdjustNotNullInheritance), but only after potentially wasting more work, so it makes sense to do it here (alter table phase 1) given that it's very easy. I added some tests for these things also, as those cases weren't covered. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "¿Cómo puedes confiar en algo que pagas y que no ves, y no confiar en algo que te dan y te lo muestran?" (Germán Poo) Attachments: [text/x-diff] v2-0001-Fix-verification-of-not-null-constraints-on-child.patch (12.6K, ../../[email protected]/2-v2-0001-Fix-verification-of-not-null-constraints-on-child.patch) download | inline diff: From 710ba9b55d9fcf38464c9d2e00e2946e051425ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Tue, 15 Apr 2025 20:38:54 +0200 Subject: [PATCH v2] Fix verification of not-null constraints on children during PK creation --- src/backend/commands/tablecmds.c | 145 +++++++++++++--------- src/test/regress/expected/constraints.out | 22 +++- src/test/regress/sql/constraints.sql | 14 +++ 3 files changed, 122 insertions(+), 59 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b3ed69457fc..a485f045890 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -540,6 +540,7 @@ static ObjectAddress ATExecDropColumn(List **wqueue, Relation rel, const char *c static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode, AlterTableUtilityContext *context); +static void verifyNotNullPKCompatible(HeapTuple tuple, const char *colname); static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel, IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static ObjectAddress ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, @@ -9438,8 +9439,26 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, } /* - * Prepare to add a primary key on table, by adding not-null constraints + * Prepare to add a primary key on a table, by adding not-null constraints * on all columns. + * + * The not-null constraints for a primary key must cover the whole inheritance + * hierarchy (failing to ensure that leads to funny corner cases). For the + * normal case where we're asked to recurse, this routine ensures that the + * not-null constraints either exist already, or queues a requirement for them + * to be created by phase 2. + * + * For the case where we're asked not to recurse, we verify that a not-null + * constraint exists on each column of each (direct) child table, throwing an + * error if not. Not throwing an error would also work, because a not-null + * constraint would be created anyway, but it'd cause a silent scan of the + * child table to verify absence of nulls. We prefer to let the user know so + * that they can add the constraint manually without having to hold + * AccessExclusiveLock while at it. + * + * However, it's also important that we do not acquire locks on children if + * the not-null constraints already exist on the parent, to avoid risking + * deadlocks during parallel pg_restore of PKs on partitioned tables. */ static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, @@ -9447,42 +9466,13 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, AlterTableUtilityContext *context) { Constraint *pkconstr; + List *children; + bool got_children = false; pkconstr = castNode(Constraint, cmd->def); if (pkconstr->contype != CONSTR_PRIMARY) return; - /* - * If not recursing, we must ensure that all children have a NOT NULL - * constraint on the columns, and error out if not. - */ - if (!recurse) - { - List *children; - - children = find_inheritance_children(RelationGetRelid(rel), - lockmode); - foreach_oid(childrelid, children) - { - foreach_node(String, attname, pkconstr->keys) - { - HeapTuple tup; - Form_pg_attribute attrForm; - - tup = SearchSysCacheAttName(childrelid, strVal(attname)); - if (!tup) - elog(ERROR, "cache lookup failed for attribute %s of relation %u", - strVal(attname), childrelid); - attrForm = (Form_pg_attribute) GETSTRUCT(tup); - if (!attrForm->attnotnull) - ereport(ERROR, - errmsg("column \"%s\" of table \"%s\" is not marked NOT NULL", - strVal(attname), get_rel_name(childrelid))); - ReleaseSysCache(tup); - } - } - } - /* Verify that columns are not-null, or request that they be made so */ foreach_node(String, column, pkconstr->keys) { @@ -9498,42 +9488,46 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, tuple = findNotNullConstraint(RelationGetRelid(rel), strVal(column)); if (tuple != NULL) { - Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple); - - /* a NO INHERIT constraint is no good */ - if (conForm->connoinherit) - ereport(ERROR, - errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot create primary key on column \"%s\"", - strVal(column)), - /*- translator: third %s is a constraint characteristic such as NOT VALID */ - errdetail("The constraint \"%s\" on column \"%s\", marked %s, is incompatible with a primary key.", - NameStr(conForm->conname), strVal(column), "NO INHERIT"), - errhint("You will need to make it inheritable using %s.", - "ALTER TABLE ... ALTER CONSTRAINT ... INHERIT")); - - /* an unvalidated constraint is no good */ - if (!conForm->convalidated) - ereport(ERROR, - errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot create primary key on column \"%s\"", - strVal(column)), - /*- translator: third %s is a constraint characteristic such as NOT VALID */ - errdetail("The constraint \"%s\" on column \"%s\", marked %s, is incompatible with a primary key.", - NameStr(conForm->conname), strVal(column), "NOT VALID"), - errhint("You will need to validate it using %s.", - "ALTER TABLE ... VALIDATE CONSTRAINT")); + verifyNotNullPKCompatible(tuple, strVal(column)); /* All good with this one; don't request another */ heap_freetuple(tuple); continue; } + else if (!recurse) + { + /* + * No constraint on this column. Asked not to recurse, we won't + * create one here, but verify that all children have one. + */ + if (!got_children) + { + children = find_inheritance_children(RelationGetRelid(rel), + lockmode); + /* only search for children on the first time through */ + got_children = true; + } + + foreach_oid(childrelid, children) + { + HeapTuple tup; + + tup = findNotNullConstraint(childrelid, strVal(column)); + if (!tup) + ereport(ERROR, + errmsg("column \"%s\" of table \"%s\" is not marked NOT NULL", + strVal(column), get_rel_name(childrelid))); + /* verify it's good enough */ + verifyNotNullPKCompatible(tup, strVal(column)); + } + } /* This column is not already not-null, so add it to the queue */ nnconstr = makeNotNullConstraint(column); newcmd = makeNode(AlterTableCmd); newcmd->subtype = AT_AddConstraint; + /* note we force recurse=true here; see above */ newcmd->recurse = true; newcmd->def = (Node *) nnconstr; @@ -9541,6 +9535,43 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, } } +/* + * Verify whether the given not-null constraint is compatible with a + * primary key. If not, an error is thrown. + */ +static void +verifyNotNullPKCompatible(HeapTuple tuple, const char *colname) +{ + Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple); + + if (conForm->contype != CONSTRAINT_NOTNULL) + elog(ERROR, "constraint %u is not a not-null constraint", conForm->oid); + + /* a NO INHERIT constraint is no good */ + if (conForm->connoinherit) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot create primary key on column \"%s\"", colname), + /*- translator: third %s is a constraint characteristic such as NOT VALID */ + errdetail("The constraint \"%s\" on column \"%s\" of table \"%s\", marked %s, is incompatible with a primary key.", + NameStr(conForm->conname), colname, + get_rel_name(conForm->conrelid), "NO INHERIT"), + errhint("You will need to make it inheritable using %s.", + "ALTER TABLE ... ALTER CONSTRAINT ... INHERIT")); + + /* an unvalidated constraint is no good */ + if (!conForm->convalidated) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot create primary key on column \"%s\"", colname), + /*- translator: third %s is a constraint characteristic such as NOT VALID */ + errdetail("The constraint \"%s\" on column \"%s\" of table \"%s\", marked %s, is incompatible with a primary key.", + NameStr(conForm->conname), colname, + get_rel_name(conForm->conrelid), "NOT VALID"), + errhint("You will need to validate it using %s.", + "ALTER TABLE ... VALIDATE CONSTRAINT")); +} + /* * ALTER TABLE ADD INDEX * diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index a8c6495ae01..c151ecf76e6 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -1233,7 +1233,7 @@ Indexes: create table cnn_pk (a int not null no inherit); alter table cnn_pk add primary key (a); ERROR: cannot create primary key on column "a" -DETAIL: The constraint "cnn_pk_a_not_null" on column "a", marked NO INHERIT, is incompatible with a primary key. +DETAIL: The constraint "cnn_pk_a_not_null" on column "a" of table "cnn_pk", marked NO INHERIT, is incompatible with a primary key. HINT: You will need to make it inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. drop table cnn_pk; -- Ensure partitions are scanned for null values when adding a PK @@ -1395,7 +1395,7 @@ HINT: You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to validate it. -- cannot add primary key on a column with an invalid not-null ALTER TABLE notnull_tbl1 ADD PRIMARY KEY (a); ERROR: cannot create primary key on column "a" -DETAIL: The constraint "nn" on column "a", marked NOT VALID, is incompatible with a primary key. +DETAIL: The constraint "nn" on column "a" of table "notnull_tbl1", marked NOT VALID, is incompatible with a primary key. HINT: You will need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. -- ALTER column SET NOT NULL validates an invalid constraint (but this fails -- because of rows with null values) @@ -1567,6 +1567,24 @@ ERROR: constraint "nn1" conflicts with NOT VALID constraint on child table "pp_ ALTER TABLE pp_nn_1 VALIDATE CONSTRAINT nn1; ALTER TABLE pp_nn ATTACH PARTITION pp_nn_1 FOR VALUES IN (NULL,5); --ok DROP TABLE pp_nn; +-- Try a partition with an invalid constraint and create a PK on the parent. +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +ERROR: cannot create primary key on column "a" +DETAIL: The constraint "nn" on column "a" of table "pp_nn_1", marked NOT VALID, is incompatible with a primary key. +HINT: You will need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. +DROP TABLE pp_nn; +-- same as above, but the constraint is NO INHERIT +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NO INHERIT; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +ERROR: cannot create primary key on column "a" +DETAIL: The constraint "nn" on column "a" of table "pp_nn_1", marked NO INHERIT, is incompatible with a primary key. +HINT: You will need to make it inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. +DROP TABLE pp_nn; -- Create table with NOT NULL INVALID constraint, for pg_upgrade. CREATE TABLE notnull_tbl1_upg (a int, b int); INSERT INTO notnull_tbl1_upg VALUES (NULL, 1), (NULL, 2), (300, 3); diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index bf8f0aa181d..5d6d749c150 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -940,6 +940,20 @@ ALTER TABLE pp_nn_1 VALIDATE CONSTRAINT nn1; ALTER TABLE pp_nn ATTACH PARTITION pp_nn_1 FOR VALUES IN (NULL,5); --ok DROP TABLE pp_nn; +-- Try a partition with an invalid constraint and create a PK on the parent. +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +DROP TABLE pp_nn; + +-- same as above, but the constraint is NO INHERIT +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NO INHERIT; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +DROP TABLE pp_nn; + -- Create table with NOT NULL INVALID constraint, for pg_upgrade. CREATE TABLE notnull_tbl1_upg (a int, b int); INSERT INTO notnull_tbl1_upg VALUES (NULL, 1), (NULL, 2), (300, 3); -- 2.39.5 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: not null constraints, again @ 2025-04-16 12:11 Tender Wang <[email protected]> parent: Alvaro Herrera <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Tender Wang @ 2025-04-16 12:11 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tom Lane <[email protected]>; jian he <[email protected]>; Pg Hackers <[email protected]> Alvaro Herrera <[email protected]> 于2025年4月16日周三 19:24写道： > Here's another version where I do skip searching for children twice, and > rewrote the comments. > > I also noticed that in child tables we were only looking for > pg_attribute.attnotnull, and not whether the constraints had been > validated or made inheritable. This seemed a wasted opportunity, so I > refactored the code to instead examine the pg_constraint row and apply > the same checks as for the constraint on the parent (namely, that it's > valid and not NO INHERIT). We already check for these things downstream > (alter table phase 2, during AdjustNotNullInheritance), but only after > potentially wasting more work, so it makes sense to do it here (alter > table phase 1) given that it's very easy. I added some tests for these > things also, as those cases weren't covered. > if (conForm->contype != CONSTRAINT_NOTNULL) elog(ERROR, "constraint %u is not a not-null constraint", conForm->oid); I feel that using conForm->conname is more friendly than oid for users. Others look good for me. -- Thanks, Tender Wang ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: not null constraints, again @ 2025-04-16 19:55 Tom Lane <[email protected]> parent: Alvaro Herrera <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Tom Lane @ 2025-04-16 19:55 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tender Wang <[email protected]>; jian he <[email protected]>; Pg Hackers <[email protected]> Alvaro Herrera <[email protected]> writes: > Here's another version where I do skip searching for children twice, and > rewrote the comments. v2 LGTM, with two small nits: 1. Grammar feels shaky here: + * normal case where we're asked to recurse, this routine ensures that the + * not-null constraints either exist already, or queues a requirement for them + * to be created by phase 2. The "either" seems to apply to "ensures" versus "queues", but it's in the wrong place for that. Maybe something like + * normal case where we're asked to recurse, this routine checks if the + * not-null constraints exist already, and if not queues a requirement for + * them to be created by phase 2. 2. Stupider compilers are likely to whine about the "children" variable possibly being used uninitialized. Suggest initializing it to NIL. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-04-16 19:55 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:59 [PATCH v46 3/7] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]> 2025-04-15 21:12 Re: not null constraints, again Tom Lane <[email protected]> 2025-04-16 11:24 ` Re: not null constraints, again Alvaro Herrera <[email protected]> 2025-04-16 12:11 ` Re: not null constraints, again Tender Wang <[email protected]> 2025-04-16 19:55 ` Re: not null constraints, again Tom Lane <[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