public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v50 3/7] Make archiver process an auxiliary process
10+ messages / 4 participants
[nested] [flat]
* [PATCH v50 3/7] Make archiver process an auxiliary process
@ 2020-03-13 07:59 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 10+ 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 | 7 +-
src/backend/bootstrap/bootstrap.c | 22 +--
src/backend/postmaster/pgarch.c | 174 +++--------------------
src/backend/postmaster/postmaster.c | 88 ++++++------
src/backend/storage/lmgr/proc.c | 1 +
src/include/miscadmin.h | 2 +
src/include/postmaster/pgarch.h | 10 +-
src/include/storage/pmsignal.h | 1 -
src/include/storage/proc.h | 10 +-
9 files changed, 83 insertions(+), 232 deletions(-)
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c
index 1c5a4f8b5a..8b66a30931 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -29,7 +29,8 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/lwlock.h"
-#include "storage/pmsignal.h"
+#include "storage/latch.h"
+#include "storage/proc.h"
/*
* Attempt to retrieve the specified file from off-line archival storage.
@@ -490,8 +491,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..e237cedaff 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,25 +79,17 @@
* 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;
/* ----------
* Local function forward declarations
* ----------
*/
-#ifdef EXEC_BACKEND
-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);
@@ -105,121 +98,9 @@ static bool pgarch_readyXlog(char *xlog);
static void pgarch_archiveDone(char *xlog);
-/* ------------------------------------------------------------
- * Public functions called from postmaster follow
- * ------------------------------------------------------------
- */
-
-/*
- * 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
- * ------------------------------------------------------------
- */
-
-
-#ifdef EXEC_BACKEND
-
-/*
- * pgarch_forkexec() -
- *
- * Format up the arglist for, then fork and exec, archive process
- */
-static pid_t
-pgarch_forkexec(void)
-{
- char *av[10];
- int ac = 0;
-
- av[ac++] = "postgres";
-
- av[ac++] = "--forkarch";
-
- av[ac++] = NULL; /* filled in by postmaster_forkexec */
-
- av[ac] = NULL;
- Assert(ac < lengthof(av));
-
- return postmaster_forkexec(ac, av);
-}
-#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,31 +112,24 @@ 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;
+ proc_exit(0);
}
/* SIGUSR2 signal handler for archiver process */
@@ -282,14 +156,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 +194,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 +216,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 edab95a19e..d9f8d82650 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)
@@ -1792,7 +1793,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)
@@ -3007,7 +3008,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();
@@ -3126,6 +3127,18 @@ reaper(SIGNAL_ARGS)
continue;
}
+ /*
+ * Was it the archiver? We treat it the same way to WAL receiver.
+ */
+ if (pid == PgArchPID)
+ {
+ PgArchPID = 0;
+ if (!EXIT_STATUS_0(exitstatus) && !EXIT_STATUS_1(exitstatus))
+ HandleChildCrash(pid, exitstatus,
+ _("archiver process"));
+ continue;
+ }
+
/*
* Was it the autovacuum launcher? Normal exit can be ignored; we'll
* start a new one at the next iteration of the postmaster's main
@@ -3141,24 +3154,6 @@ reaper(SIGNAL_ARGS)
continue;
}
- /*
- * 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.
- */
- if (pid == PgArchPID)
- {
- PgArchPID = 0;
- if (!EXIT_STATUS_0(exitstatus))
- LogChildExit(LOG, _("archiver process"),
- pid, exitstatus);
- if (PgArchStartupAllowed())
- PgArchPID = pgarch_start();
- continue;
- }
-
/*
* Was it the statistics collector? If so, just try to start a new
* one; no need to force reset of the rest of the system. (If fail,
@@ -3403,7 +3398,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.
@@ -3609,19 +3604,16 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
signal_child(AutoVacPID, (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
- * simplifies the state-machine logic in the case where a shutdown request
- * arrives during crash processing.)
- */
- if (PgArchPID != 0 && take_action)
+ /* 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",
- "SIGQUIT",
+ (SendStop ? "SIGSTOP" : "SIGQUIT"),
(int) PgArchPID)));
- signal_child(PgArchPID, SIGQUIT);
+ signal_child(PgArchPID, (SendStop ? SIGSTOP : SIGQUIT));
}
/*
@@ -3912,6 +3904,7 @@ PostmasterStateMachine(void)
Assert(CheckpointerPID == 0);
Assert(WalWriterPID == 0);
Assert(AutoVacPID == 0);
+ Assert(PgArchPID == 0);
/* syslogger is not considered here */
pmState = PM_NO_CHILDREN;
}
@@ -4989,6 +4982,19 @@ SubPostmasterMain(int argc, char *argv[])
AuxiliaryProcessMain(argc - 2, argv + 2); /* does not return */
}
+ if (strcmp(argv[1], "--forkarch") == 0)
+ {
+ /* Restore basic shared memory pointers */
+ InitShmemAccess(UsedShmemSegAddr);
+
+ /* Need a PGPROC to run CreateSharedMemoryAndSemaphores */
+ InitAuxiliaryProcess();
+
+ /* Attach process to shared data structures */
+ CreateSharedMemoryAndSemaphores();
+
+ PgArchiverMain(); /* does not return */
+ }
if (strcmp(argv[1], "--forkavlauncher") == 0)
{
/* Restore basic shared memory pointers */
@@ -5037,12 +5043,6 @@ SubPostmasterMain(int argc, char *argv[])
StartBackgroundWorker();
}
- if (strcmp(argv[1], "--forkarch") == 0)
- {
- /* Do not want to attach to shared memory */
-
- PgArchiverMain(argc, argv); /* does not return */
- }
if (strcmp(argv[1], "--forkcol") == 0)
{
/* Do not want to attach to shared memory */
@@ -5140,7 +5140,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
@@ -5194,16 +5194,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)
{
@@ -5445,6 +5435,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 897045ee27..bf5d6da89b 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/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..737ed69e57 100644
--- a/src/include/postmaster/pgarch.h
+++ b/src/include/postmaster/pgarch.h
@@ -26,14 +26,6 @@
#define MAX_XFN_CHARS 40
#define VALID_XFN_CHARS "0123456789ABCDEF.history.backup.partial"
-/* ----------
- * Functions called from postmaster
- * ----------
- */
-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 a777cb64a1..d73890b228 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -357,6 +357,8 @@ 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;
} PROC_HDR;
extern PGDLLIMPORT PROC_HDR *ProcGlobal;
@@ -370,11 +372,11 @@ extern PGPROC *PreparedXactProcs;
* We set aside some extra PGPROC structures for auxiliary processes,
* ie things that aren't full-fledged backends but need shmem access.
*
- * Background writer, checkpointer and WAL writer run during normal operation.
- * Startup process and WAL receiver also consume 2 slots, but WAL writer is
- * launched only after startup has exited, so we only need 4 slots.
+ * Background writer, checkpointer, WAL writer and archiver run during normal
+ * operation. Startup process and WAL receiver also consume 2 slots, but WAL
+ * writer is launched only after startup has exited, so we only need 5 slots.
*/
-#define NUM_AUXILIARY_PROCS 4
+#define NUM_AUXILIARY_PROCS 5
/* configurable options */
extern PGDLLIMPORT int DeadlockTimeout;
--
2.27.0
----Next_Part(Tue_Mar__9_18_29_34_2021_806)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v50-0004-Shared-memory-based-stats-collector.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
@ 2024-01-01 12:12 John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: John Naylor @ 2024-01-01 12:12 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Thu, Nov 30, 2023 at 12:15 AM Nathan Bossart
<[email protected]> wrote:
> I don't intend for this patch to be
> seriously considered until we have better support for detecting/compiling
> AVX2 instructions and a buildfarm machine that uses them.
That's completely understandable, yet I'm confused why there is a
commitfest entry for it marked "needs review".
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
@ 2024-01-02 16:11 ` Nathan Bossart <[email protected]>
2024-01-02 17:50 ` Re: add AVX2 support to simd.h Tom Lane <[email protected]>
2024-01-03 14:13 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-04 17:48 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
0 siblings, 3 replies; 10+ messages in thread
From: Nathan Bossart @ 2024-01-02 16:11 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Mon, Jan 01, 2024 at 07:12:26PM +0700, John Naylor wrote:
> On Thu, Nov 30, 2023 at 12:15 AM Nathan Bossart
> <[email protected]> wrote:
>> I don't intend for this patch to be
>> seriously considered until we have better support for detecting/compiling
>> AVX2 instructions and a buildfarm machine that uses them.
>
> That's completely understandable, yet I'm confused why there is a
> commitfest entry for it marked "needs review".
Perhaps I was too optimistic about adding support for newer instructions...
I'm tempted to propose that we move forward with this patch as-is after
adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.
There is likely still follow-up work to make these improvements more
accessible, but I'm not sure that is a strict prerequisite here.
(In case it isn't clear, I'm volunteering to set up such a buildfarm
machine.)
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
@ 2024-01-02 17:50 ` Tom Lane <[email protected]>
2024-01-02 22:00 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2024-01-02 17:50 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: John Naylor <[email protected]>; Ants Aasma <[email protected]>; pgsql-hackers
Nathan Bossart <[email protected]> writes:
> I'm tempted to propose that we move forward with this patch as-is after
> adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.
> There is likely still follow-up work to make these improvements more
> accessible, but I'm not sure that is a strict prerequisite here.
The patch needs better comments (as in, more than "none whatsoever").
It doesn't need to be much though, perhaps like
+#if defined(__AVX2__)
+
+/*
+ * When compiled with -mavx2 or allied options, we prefer AVX2 instructions.
+ */
+#include <immintrin.h>
+#define USE_AVX2
+typedef __m256i Vector8;
+typedef __m256i Vector32;
Also, do you really want to structure the header so that USE_SSE2
doesn't get defined? In that case you are committing to provide
an AVX2 replacement every single place that there's USE_SSE2, which
doesn't seem like a great thing to require. OTOH, maybe there's
no choice given than we need a different definition for Vector8 and
Vector32?
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-02 17:50 ` Re: add AVX2 support to simd.h Tom Lane <[email protected]>
@ 2024-01-02 22:00 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2024-01-02 22:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: John Naylor <[email protected]>; Ants Aasma <[email protected]>; pgsql-hackers
On Tue, Jan 02, 2024 at 12:50:04PM -0500, Tom Lane wrote:
> The patch needs better comments (as in, more than "none whatsoever").
Yes, will do.
> Also, do you really want to structure the header so that USE_SSE2
> doesn't get defined? In that case you are committing to provide
> an AVX2 replacement every single place that there's USE_SSE2, which
> doesn't seem like a great thing to require. OTOH, maybe there's
> no choice given than we need a different definition for Vector8 and
> Vector32?
Yeah, the precedent is to use these abstracted types elsewhere so that any
SIMD-related improvements aren't limited to one architecture. There are a
couple of places that do explicitly check for USE_NO_SIMD, though. Maybe
there's an eventual use-case for using SSE2 intrinsics even when you have
AVX2 support, but for now, ensuring we have an AVX2 replacement for
everything doesn't seem particularly burdensome.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
@ 2024-01-03 14:13 ` John Naylor <[email protected]>
2024-01-03 15:29 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2 siblings, 1 reply; 10+ messages in thread
From: John Naylor @ 2024-01-03 14:13 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Tue, Jan 2, 2024 at 11:11 PM Nathan Bossart <[email protected]> wrote:
>
> Perhaps I was too optimistic about adding support for newer instructions...
>
> I'm tempted to propose that we move forward with this patch as-is after
> adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.
That means that we would be on the hook to fix it if it breaks, even
though nothing uses it yet in a normal build. I have pending patches
that will break, or get broken by, this, so minus-many from me until
there is an availability story.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-03 14:13 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
@ 2024-01-03 15:29 ` Nathan Bossart <[email protected]>
2024-01-05 02:03 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Bossart @ 2024-01-03 15:29 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Wed, Jan 03, 2024 at 09:13:52PM +0700, John Naylor wrote:
> On Tue, Jan 2, 2024 at 11:11 PM Nathan Bossart <[email protected]> wrote:
>> I'm tempted to propose that we move forward with this patch as-is after
>> adding a buildfarm machine that compiles with -mavx2 or -march=x86-64-v3.
>
> That means that we would be on the hook to fix it if it breaks, even
> though nothing uses it yet in a normal build. I have pending patches
> that will break, or get broken by, this, so minus-many from me until
> there is an availability story.
How will this break your patches? Is it just a matter of adding more AVX2
support, or something else?
If the requirement is that normal builds use AVX2, then I fear we will be
waiting a long time. IIUC the current proposals (building multiple
binaries or adding a configuration option that maps to compiler flags)
would still be opt-in, and I'm not sure we can mandate AVX2 support for all
x86_64 builds anytime soon.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-03 14:13 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-03 15:29 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
@ 2024-01-05 02:03 ` John Naylor <[email protected]>
2024-01-05 17:04 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: John Naylor @ 2024-01-05 02:03 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Wed, Jan 3, 2024 at 10:29 PM Nathan Bossart <[email protected]> wrote:
> If the requirement is that normal builds use AVX2, then I fear we will be
> waiting a long time. IIUC the current proposals (building multiple
> binaries or adding a configuration option that maps to compiler flags)
> would still be opt-in,
If and when we get one of those, I would consider that a "normal"
build. Since there are no concrete proposals yet, I'm still waiting
for you to justify imposing an immediate maintenance cost for zero
benefit.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-03 14:13 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-03 15:29 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-05 02:03 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
@ 2024-01-05 17:04 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2024-01-05 17:04 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Fri, Jan 05, 2024 at 09:03:39AM +0700, John Naylor wrote:
> On Wed, Jan 3, 2024 at 10:29 PM Nathan Bossart <[email protected]> wrote:
>> If the requirement is that normal builds use AVX2, then I fear we will be
>> waiting a long time. IIUC the current proposals (building multiple
>> binaries or adding a configuration option that maps to compiler flags)
>> would still be opt-in,
>
> If and when we get one of those, I would consider that a "normal"
> build. Since there are no concrete proposals yet, I'm still waiting
> for you to justify imposing an immediate maintenance cost for zero
> benefit.
I've been thinking about the configuration option approach. ISTM that
would be the most feasible strategy, at least for v17. A couple things
come to mind:
* This option would simply map to existing compiler flags. We already have
ways to provide those (-Dc_args in meson, CFLAGS in autoconf). Perhaps
we'd want to provide our own shorthand for certain platforms (e.g., ARM),
but that will still just be shorthand for compiler flags.
* Such an option would itself generate some maintenance cost. That could
be worth it because it formalizes the Postgres support for those options,
but it's still one more thing to track.
Another related option could be to simply document that we have support for
some newer instructions that can be enabled by setting the aforementioned
compiler flags. That's perhaps a little less user-friendly, but it'd avoid
the duplication and possibly reduce the maintenance cost. I also wonder if
it'd help prevent confusion when CFLAGS and this extra option conflict.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: add AVX2 support to simd.h
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
@ 2024-01-04 17:48 ` Nathan Bossart <[email protected]>
2 siblings, 0 replies; 10+ messages in thread
From: Nathan Bossart @ 2024-01-04 17:48 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: Ants Aasma <[email protected]>; pgsql-hackers
On Tue, Jan 02, 2024 at 10:11:23AM -0600, Nathan Bossart wrote:
> (In case it isn't clear, I'm volunteering to set up such a buildfarm
> machine.)
I set up "akepa" to run with -march=x86-64-v3.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-01-05 17:04 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:59 [PATCH v50 3/7] Make archiver process an auxiliary process Kyotaro Horiguchi <[email protected]>
2024-01-01 12:12 Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-02 16:11 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-02 17:50 ` Re: add AVX2 support to simd.h Tom Lane <[email protected]>
2024-01-02 22:00 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-03 14:13 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-03 15:29 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-05 02:03 ` Re: add AVX2 support to simd.h John Naylor <[email protected]>
2024-01-05 17:04 ` Re: add AVX2 support to simd.h Nathan Bossart <[email protected]>
2024-01-04 17:48 ` Re: add AVX2 support to simd.h Nathan Bossart <[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