public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Demote PoC 7+ messages / 3 participants [nested] [flat]
* [PATCH] Demote PoC @ 2020-04-10 16:01 Jehan-Guillaume de Rorthais <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Jehan-Guillaume de Rorthais @ 2020-04-10 16:01 UTC (permalink / raw) --- src/backend/access/transam/xlog.c | 3 +- src/backend/postmaster/postmaster.c | 206 ++++++++++++++++++------ src/bin/pg_controldata/pg_controldata.c | 2 + src/bin/pg_ctl/pg_ctl.c | 105 ++++++++++++ src/include/catalog/pg_control.h | 1 + src/include/libpq/libpq-be.h | 7 +- src/include/utils/pidfile.h | 1 + 7 files changed, 271 insertions(+), 54 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 55cac186dc..8a7f1a0855 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -8493,6 +8493,7 @@ ShutdownXLOG(int code, Datum arg) CurrentResourceOwner = AuxProcessResourceOwner; /* Don't be chatty in standalone mode */ + // FIXME: what message when demoting? ereport(IsPostmasterEnvironment ? LOG : NOTICE, (errmsg("shutting down"))); @@ -8760,7 +8761,7 @@ CreateCheckPoint(int flags) if (shutdown) { LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); - ControlFile->state = DB_SHUTDOWNING; + ControlFile->state = DB_SHUTDOWNING; // DEMOTING? ControlFile->time = (pg_time_t) time(NULL); UpdateControlFile(); LWLockRelease(ControlFileLock); diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index b4d475bb0b..465d020f9d 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -150,6 +150,9 @@ #define BACKEND_TYPE_WORKER (BACKEND_TYPE_AUTOVAC | BACKEND_TYPE_BGWORKER) +/* file to signal demotion from primary to standby */ +#define DEMOTE_SIGNAL_FILE "demote" + /* * List of active backends (or child processes anyway; we don't actually * know whether a given child has become a backend or is still in the @@ -269,18 +272,23 @@ typedef enum static StartupStatusEnum StartupStatus = STARTUP_NOT_RUNNING; /* Startup/shutdown state */ -#define NoShutdown 0 -#define SmartShutdown 1 -#define FastShutdown 2 -#define ImmediateShutdown 3 - -static int Shutdown = NoShutdown; +typedef enum StepDownState { + NoShutdown = 0, /* find better label? */ + SmartShutdown, + SmartDemote, + FastShutdown, + FastDemote, + ImmediateShutdown +} StepDownState; + +static StepDownState StepDown = NoShutdown; +static bool DemoteSignal = false; /* true on demote request */ static bool FatalError = false; /* T if recovering from backend crash */ /* - * We use a simple state machine to control startup, shutdown, and - * crash recovery (which is rather like shutdown followed by startup). + * We use a simple state machine to control startup, shutdown, demote and + * crash recovery (both are rather like shutdown followed by startup). * * After doing all the postmaster initialization work, we enter PM_STARTUP * state and the startup process is launched. The startup process begins by @@ -314,7 +322,7 @@ static bool FatalError = false; /* T if recovering from backend crash */ * will not be very long). * * Notice that this state variable does not distinguish *why* we entered - * states later than PM_RUN --- Shutdown and FatalError must be consulted + * states later than PM_RUN --- StepDown and FatalError must be consulted * to find that out. FatalError is never true in PM_RECOVERY_* or PM_RUN * states, nor in PM_SHUTDOWN states (because we don't enter those states * when trying to recover from a crash). It can be true in PM_STARTUP state, @@ -414,6 +422,8 @@ static bool RandomCancelKey(int32 *cancel_key); static void signal_child(pid_t pid, int signal); static bool SignalSomeChildren(int signal, int targets); static void TerminateChildren(int signal); +static bool CheckDemoteSignal(void); + #define SignalChildren(sig) SignalSomeChildren(sig, BACKEND_TYPE_ALL) @@ -1550,7 +1560,7 @@ DetermineSleepTime(struct timeval *timeout) * Normal case: either there are no background workers at all, or we're in * a shutdown sequence (during which we ignore bgworkers altogether). */ - if (Shutdown > NoShutdown || + if (StepDown > NoShutdown || (!StartWorkerNeeded && !HaveCrashedWorker)) { if (AbortStartTime != 0) @@ -1830,7 +1840,7 @@ ServerLoop(void) * * Note we also do this during recovery from a process crash. */ - if ((Shutdown >= ImmediateShutdown || (FatalError && !SendStop)) && + if ((StepDown >= ImmediateShutdown || (FatalError && !SendStop)) && AbortStartTime != 0 && (now - AbortStartTime) >= SIGKILL_CHILDREN_AFTER_SECS) { @@ -2305,6 +2315,11 @@ retry1: (errcode(ERRCODE_CANNOT_CONNECT_NOW), errmsg("the database system is starting up"))); break; + case CAC_DEMOTE: + ereport(FATAL, + (errcode(ERRCODE_CANNOT_CONNECT_NOW), + errmsg("the database system is demoting"))); + break; case CAC_SHUTDOWN: ereport(FATAL, (errcode(ERRCODE_CANNOT_CONNECT_NOW), @@ -2436,7 +2451,7 @@ canAcceptConnections(int backend_type) CAC_state result = CAC_OK; /* - * Can't start backends when in startup/shutdown/inconsistent recovery + * Can't start backends when in startup/demote/shutdown/inconsistent recovery * state. We treat autovac workers the same as user backends for this * purpose. However, bgworkers are excluded from this test; we expect * bgworker_should_start_now() decided whether the DB state allows them. @@ -2452,7 +2467,9 @@ canAcceptConnections(int backend_type) { if (pmState == PM_WAIT_BACKUP) result = CAC_WAITBACKUP; /* allow superusers only */ - else if (Shutdown > NoShutdown) + else if (StepDown == SmartDemote || StepDown == FastDemote) + return CAC_DEMOTE; /* demote is pending */ + else if (StepDown > NoShutdown) return CAC_SHUTDOWN; /* shutdown is pending */ else if (!FatalError && (pmState == PM_STARTUP || @@ -2683,7 +2700,8 @@ SIGHUP_handler(SIGNAL_ARGS) PG_SETMASK(&BlockSig); #endif - if (Shutdown <= SmartShutdown) + if (StepDown == NoShutdown || StepDown == SmartShutdown || + StepDown == SmartDemote) { ereport(LOG, (errmsg("received SIGHUP, reloading configuration files"))); @@ -2769,26 +2787,72 @@ pmdie(SIGNAL_ARGS) (errmsg_internal("postmaster received signal %d", postgres_signal_arg))); + if (CheckDemoteSignal()) + { + if (pmState != PM_RUN) + { + DemoteSignal = false; + unlink(DEMOTE_SIGNAL_FILE); + ereport(LOG, + (errmsg("ignoring demote signal because already in standby mode"))); + } + else if (postgres_signal_arg == SIGQUIT) { + DemoteSignal = false; + ereport(WARNING, + (errmsg("can not demote in immediate stop mode"))); + // FIXME: should we abort the shutdown process? + } + else + { + FILE *standby_file; + + DemoteSignal = true; + + /* create the standby signal file */ + standby_file = AllocateFile(STANDBY_SIGNAL_FILE, "w"); + if (!standby_file) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not create file \"%s\": %m", + STANDBY_SIGNAL_FILE))); + + if (FreeFile(standby_file)) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not write file \"%s\": %m", + STANDBY_SIGNAL_FILE))); + } + + unlink(DEMOTE_SIGNAL_FILE); + } + switch (postgres_signal_arg) { case SIGTERM: /* - * Smart Shutdown: + * Smart Stepdown: * - * Wait for children to end their work, then shut down. + * Wait for children to end their work, then shut down or demote. */ - if (Shutdown >= SmartShutdown) + if (StepDown >= SmartShutdown) break; - Shutdown = SmartShutdown; - ereport(LOG, - (errmsg("received smart shutdown request"))); - /* Report status */ - AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_STOPPING); + if (DemoteSignal) { + StepDown = SmartDemote; + ereport(LOG, (errmsg("received smart demote request"))); + /* Report status */ + AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_DEMOTING); + } + else { + StepDown = SmartShutdown; + ereport(LOG, (errmsg("received smart shutdown request"))); + /* Report status */ + AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_STOPPING); #ifdef USE_SYSTEMD - sd_notify(0, "STOPPING=1"); + sd_notify(0, "STOPPING=1"); #endif + } if (pmState == PM_RUN || pmState == PM_RECOVERY || pmState == PM_HOT_STANDBY || pmState == PM_STARTUP) @@ -2831,22 +2895,29 @@ pmdie(SIGNAL_ARGS) case SIGINT: /* - * Fast Shutdown: + * Fast StepDown: * * Abort all children with SIGTERM (rollback active transactions - * and exit) and shut down when they are gone. + * and exit) and shut down or demote when they are gone. */ - if (Shutdown >= FastShutdown) + if (StepDown >= FastShutdown) break; - Shutdown = FastShutdown; - ereport(LOG, - (errmsg("received fast shutdown request"))); - /* Report status */ - AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_STOPPING); + if (DemoteSignal) { + StepDown = FastDemote; + ereport(LOG, (errmsg("received fast demote request"))); + /* Report status */ + AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_DEMOTING); + } + else { + StepDown = FastShutdown; + ereport(LOG, (errmsg("received fast shutdown request"))); + /* Report status */ + AddToDataDirLockFile(LOCK_FILE_LINE_PM_STATUS, PM_STATUS_STOPPING); #ifdef USE_SYSTEMD - sd_notify(0, "STOPPING=1"); + sd_notify(0, "STOPPING=1"); #endif + } if (StartupPID != 0) signal_child(StartupPID, SIGTERM); @@ -2903,9 +2974,9 @@ pmdie(SIGNAL_ARGS) * terminate remaining ones with SIGKILL, then exit without * attempt to properly shut down the data base system. */ - if (Shutdown >= ImmediateShutdown) + if (StepDown >= ImmediateShutdown) break; - Shutdown = ImmediateShutdown; + StepDown = ImmediateShutdown; ereport(LOG, (errmsg("received immediate shutdown request"))); @@ -2967,10 +3038,11 @@ reaper(SIGNAL_ARGS) StartupPID = 0; /* - * Startup process exited in response to a shutdown request (or it - * completed normally regardless of the shutdown request). + * Startup process exited in response to a shutdown or demote + * request (or it completed normally regardless of the shutdown + * request). */ - if (Shutdown > NoShutdown && + if (StepDown > NoShutdown && (EXIT_STATUS_0(exitstatus) || EXIT_STATUS_1(exitstatus))) { StartupStatus = STARTUP_NOT_RUNNING; @@ -2984,7 +3056,7 @@ reaper(SIGNAL_ARGS) ereport(LOG, (errmsg("shutdown at recovery target"))); StartupStatus = STARTUP_NOT_RUNNING; - Shutdown = SmartShutdown; + StepDown = SmartShutdown; TerminateChildren(SIGTERM); pmState = PM_WAIT_BACKENDS; /* PostmasterStateMachine logic does the rest */ @@ -3124,7 +3196,7 @@ reaper(SIGNAL_ARGS) * archive cycle and quit. Likewise, if we have walsender * processes, tell them to send any remaining WAL and quit. */ - Assert(Shutdown > NoShutdown); + Assert(StepDown > NoShutdown); /* Waken archiver for the last time */ if (PgArchPID != 0) @@ -3484,7 +3556,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) * signaled children, nonzero exit status is to be expected, so don't * clutter log. */ - take_action = !FatalError && Shutdown != ImmediateShutdown; + take_action = !FatalError && StepDown != ImmediateShutdown; if (take_action) { @@ -3702,7 +3774,7 @@ HandleChildCrash(int pid, int exitstatus, const char *procname) /* We do NOT restart the syslogger */ - if (Shutdown != ImmediateShutdown) + if (StepDown != ImmediateShutdown) FatalError = true; /* We now transit into a state of waiting for children to die */ @@ -3845,11 +3917,11 @@ PostmasterStateMachine(void) WalReceiverPID == 0 && BgWriterPID == 0 && (CheckpointerPID == 0 || - (!FatalError && Shutdown < ImmediateShutdown)) && + (!FatalError && StepDown < ImmediateShutdown)) && WalWriterPID == 0 && AutoVacPID == 0) { - if (Shutdown >= ImmediateShutdown || FatalError) + if (StepDown >= ImmediateShutdown || FatalError) { /* * Start waiting for dead_end children to die. This state @@ -3870,7 +3942,7 @@ PostmasterStateMachine(void) * the regular children are gone, and it's time to tell the * checkpointer to do a shutdown checkpoint. */ - Assert(Shutdown > NoShutdown); + Assert(StepDown > NoShutdown); /* Start the checkpointer if not running */ if (CheckpointerPID == 0) CheckpointerPID = StartCheckpointer(); @@ -3958,7 +4030,8 @@ PostmasterStateMachine(void) * EOF on its input pipe, which happens when there are no more upstream * processes. */ - if (Shutdown > NoShutdown && pmState == PM_NO_CHILDREN) + if (pmState == PM_NO_CHILDREN && (StepDown == SmartShutdown || + StepDown == FastShutdown || StepDown == ImmediateShutdown)) { if (FatalError) { @@ -3991,15 +4064,29 @@ PostmasterStateMachine(void) * startup process fails, because more than likely it will just fail again * and we will keep trying forever. */ - if (pmState == PM_NO_CHILDREN && + if (pmState == PM_NO_CHILDREN && !DemoteSignal && (StartupStatus == STARTUP_CRASHED || !restart_after_crash)) ExitPostmaster(1); + /* Handle demote signal */ + if (DemoteSignal && pmState == PM_NO_CHILDREN) + { + ereport(LOG, (errmsg("all server processes terminated; demoting"))); + + // Signal bgworkers? + + StartupPID = StartupDataBase(); + Assert(StartupPID != 0); + StartupStatus = STARTUP_RUNNING; + pmState = PM_STARTUP; + StepDown = NoShutdown; + } + /* * If we need to recover from a crash, wait for all non-syslogger children * to exit, then reset shmem and StartupDataBase. */ - if (FatalError && pmState == PM_NO_CHILDREN) + else if (FatalError && pmState == PM_NO_CHILDREN) { ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); @@ -5195,7 +5282,7 @@ sigusr1_handler(SIGNAL_ARGS) * first. We don't want to go back to recovery in that case. */ if (CheckPostmasterSignal(PMSIGNAL_RECOVERY_STARTED) && - pmState == PM_STARTUP && Shutdown == NoShutdown) + pmState == PM_STARTUP && StepDown == NoShutdown) { /* WAL redo has started. We're out of reinitialization. */ FatalError = false; @@ -5234,7 +5321,7 @@ sigusr1_handler(SIGNAL_ARGS) pmState = PM_RECOVERY; } if (CheckPostmasterSignal(PMSIGNAL_BEGIN_HOT_STANDBY) && - pmState == PM_RECOVERY && Shutdown == NoShutdown) + pmState == PM_RECOVERY && StepDown == NoShutdown) { /* * Likewise, start other special children as needed. @@ -5284,7 +5371,7 @@ sigusr1_handler(SIGNAL_ARGS) } if (CheckPostmasterSignal(PMSIGNAL_START_AUTOVAC_LAUNCHER) && - Shutdown == NoShutdown) + StepDown == NoShutdown) { /* * Start one iteration of the autovacuum daemon, even if autovacuuming @@ -5299,7 +5386,7 @@ sigusr1_handler(SIGNAL_ARGS) } if (CheckPostmasterSignal(PMSIGNAL_START_AUTOVAC_WORKER) && - Shutdown == NoShutdown) + StepDown == NoShutdown) { /* The autovacuum launcher wants us to start a worker process. */ StartAutovacuumWorker(); @@ -5644,7 +5731,7 @@ MaybeStartWalReceiver(void) if (WalReceiverPID == 0 && (pmState == PM_STARTUP || pmState == PM_RECOVERY || pmState == PM_HOT_STANDBY || pmState == PM_WAIT_READONLY) && - Shutdown == NoShutdown) + StepDown == NoShutdown) { WalReceiverPID = StartWalReceiver(); if (WalReceiverPID != 0) @@ -6647,3 +6734,18 @@ InitPostmasterDeathWatchHandle(void) GetLastError()))); #endif /* WIN32 */ } + +/* + * Check if a promote request appeared. Should be called by postmaster before + * shutting down. + */ +bool +CheckDemoteSignal(void) +{ + struct stat stat_buf; + + if (stat(DEMOTE_SIGNAL_FILE, &stat_buf) == 0) + return true; + + return false; +} diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c index e73639df74..c144cc35d3 100644 --- a/src/bin/pg_controldata/pg_controldata.c +++ b/src/bin/pg_controldata/pg_controldata.c @@ -57,6 +57,8 @@ dbState(DBState state) return _("shut down"); case DB_SHUTDOWNED_IN_RECOVERY: return _("shut down in recovery"); + case DB_DEMOTING: + return _("demoting"); case DB_SHUTDOWNING: return _("shutting down"); case DB_IN_CRASH_RECOVERY: diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 3c03ace7ed..0bb7d69682 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -62,6 +62,7 @@ typedef enum RESTART_COMMAND, RELOAD_COMMAND, STATUS_COMMAND, + DEMOTE_COMMAND, PROMOTE_COMMAND, LOGROTATE_COMMAND, KILL_COMMAND, @@ -103,6 +104,7 @@ static char version_file[MAXPGPATH]; static char pid_file[MAXPGPATH]; static char backup_file[MAXPGPATH]; static char promote_file[MAXPGPATH]; +static char demote_file[MAXPGPATH]; static char logrotate_file[MAXPGPATH]; static volatile pgpid_t postmasterPID = -1; @@ -129,6 +131,7 @@ static void do_stop(void); static void do_restart(void); static void do_reload(void); static void do_status(void); +static void do_demote(void); static void do_promote(void); static void do_logrotate(void); static void do_kill(pgpid_t pid); @@ -1029,6 +1032,103 @@ do_stop(void) } +static void +do_demote(void) +{ + int cnt; + FILE *dmtfile; + pgpid_t pid; + struct stat statbuf; + + pid = get_pgpid(false); + + if (pid == 0) /* no pid file */ + { + write_stderr(_("%s: PID file \"%s\" does not exist\n"), progname, pid_file); + write_stderr(_("Is server running?\n")); + exit(1); + } + else if (pid < 0) /* standalone backend, not postmaster */ + { + pid = -pid; + write_stderr(_("%s: cannot demote server; " + "single-user server is running (PID: %ld)\n"), + progname, pid); + exit(1); + } + + snprintf(demote_file, MAXPGPATH, "%s/demote", pg_data); + + if ((dmtfile = fopen(demote_file, "w")) == NULL) + { + write_stderr(_("%s: could not create demote signal file \"%s\": %s\n"), + progname, demote_file, strerror(errno)); + exit(1); + } + if (fclose(dmtfile)) + { + write_stderr(_("%s: could not write demote signal file \"%s\": %s\n"), + progname, demote_file, strerror(errno)); + exit(1); + } + + if (kill((pid_t) pid, sig) != 0) + { + write_stderr(_("%s: could not send stop signal (PID: %ld): %s\n"), progname, pid, + strerror(errno)); + exit(1); + } + + if (!do_wait) + { + print_msg(_("server demoting\n")); + return; + } + else + { + /* + * If backup_label exists, an online backup is running. Warn the user + * that smart demote will wait for it to finish. However, if the + * server is in archive recovery, we're recovering from an online + * backup instead of performing one. + */ + if (shutdown_mode == SMART_MODE && + stat(backup_file, &statbuf) == 0 && + get_control_dbstate() != DB_IN_ARCHIVE_RECOVERY) + { + print_msg(_("WARNING: online backup mode is active\n" + "Demote will not complete until pg_stop_backup() is called.\n\n")); + } + + print_msg(_("waiting for server to demote...")); + + for (cnt = 0; cnt < wait_seconds * WAITS_PER_SEC; cnt++) + { + if (get_control_dbstate() == DB_IN_ARCHIVE_RECOVERY) + break; + + if (cnt % WAITS_PER_SEC == 0) + print_msg("."); + pg_usleep(USEC_PER_SEC / WAITS_PER_SEC); + } + + if (get_control_dbstate() != DB_IN_ARCHIVE_RECOVERY) + { + print_msg(_(" failed\n")); + + write_stderr(_("%s: server does not demote\n"), progname); + if (shutdown_mode == SMART_MODE) + write_stderr(_("HINT: The \"-m fast\" option immediately disconnects sessions rather than\n" + "waiting for session-initiated disconnection.\n")); + exit(1); + } + print_msg(_(" done\n")); + + print_msg(_("server demoted\n")); + } +} + + /* * restart/reload routines */ @@ -2452,6 +2552,8 @@ main(int argc, char **argv) ctl_command = RELOAD_COMMAND; else if (strcmp(argv[optind], "status") == 0) ctl_command = STATUS_COMMAND; + else if (strcmp(argv[optind], "demote") == 0) + ctl_command = DEMOTE_COMMAND; else if (strcmp(argv[optind], "promote") == 0) ctl_command = PROMOTE_COMMAND; else if (strcmp(argv[optind], "logrotate") == 0) @@ -2559,6 +2661,9 @@ main(int argc, char **argv) case RELOAD_COMMAND: do_reload(); break; + case DEMOTE_COMMAND: + do_demote(); + break; case PROMOTE_COMMAND: do_promote(); break; diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h index de5670e538..f529f8c7bd 100644 --- a/src/include/catalog/pg_control.h +++ b/src/include/catalog/pg_control.h @@ -87,6 +87,7 @@ typedef enum DBState DB_STARTUP = 0, DB_SHUTDOWNED, DB_SHUTDOWNED_IN_RECOVERY, + DB_DEMOTING, DB_SHUTDOWNING, DB_IN_CRASH_RECOVERY, DB_IN_ARCHIVE_RECOVERY, diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 179ebaa104..a9e27f009e 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -70,7 +70,12 @@ typedef struct typedef enum CAC_state { - CAC_OK, CAC_STARTUP, CAC_SHUTDOWN, CAC_RECOVERY, CAC_TOOMANY, + CAC_OK, + CAC_STARTUP, + CAC_DEMOTE, + CAC_SHUTDOWN, + CAC_RECOVERY, + CAC_TOOMANY, CAC_WAITBACKUP } CAC_state; diff --git a/src/include/utils/pidfile.h b/src/include/utils/pidfile.h index 63fefe5c4c..f761d2c4ef 100644 --- a/src/include/utils/pidfile.h +++ b/src/include/utils/pidfile.h @@ -50,6 +50,7 @@ */ #define PM_STATUS_STARTING "starting" /* still starting up */ #define PM_STATUS_STOPPING "stopping" /* in shutdown sequence */ +#define PM_STATUS_DEMOTING "demoting" /* demote sequence */ #define PM_STATUS_READY "ready " /* ready for connections */ #define PM_STATUS_STANDBY "standby " /* up, won't accept connections */ -- 2.20.1 --MP_/9v.jdvlK+JLUeXFsf_NviJr-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-19 07:51 Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-02-19 07:51 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Ashutosh Bapat <[email protected]> On Mon, Feb 19, 2024 at 03:01:40PM +0900, Michael Paquier wrote: > 0002 is a polished version of the TAP test that makes use of this > facility, providing coverage for the bug fixed by 7863ee4def65 > (reverting this commit causes the test to fail), where a restart point > runs across a promotion request. The trick is to stop the > checkpointer in the middle of a restart point and issue a promotion > in-between. The CF bot has been screaming at this one on Windows because the process started with IPC::Run::start was not properly finished, so attached is an updated version to bring that back to green. -- Michael Attachments: [text/x-diff] v2-0001-injection_points-Add-routines-to-wait-and-wake-pr.patch (5.8K, ../../[email protected]/2-v2-0001-injection_points-Add-routines-to-wait-and-wake-pr.patch) download | inline diff: From ef27d6eba619f5106915de6b05cbeb5294263c30 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 19 Feb 2024 14:35:55 +0900 Subject: [PATCH v2 1/2] injection_points: Add routines to wait and wake processes This commit is made of two parts: - A new callback that can be attached to a process to make it wait on a condition variable. The condition checked is registered in shared memory by the module injection_points. - A new SQL function to update the shared state and broadcast the update using a condition variable. The shared state used by the module is registered using the DSM registry, and is optional. --- .../injection_points--1.0.sql | 10 ++ .../injection_points/injection_points.c | 104 ++++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 3 files changed, 115 insertions(+) diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql index 5944c41716..20479991f2 100644 --- a/src/test/modules/injection_points/injection_points--1.0.sql +++ b/src/test/modules/injection_points/injection_points--1.0.sql @@ -24,6 +24,16 @@ RETURNS void AS 'MODULE_PATHNAME', 'injection_points_run' LANGUAGE C STRICT PARALLEL UNSAFE; +-- +-- injection_points_wake() +-- +-- Wakes a condition variable waited on in an injection point. +-- +CREATE FUNCTION injection_points_wake() +RETURNS void +AS 'MODULE_PATHNAME', 'injection_points_wake' +LANGUAGE C STRICT PARALLEL UNSAFE; + -- -- injection_points_detach() -- diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index e843e6594f..3a319b1525 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -18,18 +18,67 @@ #include "postgres.h" #include "fmgr.h" +#include "storage/condition_variable.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/dsm_registry.h" #include "utils/builtins.h" #include "utils/injection_point.h" #include "utils/wait_event.h" PG_MODULE_MAGIC; +/* Shared state information for injection points. */ +typedef struct InjectionPointSharedState +{ + /* protects accesses to wait_counts */ + slock_t lock; + + /* Counter advancing when injection_points_wake() is called */ + int wait_counts; + + /* + * Condition variable that can be used in an injection point, checking + * upon wait_counts when waiting. + */ + ConditionVariable wait_point; +} InjectionPointSharedState; + +/* Pointer to shared-memory state. */ +static InjectionPointSharedState *inj_state = NULL; + +/* Wait event when waiting on condition variable */ +static uint32 injection_wait_event = 0; + extern PGDLLEXPORT void injection_error(const char *name); extern PGDLLEXPORT void injection_notice(const char *name); +extern PGDLLEXPORT void injection_wait(const char *name); +static void +injection_point_init_state(void *ptr) +{ + InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; + + state->wait_counts = 0; + SpinLockInit(&state->lock); + ConditionVariableInit(&state->wait_point); +} + +static void +injection_init_shmem(void) +{ + bool found; + + if (inj_state != NULL) + return; + + inj_state = GetNamedDSMSegment("injection_points", + sizeof(InjectionPointSharedState), + injection_point_init_state, + &found); +} + /* Set of callbacks available to be attached to an injection point. */ void injection_error(const char *name) @@ -43,6 +92,38 @@ injection_notice(const char *name) elog(NOTICE, "notice triggered for injection point %s", name); } +/* Wait on a condition variable, awaken by injection_points_wake() */ +void +injection_wait(const char *name) +{ + int old_wait_counts; + + if (inj_state == NULL) + injection_init_shmem(); + if (injection_wait_event == 0) + injection_wait_event = WaitEventExtensionNew("injection_wait"); + + SpinLockAcquire(&inj_state->lock); + old_wait_counts = inj_state->wait_counts; + SpinLockRelease(&inj_state->lock); + + /* And sleep.. */ + ConditionVariablePrepareToSleep(&inj_state->wait_point); + for (;;) + { + int new_wait_counts; + + SpinLockAcquire(&inj_state->lock); + new_wait_counts = inj_state->wait_counts; + SpinLockRelease(&inj_state->lock); + + if (old_wait_counts != new_wait_counts) + break; + ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + } + ConditionVariableCancelSleep(); +} + /* * SQL function for creating an injection point. */ @@ -58,6 +139,8 @@ injection_points_attach(PG_FUNCTION_ARGS) function = "injection_error"; else if (strcmp(action, "notice") == 0) function = "injection_notice"; + else if (strcmp(action, "wait") == 0) + function = "injection_wait"; else elog(ERROR, "incorrect action \"%s\" for injection point creation", action); @@ -80,6 +163,27 @@ injection_points_run(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +/* + * SQL function for waking a condition variable. + */ +PG_FUNCTION_INFO_V1(injection_points_wake); +Datum +injection_points_wake(PG_FUNCTION_ARGS) +{ + + if (inj_state == NULL) + injection_init_shmem(); + + /* First bump the wait counter */ + SpinLockAcquire(&inj_state->lock); + inj_state->wait_counts++; + SpinLockRelease(&inj_state->lock); + + /* And broadcast the change for the waiters */ + ConditionVariableBroadcast(&inj_state->wait_point); + PG_RETURN_VOID(); +} + /* * SQL function for dropping an injection point. */ diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index d808aad8b0..d7eca00502 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1208,6 +1208,7 @@ InitializeDSMForeignScan_function InitializeWorkerForeignScan_function InjectionPointCacheEntry InjectionPointEntry +InjectionPointSharedState InlineCodeBlock InsertStmt Instrumentation -- 2.43.0 [text/x-diff] v2-0002-Add-regression-test-for-restart-points-during-pro.patch (9.1K, ../../[email protected]/3-v2-0002-Add-regression-test-for-restart-points-during-pro.patch) download | inline diff: From c64634c5d1826dab6012d8055b494d807ebc6be7 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 19 Feb 2024 16:49:36 +0900 Subject: [PATCH v2 2/2] Add regression test for restart points during promotion This test fails when 7863ee4def65 is reverted, checking that a node is able to properly restart following a crash when a restart point was finishing across a promotion. This is an old bug that had no coverage, and injection points make that cheap to achieve. --- src/backend/access/transam/xlog.c | 7 + src/test/recovery/Makefile | 7 +- src/test/recovery/meson.build | 4 + .../t/041_invalid_checkpoint_after_promote.pl | 175 ++++++++++++++++++ 4 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/041_invalid_checkpoint_after_promote.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 50c347a679..50b045ff08 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -100,6 +100,7 @@ #include "storage/sync.h" #include "utils/guc_hooks.h" #include "utils/guc_tables.h" +#include "utils/injection_point.h" #include "utils/memutils.h" #include "utils/ps_status.h" #include "utils/relmapper.h" @@ -7536,6 +7537,12 @@ CreateRestartPoint(int flags) CheckPointGuts(lastCheckPoint.redo, flags); + /* + * This location needs to be after CheckPointGuts() to ensure that some + * work has already happened during this checkpoint. + */ + INJECTION_POINT("CreateRestartPoint"); + /* * Remember the prior checkpoint's redo ptr for * UpdateCheckPointDistanceEstimate() diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 17ee353735..f57baba5e8 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -9,12 +9,17 @@ # #------------------------------------------------------------------------- -EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding +EXTRA_INSTALL=contrib/pg_prewarm \ + contrib/pg_stat_statements \ + contrib/test_decoding \ + src/test/modules/injection_points subdir = src/test/recovery top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +export enable_injection_points enable_injection_points + # required for 017_shm.pl and 027_stream_regress.pl REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) export REGRESS_SHLIB diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index bf087ac2a9..e4e0e2b4cc 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -6,6 +6,9 @@ tests += { 'bd': meson.current_build_dir(), 'tap': { 'test_kwargs': {'priority': 40}, # recovery tests are slow, start early + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, 'tests': [ 't/001_stream_rep.pl', 't/002_archiving.pl', @@ -46,6 +49,7 @@ tests += { 't/038_save_logical_slots_shutdown.pl', 't/039_end_of_wal.pl', 't/040_standby_failover_slots_sync.pl', + 't/041_invalid_checkpoint_after_promote.pl', ], }, } diff --git a/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl new file mode 100644 index 0000000000..e4812d569d --- /dev/null +++ b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl @@ -0,0 +1,175 @@ + +# Copyright (c) 2024, PostgreSQL Global Development Group + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Time::HiRes qw(usleep); +use Test::More; + +################################################## +# Test race condition when a restart point is running during a promotion, +# checking that WAL segments are correctly removed in the restart point +# while the promotion finishes. +# +# This test relies on an injection point that causes the checkpointer to +# wait in the middle of a restart point on a standby. The checkpointer +# is awaken to finish its restart point only once the promotion of the +# standby is completed, and the node should be able to restart properly. +################################################## + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Initialize primary node +my $node_primary = PostgreSQL::Test::Cluster->new('master'); +$node_primary->init(allows_streaming => 1); +$node_primary->append_conf( + 'postgresql.conf', q[ +checkpoint_timeout = 30s +log_checkpoints = on +restart_after_crash = on +]); +$node_primary->start; + +my $backup_name = 'my_backup'; +$node_primary->backup($backup_name); + +# Setup a standby +my $node_standby = PostgreSQL::Test::Cluster->new('standby1'); +$node_standby->init_from_backup($node_primary, $backup_name, + has_streaming => 1); +$node_standby->start; + +# Dummy table for the upcoming tests. +$node_primary->safe_psql('postgres', 'checkpoint'); +$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);'); + +# Register a injection point on the standby so as the follow-up +# restart point will wait on it. +$node_primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); +# Wait until the extension has been created on the standby +$node_primary->wait_for_replay_catchup($node_standby); + +# Note that from this point the checkpointer will wait in the middle of +# a restart point on the standby. +$node_standby->safe_psql('postgres', + "SELECT injection_points_attach('CreateRestartPoint', 'wait');"); + +# Execute a restart point on the standby, that we will now be waiting on. +# This needs to be in the background. +my $logstart = -s $node_standby->logfile; +my $psql_session = + $node_standby->background_psql('postgres', on_error_stop => 0); +$psql_session->query_until( + qr/starting_checkpoint/, q( + \echo starting_checkpoint + CHECKPOINT; +)); + +# Switch one WAL segment to make the previous restart point remove the +# segment once the restart point completes. +$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);'); +$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();'); +$node_primary->wait_for_replay_catchup($node_standby); + +# Wait until the checkpointer is in the middle of the restart point +# processing, relying on the custom wait event generated in the +# wait callback used in the injection point previously attached. +ok( $node_standby->poll_query_until( + 'postgres', + qq[SELECT count(*) FROM pg_stat_activity + WHERE backend_type = 'checkpointer' AND wait_event = 'injection_wait' ;], + '1'), + 'checkpointer is waiting in restart point' +) or die "Timed out while waiting for checkpointer to run restart point"; + +# Check the logs that the restart point has started on standby. This is +# optional, but let's be sure. +my $log = slurp_file($node_standby->logfile, $logstart); +my $checkpoint_start = 0; +if ($log =~ m/restartpoint starting: immediate wait/) +{ + $checkpoint_start = 1; +} +is($checkpoint_start, 1, 'restartpoint has started'); + +# Trigger promotion during the restart point. +$node_primary->stop; +$node_standby->promote; + +# Update the start position before waking up the checkpointer! +$logstart = -s $node_standby->logfile; + +# Now wake up the checkpointer. +$node_standby->safe_psql('postgres', "SELECT injection_points_wake();"); + +# Wait until the previous restart point completes on the newly-promoted +# standby, checking the logs for that. +my $checkpoint_complete = 0; +for (my $i = 0; $i < 3000; $i++) +{ + my $log = slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint complete/) + { + $checkpoint_complete = 1; + last; + } + usleep(100_000); +} +is($checkpoint_complete, 1, 'restart point has completed'); + +# Kill with SIGKILL, forcing all the backends to restart. +my $psql_timeout = IPC::Run::timer(3600); +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node_standby->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/; +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Wait until the server restarts, finish consuming output. +$killme_stdin .= q[ +SELECT 1; +]; +ok( pump_until( + $killme, + $psql_timeout, + \$killme_stderr, + qr/server closed the connection unexpectedly|connection to server was lost|could not send data to server/m + ), + "psql query died successfully after SIGKILL"); +$killme->finish; + +# Wait till server finishes restarting +$node_standby->poll_query_until('postgres', undef, ''); + +# After recovery, the server should be able to start. +my $stdout; +my $stderr; +($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1'); +is($ret, 0, "psql connect success"); +is($stdout, 1, "psql select 1"); + +done_testing(); -- 2.43.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-19 14:28 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bertrand Drouvot @ 2024-02-19 14:28 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Ashutosh Bapat <[email protected]> Hi, On Mon, Feb 19, 2024 at 04:51:45PM +0900, Michael Paquier wrote: > On Mon, Feb 19, 2024 at 03:01:40PM +0900, Michael Paquier wrote: > > 0002 is a polished version of the TAP test that makes use of this > > facility, providing coverage for the bug fixed by 7863ee4def65 > > (reverting this commit causes the test to fail), where a restart point > > runs across a promotion request. The trick is to stop the > > checkpointer in the middle of a restart point and issue a promotion > > in-between. > > The CF bot has been screaming at this one on Windows because the > process started with IPC::Run::start was not properly finished, so > attached is an updated version to bring that back to green. Thanks for the patch, that's a very cool feature! Random comments: 1 === +CREATE FUNCTION injection_points_wake() what about injection_points_wakeup() instead? 2 === +/* Shared state information for injection points. */ +typedef struct InjectionPointSharedState +{ + /* protects accesses to wait_counts */ + slock_t lock; + + /* Counter advancing when injection_points_wake() is called */ + int wait_counts; If slock_t protects "only" one counter, then what about using pg_atomic_uint64 or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment number 4) 3 === + * SQL function for waking a condition variable waking up instead? 4 === + for (;;) + { + int new_wait_counts; + + SpinLockAcquire(&inj_state->lock); + new_wait_counts = inj_state->wait_counts; + SpinLockRelease(&inj_state->lock); + + if (old_wait_counts != new_wait_counts) + break; + ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + } I'm wondering if this loop and wait_counts are needed, why not doing something like (and completely get rid of wait_counts)? " ConditionVariablePrepareToSleep(&inj_state->wait_point); ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); ConditionVariableCancelSleep(); " It's true that the comment above ConditionVariableSleep() mentions that: " * This should be called in a predicate loop that tests for a specific exit * condition and otherwise sleeps " but is it needed in our particular case here? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-19 23:28 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-02-19 23:28 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Postgres hackers <[email protected]>; Ashutosh Bapat <[email protected]> On Mon, Feb 19, 2024 at 02:28:04PM +0000, Bertrand Drouvot wrote: > +CREATE FUNCTION injection_points_wake() > > what about injection_points_wakeup() instead? Sure. > +/* Shared state information for injection points. */ > +typedef struct InjectionPointSharedState > +{ > + /* protects accesses to wait_counts */ > + slock_t lock; > + > + /* Counter advancing when injection_points_wake() is called */ > + int wait_counts; > > If slock_t protects "only" one counter, then what about using pg_atomic_uint64 > or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment > number 4) We could, indeed, even if we use more than one counter. Still, I would be tempted to keep it in case more data is added to this structure as that would make for less stuff to do while being normally non-critical. This sentence may sound pedantic, though.. > + * SQL function for waking a condition variable > > waking up instead? Okay. > I'm wondering if this loop and wait_counts are needed, why not doing something > like (and completely get rid of wait_counts)? > > " > ConditionVariablePrepareToSleep(&inj_state->wait_point); > ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); > ConditionVariableCancelSleep(); > " > > It's true that the comment above ConditionVariableSleep() mentions that: Perhaps not, but it encourages good practices around the use of condition variables, and we need to track all that in shared memory anyway. Ashutosh has argued in favor of the approach taken by the patch in the original thread when I've sent a version doing exactly what you are saying now to not track a state in shmem. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-20 15:55 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bertrand Drouvot @ 2024-02-20 15:55 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Ashutosh Bapat <[email protected]> Hi, On Tue, Feb 20, 2024 at 08:28:28AM +0900, Michael Paquier wrote: > On Mon, Feb 19, 2024 at 02:28:04PM +0000, Bertrand Drouvot wrote: > > If slock_t protects "only" one counter, then what about using pg_atomic_uint64 > > or pg_atomic_uint32 instead? And btw do we need wait_counts at all? (see comment > > number 4) > > We could, indeed, even if we use more than one counter. Still, I > would be tempted to keep it in case more data is added to this > structure as that would make for less stuff to do while being normally > non-critical. This sentence may sound pedantic, though.. Okay, makes sense to keep this as it is as a "template" in case more stuff is added. + /* Counter advancing when injection_points_wake() is called */ + int wait_counts; In that case what about using an unsigned instead? (Nit) > > I'm wondering if this loop and wait_counts are needed, why not doing something > > like (and completely get rid of wait_counts)? > > > > " > > ConditionVariablePrepareToSleep(&inj_state->wait_point); > > ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); > > ConditionVariableCancelSleep(); > > " > > > > It's true that the comment above ConditionVariableSleep() mentions that: > > Perhaps not, but it encourages good practices around the use of > condition variables, and we need to track all that in shared memory > anyway. Ashutosh has argued in favor of the approach taken by the > patch in the original thread when I've sent a version doing exactly > what you are saying now to not track a state in shmem. Oh okay I missed this previous discussion, let's keep it as it is then. New comments: 1 === +void +injection_wait(const char *name) Looks like name is not used in the function. I guess the reason it is a parameter is because that's the way the callback function is being called in InjectionPointRun()? 2 === +PG_FUNCTION_INFO_V1(injection_points_wake); +Datum +injection_points_wake(PG_FUNCTION_ARGS) +{ I think that This function will wake up all the "wait" injection points. Would that make sense to implement some filtering based on the name? That could be useful for tests that would need multiple wait injection points and that want to wake them up "sequentially". We could think about it if there is such a need in the future though. 3 === +# Register a injection point on the standby so as the follow-up typo: "an injection"? 4 === +for (my $i = 0; $i < 3000; $i++) +{ is 3000 due to?: +checkpoint_timeout = 30s If so, would that make sense to reduce the value for both? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-20 22:08 Michael Paquier <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Michael Paquier @ 2024-02-20 22:08 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Postgres hackers <[email protected]>; Ashutosh Bapat <[email protected]> On Tue, Feb 20, 2024 at 03:55:08PM +0000, Bertrand Drouvot wrote: > Okay, makes sense to keep this as it is as a "template" in case more stuff is > added. > > + /* Counter advancing when injection_points_wake() is called */ > + int wait_counts; > > In that case what about using an unsigned instead? (Nit) uint32. Sure. > 1 === > > +void > +injection_wait(const char *name) > > Looks like name is not used in the function. I guess the reason it is a parameter > is because that's the way the callback function is being called in > InjectionPointRun()? Right. The callback has to define this argument. > 2 === > > +PG_FUNCTION_INFO_V1(injection_points_wake); > +Datum > +injection_points_wake(PG_FUNCTION_ARGS) > +{ > > I think that This function will wake up all the "wait" injection points. > Would that make sense to implement some filtering based on the name? That could > be useful for tests that would need multiple wait injection points and that want > to wake them up "sequentially". > > We could think about it if there is such a need in the future though. Well, both you and Andrey are asking for it now, so let's do it. The implementation is simple: - Store in InjectionPointSharedState an array of wait_counts and an array of names. There is only one condition variable. - When a point wants to wait, it takes the spinlock and looks within the array of names until it finds a free slot, adds its name into the array to reserve a wait counter at the same position, releases the spinlock. Then it loops on the condition variable for an update of the counter it has reserved. It is possible to make something more efficient, but at a small size it would not really matter. - The wakeup takes a point name in argument, acquires the spinlock, and checks if it can find the point into the array, pinpoints the location of the counter to update and updates it. Then it broadcasts the change. - The wait loop checks its counter, leaves its loop, cancels the sleep, takes the spinlock to unregister from the array, and leaves. I would just hardcode the number of points that can wait, say 5 of them tracked in shmem? Does that look like what you are looking at? > +# Register a injection point on the standby so as the follow-up > > typo: "an injection"? Oops. Fixed locally. > +for (my $i = 0; $i < 3000; $i++) > +{ > > is 3000 due to?: > > +checkpoint_timeout = 30s > > If so, would that make sense to reduce the value for both? That had better be based on PostgreSQL::Test::Utils::timeout_default, actually, as in something like: foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default) -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Injection points: some tools to wait and wake @ 2024-02-21 07:46 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Michael Paquier @ 2024-02-21 07:46 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Postgres hackers <[email protected]>; Ashutosh Bapat <[email protected]> On Wed, Feb 21, 2024 at 07:08:03AM +0900, Michael Paquier wrote: > Well, both you and Andrey are asking for it now, so let's do it. The > implementation is simple: > - Store in InjectionPointSharedState an array of wait_counts and an > array of names. There is only one condition variable. > - When a point wants to wait, it takes the spinlock and looks within > the array of names until it finds a free slot, adds its name into the > array to reserve a wait counter at the same position, releases the > spinlock. Then it loops on the condition variable for an update of > the counter it has reserved. It is possible to make something more > efficient, but at a small size it would not really matter. > - The wakeup takes a point name in argument, acquires the spinlock, > and checks if it can find the point into the array, pinpoints the > location of the counter to update and updates it. Then it broadcasts > the change. > - The wait loop checks its counter, leaves its loop, cancels the > sleep, takes the spinlock to unregister from the array, and leaves. > > I would just hardcode the number of points that can wait, say 5 of > them tracked in shmem? Does that look like what you are looking at? I was looking at that, and it proves to work OK, so you can do stuff like waits and wakeups for multiple processes in a controlled manner. The attached patch authorizes up to 32 waiters. I have switched things so as the information reported in pg_stat_activity is the name of the injection point itself. Comments and ideas are welcome. -- Michael Attachments: [text/x-diff] v2-0001-injection_points-Add-routines-to-wait-and-wake-pr.patch (7.1K, ../../[email protected]/2-v2-0001-injection_points-Add-routines-to-wait-and-wake-pr.patch) download | inline diff: From 75302cba302b83ce2a6d6eaf30b163f473b87276 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Wed, 21 Feb 2024 16:36:25 +0900 Subject: [PATCH v2 1/2] injection_points: Add routines to wait and wake processes This commit is made of two parts: - A new callback that can be attached to a process to make it wait on a condition variable. The condition checked is registered in shared memory by the module injection_points. - A new SQL function to update the shared state and broadcast the update using a condition variable. The shared state used by the module is registered using the DSM registry, and is optional. --- .../injection_points--1.0.sql | 10 ++ .../injection_points/injection_points.c | 151 ++++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 3 files changed, 162 insertions(+) diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql index 5944c41716..eed0310cf6 100644 --- a/src/test/modules/injection_points/injection_points--1.0.sql +++ b/src/test/modules/injection_points/injection_points--1.0.sql @@ -24,6 +24,16 @@ RETURNS void AS 'MODULE_PATHNAME', 'injection_points_run' LANGUAGE C STRICT PARALLEL UNSAFE; +-- +-- injection_points_wakeup() +-- +-- Wakes a condition variable waited on in an injection point. +-- +CREATE FUNCTION injection_points_wakeup(IN point_name TEXT) +RETURNS void +AS 'MODULE_PATHNAME', 'injection_points_wakeup' +LANGUAGE C STRICT PARALLEL UNSAFE; + -- -- injection_points_detach() -- diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index e843e6594f..052b20f9c8 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -18,18 +18,72 @@ #include "postgres.h" #include "fmgr.h" +#include "storage/condition_variable.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/dsm_registry.h" #include "utils/builtins.h" #include "utils/injection_point.h" #include "utils/wait_event.h" PG_MODULE_MAGIC; +/* Maximum number of wait usable in injection points at once */ +#define INJ_MAX_WAIT 32 +#define INJ_NAME_MAXLEN 64 + +/* Shared state information for injection points. */ +typedef struct InjectionPointSharedState +{ + /* protects accesses to wait_counts */ + slock_t lock; + + /* Counters advancing when injection_points_wakeup() is called */ + int wait_counts[INJ_MAX_WAIT]; + + /* Names of injection points attached to wait counters */ + char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; + + /* + * Condition variable used for waits and wakeups, checking upon the set of + * wait_counts when waiting. + */ + ConditionVariable wait_point; +} InjectionPointSharedState; + +/* Pointer to shared-memory state. */ +static InjectionPointSharedState *inj_state = NULL; + extern PGDLLEXPORT void injection_error(const char *name); extern PGDLLEXPORT void injection_notice(const char *name); +extern PGDLLEXPORT void injection_wait(const char *name); +static void +injection_point_init_state(void *ptr) +{ + InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; + + SpinLockInit(&state->lock); + memset(state->wait_counts, 0, sizeof(state->wait_counts)); + memset(state->name, 0, sizeof(state->name)); + ConditionVariableInit(&state->wait_point); +} + +static void +injection_init_shmem(void) +{ + bool found; + + if (inj_state != NULL) + return; + + inj_state = GetNamedDSMSegment("injection_points", + sizeof(InjectionPointSharedState), + injection_point_init_state, + &found); +} + /* Set of callbacks available to be attached to an injection point. */ void injection_error(const char *name) @@ -43,6 +97,65 @@ injection_notice(const char *name) elog(NOTICE, "notice triggered for injection point %s", name); } +/* Wait on a condition variable, awaken by injection_points_wakeup() */ +void +injection_wait(const char *name) +{ + int old_wait_counts = -1; + int index = -1; + uint32 injection_wait_event = 0; + + if (inj_state == NULL) + injection_init_shmem(); + + /* + * This custom wait event name is not released, but we don't care much for + * testing as this will be short-lived. + */ + injection_wait_event = WaitEventExtensionNew(name); + + /* + * Find a free slot to wait for, and register this injection point's name. + */ + SpinLockAcquire(&inj_state->lock); + for (int i = 0; i < INJ_MAX_WAIT; i++) + { + if (inj_state->name[i][0] == '\0') + { + index = i; + strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); + old_wait_counts = inj_state->wait_counts[i]; + break; + } + } + SpinLockRelease(&inj_state->lock); + + if (index < 0) + elog(ERROR, "could not find free slot for wait of injection point %s ", + name); + + /* And sleep.. */ + ConditionVariablePrepareToSleep(&inj_state->wait_point); + for (;;) + { + int new_wait_counts; + + SpinLockAcquire(&inj_state->lock); + new_wait_counts = inj_state->wait_counts[index]; + SpinLockRelease(&inj_state->lock); + + if (old_wait_counts != new_wait_counts) + break; + ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + } + ConditionVariableCancelSleep(); + + /* Remove us from the waiting list */ + SpinLockAcquire(&inj_state->lock); + inj_state->name[index][0] = '\0'; + SpinLockRelease(&inj_state->lock); +} + /* * SQL function for creating an injection point. */ @@ -58,6 +171,8 @@ injection_points_attach(PG_FUNCTION_ARGS) function = "injection_error"; else if (strcmp(action, "notice") == 0) function = "injection_notice"; + else if (strcmp(action, "wait") == 0) + function = "injection_wait"; else elog(ERROR, "incorrect action \"%s\" for injection point creation", action); @@ -80,6 +195,42 @@ injection_points_run(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +/* + * SQL function for waking a condition variable. + */ +PG_FUNCTION_INFO_V1(injection_points_wakeup); +Datum +injection_points_wakeup(PG_FUNCTION_ARGS) +{ + char *name = text_to_cstring(PG_GETARG_TEXT_PP(0)); + int index = -1; + + if (inj_state == NULL) + injection_init_shmem(); + + /* First bump the wait counter for the injection point to wake */ + SpinLockAcquire(&inj_state->lock); + for (int i = 0; i < INJ_MAX_WAIT; i++) + { + if (strcmp(name, inj_state->name[i]) == 0) + { + index = i; + break; + } + } + if (index < 0) + { + SpinLockRelease(&inj_state->lock); + elog(ERROR, "could not find injection point %s to wake", name); + } + inj_state->wait_counts[index]++; + SpinLockRelease(&inj_state->lock); + + /* And broadcast the change for the waiters */ + ConditionVariableBroadcast(&inj_state->wait_point); + PG_RETURN_VOID(); +} + /* * SQL function for dropping an injection point. */ diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index d808aad8b0..d7eca00502 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1208,6 +1208,7 @@ InitializeDSMForeignScan_function InitializeWorkerForeignScan_function InjectionPointCacheEntry InjectionPointEntry +InjectionPointSharedState InlineCodeBlock InsertStmt Instrumentation -- 2.43.0 [text/x-diff] v2-0002-Add-regression-test-for-restart-points-during-pro.patch (9.1K, ../../[email protected]/3-v2-0002-Add-regression-test-for-restart-points-during-pro.patch) download | inline diff: From 79cc3e309c15bec19fec8c3cb2ac2e48bd42d7a2 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Wed, 21 Feb 2024 16:37:17 +0900 Subject: [PATCH v2 2/2] Add regression test for restart points during promotion This test fails when 7863ee4def65 is reverted, checking that a node is able to properly restart following a crash when a restart point was finishing across a promotion. This is an old bug that had no coverage, and injection points make that cheap to achieve. --- src/backend/access/transam/xlog.c | 7 + src/test/recovery/Makefile | 7 +- src/test/recovery/meson.build | 4 + .../t/041_invalid_checkpoint_after_promote.pl | 176 ++++++++++++++++++ 4 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/041_invalid_checkpoint_after_promote.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 50c347a679..50b045ff08 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -100,6 +100,7 @@ #include "storage/sync.h" #include "utils/guc_hooks.h" #include "utils/guc_tables.h" +#include "utils/injection_point.h" #include "utils/memutils.h" #include "utils/ps_status.h" #include "utils/relmapper.h" @@ -7536,6 +7537,12 @@ CreateRestartPoint(int flags) CheckPointGuts(lastCheckPoint.redo, flags); + /* + * This location needs to be after CheckPointGuts() to ensure that some + * work has already happened during this checkpoint. + */ + INJECTION_POINT("CreateRestartPoint"); + /* * Remember the prior checkpoint's redo ptr for * UpdateCheckPointDistanceEstimate() diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 17ee353735..f57baba5e8 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -9,12 +9,17 @@ # #------------------------------------------------------------------------- -EXTRA_INSTALL=contrib/pg_prewarm contrib/pg_stat_statements contrib/test_decoding +EXTRA_INSTALL=contrib/pg_prewarm \ + contrib/pg_stat_statements \ + contrib/test_decoding \ + src/test/modules/injection_points subdir = src/test/recovery top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +export enable_injection_points enable_injection_points + # required for 017_shm.pl and 027_stream_regress.pl REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX) export REGRESS_SHLIB diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index bf087ac2a9..e4e0e2b4cc 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -6,6 +6,9 @@ tests += { 'bd': meson.current_build_dir(), 'tap': { 'test_kwargs': {'priority': 40}, # recovery tests are slow, start early + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, 'tests': [ 't/001_stream_rep.pl', 't/002_archiving.pl', @@ -46,6 +49,7 @@ tests += { 't/038_save_logical_slots_shutdown.pl', 't/039_end_of_wal.pl', 't/040_standby_failover_slots_sync.pl', + 't/041_invalid_checkpoint_after_promote.pl', ], }, } diff --git a/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl new file mode 100644 index 0000000000..e91f360d12 --- /dev/null +++ b/src/test/recovery/t/041_invalid_checkpoint_after_promote.pl @@ -0,0 +1,176 @@ + +# Copyright (c) 2024, PostgreSQL Global Development Group + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Time::HiRes qw(usleep); +use Test::More; + +################################################## +# Test race condition when a restart point is running during a promotion, +# checking that WAL segments are correctly removed in the restart point +# while the promotion finishes. +# +# This test relies on an injection point that causes the checkpointer to +# wait in the middle of a restart point on a standby. The checkpointer +# is awaken to finish its restart point only once the promotion of the +# standby is completed, and the node should be able to restart properly. +################################################## + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Initialize primary node +my $node_primary = PostgreSQL::Test::Cluster->new('master'); +$node_primary->init(allows_streaming => 1); +$node_primary->append_conf( + 'postgresql.conf', q[ +checkpoint_timeout = 30s +log_checkpoints = on +restart_after_crash = on +]); +$node_primary->start; + +my $backup_name = 'my_backup'; +$node_primary->backup($backup_name); + +# Setup a standby +my $node_standby = PostgreSQL::Test::Cluster->new('standby1'); +$node_standby->init_from_backup($node_primary, $backup_name, + has_streaming => 1); +$node_standby->start; + +# Dummy table for the upcoming tests. +$node_primary->safe_psql('postgres', 'checkpoint'); +$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);'); + +# Register an injection point on the standby so as the follow-up +# restart point will wait on it. +$node_primary->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); +# Wait until the extension has been created on the standby +$node_primary->wait_for_replay_catchup($node_standby); + +# Note that from this point the checkpointer will wait in the middle of +# a restart point on the standby. +$node_standby->safe_psql('postgres', + "SELECT injection_points_attach('CreateRestartPoint', 'wait');"); + +# Execute a restart point on the standby, that we will now be waiting on. +# This needs to be in the background. +my $logstart = -s $node_standby->logfile; +my $psql_session = + $node_standby->background_psql('postgres', on_error_stop => 0); +$psql_session->query_until( + qr/starting_checkpoint/, q( + \echo starting_checkpoint + CHECKPOINT; +)); + +# Switch one WAL segment to make the previous restart point remove the +# segment once the restart point completes. +$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);'); +$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();'); +$node_primary->wait_for_replay_catchup($node_standby); + +# Wait until the checkpointer is in the middle of the restart point +# processing, relying on the custom wait event generated in the +# wait callback used in the injection point previously attached. +ok( $node_standby->poll_query_until( + 'postgres', + qq[SELECT count(*) FROM pg_stat_activity + WHERE backend_type = 'checkpointer' AND wait_event = 'CreateRestartPoint' ;], + '1'), + 'checkpointer is waiting in restart point' +) or die "Timed out while waiting for checkpointer to run restart point"; + +# Check the logs that the restart point has started on standby. This is +# optional, but let's be sure. +my $log = slurp_file($node_standby->logfile, $logstart); +my $checkpoint_start = 0; +if ($log =~ m/restartpoint starting: immediate wait/) +{ + $checkpoint_start = 1; +} +is($checkpoint_start, 1, 'restartpoint has started'); + +# Trigger promotion during the restart point. +$node_primary->stop; +$node_standby->promote; + +# Update the start position before waking up the checkpointer! +$logstart = -s $node_standby->logfile; + +# Now wake up the checkpointer. +$node_standby->safe_psql('postgres', + "SELECT injection_points_wakeup('CreateRestartPoint');"); + +# Wait until the previous restart point completes on the newly-promoted +# standby, checking the logs for that. +my $checkpoint_complete = 0; +foreach my $i (0 .. 10 * $PostgreSQL::Test::Utils::timeout_default) +{ + my $log = slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint complete/) + { + $checkpoint_complete = 1; + last; + } + usleep(100_000); +} +is($checkpoint_complete, 1, 'restart point has completed'); + +# Kill with SIGKILL, forcing all the backends to restart. +my $psql_timeout = IPC::Run::timer(3600); +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node_standby->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/; +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Wait until the server restarts, finish consuming output. +$killme_stdin .= q[ +SELECT 1; +]; +ok( pump_until( + $killme, + $psql_timeout, + \$killme_stderr, + qr/server closed the connection unexpectedly|connection to server was lost|could not send data to server/m + ), + "psql query died successfully after SIGKILL"); +$killme->finish; + +# Wait till server finishes restarting +$node_standby->poll_query_until('postgres', undef, ''); + +# After recovery, the server should be able to start. +my $stdout; +my $stderr; +($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1'); +is($ret, 0, "psql connect success"); +is($stdout, 1, "psql select 1"); + +done_testing(); -- 2.43.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-02-21 07:46 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-04-10 16:01 [PATCH] Demote PoC Jehan-Guillaume de Rorthais <[email protected]> 2024-02-19 07:51 Re: Injection points: some tools to wait and wake Michael Paquier <[email protected]> 2024-02-19 14:28 ` Re: Injection points: some tools to wait and wake Bertrand Drouvot <[email protected]> 2024-02-19 23:28 ` Re: Injection points: some tools to wait and wake Michael Paquier <[email protected]> 2024-02-20 15:55 ` Re: Injection points: some tools to wait and wake Bertrand Drouvot <[email protected]> 2024-02-20 22:08 ` Re: Injection points: some tools to wait and wake Michael Paquier <[email protected]> 2024-02-21 07:46 ` Re: Injection points: some tools to wait and wake 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