public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/2] doc review 4+ messages / 3 participants [nested] [flat]
* [PATCH 2/2] doc review @ 2021-03-03 22:36 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2021-03-03 22:36 UTC (permalink / raw) --- doc/src/sgml/libpq.sgml | 67 +++++++++++++------------- src/interfaces/libpq/fe-exec.c | 2 +- src/test/modules/test_libpq/pipeline.c | 6 +-- 3 files changed, 38 insertions(+), 37 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index a34ecbb144..e2865a218b 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -3130,8 +3130,9 @@ ExecStatusType PQresultStatus(const PGresult *res); <term><literal>PGRES_PIPELINE_SYNC</literal></term> <listitem> <para> - The <structname>PGresult</structname> represents a point in a - pipeline where a synchronization point has been established. + The <structname>PGresult</structname> represents a + synchronization point in pipeline mode, requested by + <xref linkend="libpq-PQsendPipeline"/>. This status occurs only when pipeline mode has been selected. </para> </listitem> @@ -3141,9 +3142,9 @@ ExecStatusType PQresultStatus(const PGresult *res); <term><literal>PGRES_PIPELINE_ABORTED</literal></term> <listitem> <para> - The <structname>PGresult</structname> represents a pipeline that's + The <structname>PGresult</structname> represents a pipeline that has received an error from the server. <function>PQgetResult</function> - must be called repeatedly, and it will return this status code, + must be called repeatedly, and each time it will return this status code until the end of the current pipeline, at which point it will return <literal>PGRES_PIPELINE_SYNC</literal> and normal processing can resume. @@ -4953,7 +4954,7 @@ int PQflush(PGconn *conn); <title>Using Pipeline Mode</title> <para> - To issue pipelines the application must switch a connection into pipeline mode. + To issue pipelines, the application must switch a connection into pipeline mode. Enter pipeline mode with <xref linkend="libpq-PQenterPipelineMode"/> or test whether pipeline mode is active with <xref linkend="libpq-PQpipelineStatus"/>. @@ -4975,7 +4976,7 @@ int PQflush(PGconn *conn); server will block trying to send results to the client from queries it has already processed. This only occurs when the client sends enough queries to fill both its output buffer and the server's receive - buffer before switching to processing input from the server, + buffer before it switches to processing input from the server, but it's hard to predict exactly when that will happen. </para> </footnote> @@ -5025,7 +5026,7 @@ int PQflush(PGconn *conn); <title>Processing Results</title> <para> - To process the result of one pipeline query, the application calls + To process the result of one query in a pipeline, the application calls <function>PQgetResult</function> repeatedly and handles each result until <function>PQgetResult</function> returns null. The result from the next query in the pipeline may then be retrieved using @@ -5057,10 +5058,10 @@ int PQflush(PGconn *conn); <type>PGresult</type> types <literal>PGRES_PIPELINE_SYNC</literal> and <literal>PGRES_PIPELINE_ABORTED</literal>. <literal>PGRES_PIPELINE_SYNC</literal> is reported exactly once for each - <function>PQsendPipeline</function> call at the corresponding point in - the result stream. + <function>PQsendPipeline</function> after retrieving results for all + queries in the pipeline. <literal>PGRES_PIPELINE_ABORTED</literal> is emitted in place of a normal - result stream result for the first error and all subsequent results + stream result for the first error and all subsequent results except <literal>PGRES_PIPELINE_SYNC</literal> and null; see <xref linkend="libpq-pipeline-errors"/>. </para> @@ -5075,7 +5076,8 @@ int PQflush(PGconn *conn); application about the query currently being processed (except that <function>PQgetResult</function> returns null to indicate that we start returning the results of next query). The application must keep track - of the order in which it sent queries and the expected results. + of the order in which it sent queries, to associate them with their + corresponding results. Applications will typically use a state machine or a FIFO queue for this. </para> @@ -5091,10 +5093,10 @@ int PQflush(PGconn *conn); </para> <para> - From the client perspective, after the client gets a - <literal>PGRES_FATAL_ERROR</literal> return from - <function>PQresultStatus</function> the pipeline is flagged as aborted. - <application>libpq</application> will report + From the client perspective, after <function>PQresultStatus</function> + returns <literal>PGRES_FATAL_ERROR</literal>, + the pipeline is flagged as aborted. + <function>PQresultStatus</function>, will report a <literal>PGRES_PIPELINE_ABORTED</literal> result for each remaining queued operation in an aborted pipeline. The result for <function>PQsendPipeline</function> is reported as @@ -5108,8 +5110,8 @@ int PQflush(PGconn *conn); </para> <para> - If the pipeline used an implicit transaction then operations that have - already executed are rolled back and operations that were queued for after + If the pipeline used an implicit transaction, then operations that have + already executed are rolled back and operations that were queued to follow the failed operation are skipped entirely. The same behaviour holds if the pipeline starts and commits a single explicit transaction (i.e. the first statement is <literal>BEGIN</literal> and the last is @@ -5145,11 +5147,11 @@ int PQflush(PGconn *conn); <para> The client application should generally maintain a queue of work - still to be dispatched and a queue of work that has been dispatched + remaining to be dispatched and a queue of work that has been dispatched but not yet had its results processed. When the socket is writable it should dispatch more work. When the socket is readable it should read results and process them, matching them up to the next entry in - its expected results queue. Based on available memory, results from + its expected results queue. Based on available memory, results from the socket should be read frequently: there's no need to wait until the pipeline end to read the results. Pipelines should be scoped to logical units of work, usually (but not necessarily) one transaction per pipeline. @@ -5191,8 +5193,8 @@ int PQflush(PGconn *conn); <listitem> <para> - Returns current pipeline mode status of the <application>libpq</application> - connection. + Returns the current pipeline mode status of the + <application>libpq</application> connection. <synopsis> PGpipelineStatus PQpipelineStatus(const PGconn *conn); </synopsis> @@ -5233,11 +5235,10 @@ PGpipelineStatus PQpipelineStatus(const PGconn *conn); <listitem> <para> The <application>libpq</application> connection is in pipeline - mode and an error has occurred while processing the current - pipeline. - The aborted flag is cleared as soon as the result - of the <function>PQsendPipeline</function> at the end of the aborted - pipeline is processed. Clients don't usually need this function to + mode and an error occurred while processing the current pipeline. + The aborted flag is cleared when <function>PQresultStatus</function> + returns PGRES_PIPELINE_SYNC at the end of the pipeline. + Clients don't usually need this function to verify aborted status, as they can tell that the pipeline is aborted from the <literal>PGRES_PIPELINE_ABORTED</literal> result code. </para> @@ -5328,7 +5329,7 @@ int PQsendPipeline(PGconn *conn); Returns 1 for success. Returns 0 if the connection is not in pipeline mode or sending a <link linkend="protocol-flow-ext-query">sync message</link> - is failed. + failed. </para> </listitem> </varlistentry> @@ -5349,7 +5350,7 @@ int PQsendPipeline(PGconn *conn); <para> Pipeline mode is most useful when the server is distant, i.e., network latency (<quote>ping time</quote>) is high, and also when many small operations - are being performed in rapid sequence. There is usually less benefit + are being performed in rapid succession. There is usually less benefit in using pipelined commands when each query takes many multiples of the client/server round-trip time to execute. A 100-statement operation run on a server 300ms round-trip-time away would take 30 seconds in network latency alone @@ -5367,7 +5368,7 @@ int PQsendPipeline(PGconn *conn); <para> Pipeline mode is not useful when information from one operation is required by the client to produce the next operation. In such cases, the client - must introduce a synchronization point and wait for a full client/server + would have to introduce a synchronization point and wait for a full client/server round-trip to get the results it needs. However, it's often possible to adjust the client design to exchange the required information server-side. Read-modify-write cycles are especially good candidates; for example: @@ -5392,10 +5393,10 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; <note> <para> - The pipeline API was introduced in PostgreSQL 14, but clients using - the PostgreSQL 14 version of <application>libpq</application> can use - pipelines on server versions 7.4 and newer. Pipeline mode works on any server - that supports the v3 extended query protocol. + The pipeline API was introduced in <productname>PostgreSQL</productname> 14. + Pipeline mode is a client-side feature which doesn't require server + support, and works on any server that supports the v3 extended query + protocol. </para> </note> </sect2> diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 7ae7c14948..d7b036a35c 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -3803,7 +3803,7 @@ pqPipelineFlush(PGconn *conn) { if ((conn->pipelineStatus == PQ_PIPELINE_OFF) || (conn->outCount >= OUTBUFFER_THRESHOLD)) - return (pqFlush(conn)); + return pqFlush(conn); return 0; } diff --git a/src/test/modules/test_libpq/pipeline.c b/src/test/modules/test_libpq/pipeline.c index f4a2bdec57..01d5a9a8ff 100644 --- a/src/test/modules/test_libpq/pipeline.c +++ b/src/test/modules/test_libpq/pipeline.c @@ -126,7 +126,7 @@ test_simple_pipeline(PGconn *conn) * process the results of as they come in. * * For a simple case we should be able to do this without interim - * processing of results since our out buffer will give us enough slush to + * processing of results since our output buffer will give us enough slush to * work with and we won't block on sending. So blocking mode is fine. */ if (PQisnonblocking(conn)) @@ -603,7 +603,7 @@ test_pipelined_insert(PGconn *conn, int n_rows) /* * Now we start inserting. We'll be sending enough data that we could fill - * our out buffer, so to avoid deadlocking we need to enter nonblocking + * our output buffer, so to avoid deadlocking we need to enter nonblocking * mode and consume input while we send more output. As results of each * query are processed we should pop them to allow processing of the next * query. There's no need to finish the pipeline before processing @@ -635,7 +635,7 @@ test_pipelined_insert(PGconn *conn, int n_rows) } /* - * Process any results, so we keep the server's out buffer free + * Process any results, so we keep the server's output buffer free * flowing and it can continue to process input */ if (FD_ISSET(sock, &input_mask)) -- 2.17.0 --oC1+HKm2/end4ao3-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* optimize file transfer in pg_upgrade @ 2024-11-06 22:07 Nathan Bossart <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Nathan Bossart @ 2024-11-06 22:07 UTC (permalink / raw) To: pgsql-hackers For clusters with many relations, the file transfer step of pg_upgrade can take the longest. This step clones, copies, or links the user relation files from the older cluster to the new cluster, so the amount of time it takes is closely related to the number of relations. However, since v15, we've preserved the relfilenodes during pg_upgrade, which means that all of these user relation files will have the same name. Therefore, it can be much faster to instead move the entire data directory from the old cluster to the new cluster and to then swap the catalog relation files. The attached proof-of-concept patches implement this "catalog-swap" mode for demonstration purposes. I tested this mode on a cluster with 200 databases, each with 10,000 tables with 1,000 rows and 2 unique constraints apiece. Each database also had 10,000 sequences. The test used 96 jobs. pg_upgrade --link --sync-method syncfs --> 10m 23s (~5m linking) pg_upgrade --catalog-swap --> 5m 32s (~30s linking) While these results are encouraging, there are a couple of interesting problems to manage. First, in order to move the data directory from the old cluster to the new cluster, we will have first moved the new cluster's data directory (full of files created by pg_restore) aside. After the file transfer stage, this directory will be filled with useless empty files that should eventually be deleted. Furthermore, none of these files will have been synchronized to disk (outside of whatever the kernel has done in the background), so pg_upgrade's data synchronization step can take a very long time, even when syncfs() is used (so long that pg_upgrade can take even longer than before). After much testing, the best way I've found to deal with this problem is to introduce a special mode for "initdb --sync-only" that calls fsync() for everything _except_ the actual data files. If we fsync() the new catalog files as we move them into place, and if we assume that the old catalog files will have been properly synchronized before upgrading, there's no reason to synchronize them again at the end. Another interesting problem is that pg_upgrade currently doesn't transfer the sequence data files. Since v10, we've restored these via pg_restore. I believe this was originally done for the introduction of the pg_sequence catalog, which changed the format of sequence tuples. In the new catalog-swap mode I am proposing, this means we need to transfer all the pg_restore-generated sequence data files. If there are many sequences, it can be difficult to determine which transfer mode and synchronization method will be faster. Since sequence tuple modifications are very rare, I think the new catalog-swap mode should just use the sequence data files from the old cluster whenever possible. There are a couple of other smaller trade-offs with this approach, too. First, this new mode complicates rollback if, say, the machine loses power during file transfer. IME the vast majority of failures happen before this step, and it should be relatively simple to generate a script that will safely perform the required rollback steps, so I don't think this is a deal-breaker. Second, this mode leaves around a bunch of files that users would likely want to clean up at some point. I think the easiest way to handle this is to just put all these files in the old cluster's data directory so that the cleanup script generated by pg_upgrade also takes care of them. Thoughts? -- nathan From f800010296b1749b57e0fe3dcde010cc2ba41973 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 15:59:51 -0600 Subject: [PATCH v1 1/8] Export walkdir(). THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. A follow-up commit will use this function to swap catalog files between database directories during pg_upgrade. --- src/common/file_utils.c | 5 +---- src/include/common/file_utils.h | 3 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 398fe1c334..3f488bf5ec 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -48,9 +48,6 @@ #ifdef PG_FLUSH_DATA_WORKS static int pre_sync_fname(const char *fname, bool isdir); #endif -static void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks); #ifdef HAVE_SYNCFS @@ -268,7 +265,7 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * * See also walkdir in fd.c, which is a backend version of this logic. */ -static void +void walkdir(const char *path, int (*action) (const char *fname, bool isdir), bool process_symlinks) diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index e4339fb7b6..5a9519acfe 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -39,6 +39,9 @@ extern void sync_pgdata(const char *pg_data, int serverVersion, extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); +extern void walkdir(const char *path, + int (*action) (const char *fname, bool isdir), + bool process_symlinks); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) From 2d6b0d5708f07203ad2ffbd889404094d0c5969c Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 13:59:39 -0600 Subject: [PATCH v1 2/8] Add "void *arg" parameter to walkdir() that is passed to function. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This will be used in follow up commits to pass private state to the functions called by walkdir(). --- src/bin/pg_basebackup/walmethods.c | 8 +++---- src/bin/pg_dump/pg_backup_custom.c | 2 +- src/bin/pg_dump/pg_backup_tar.c | 2 +- src/bin/pg_dump/pg_dumpall.c | 2 +- src/common/file_utils.c | 38 +++++++++++++++--------------- src/include/common/file_utils.h | 6 ++--- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c index 215b24597f..51640cb493 100644 --- a/src/bin/pg_basebackup/walmethods.c +++ b/src/bin/pg_basebackup/walmethods.c @@ -251,7 +251,7 @@ dir_open_for_write(WalWriteMethod *wwmethod, const char *pathname, */ if (wwmethod->sync) { - if (fsync_fname(tmppath, false) != 0 || + if (fsync_fname(tmppath, false, NULL) != 0 || fsync_parent_path(tmppath) != 0) { wwmethod->lasterrno = errno; @@ -486,7 +486,7 @@ dir_close(Walfile *f, WalCloseMethod method) */ if (f->wwmethod->sync) { - r = fsync_fname(df->fullpath, false); + r = fsync_fname(df->fullpath, false, NULL); if (r == 0) r = fsync_parent_path(df->fullpath); } @@ -617,7 +617,7 @@ dir_finish(WalWriteMethod *wwmethod) * Files are fsynced when they are closed, but we need to fsync the * directory entry here as well. */ - if (fsync_fname(dir_data->basedir, true) != 0) + if (fsync_fname(dir_data->basedir, true, NULL) != 0) { wwmethod->lasterrno = errno; return false; @@ -1321,7 +1321,7 @@ tar_finish(WalWriteMethod *wwmethod) if (wwmethod->sync) { - if (fsync_fname(tar_data->tarfilename, false) != 0 || + if (fsync_fname(tar_data->tarfilename, false, NULL) != 0 || fsync_parent_path(tar_data->tarfilename) != 0) { wwmethod->lasterrno = errno; diff --git a/src/bin/pg_dump/pg_backup_custom.c b/src/bin/pg_dump/pg_backup_custom.c index ecaad7321a..6f750c916c 100644 --- a/src/bin/pg_dump/pg_backup_custom.c +++ b/src/bin/pg_dump/pg_backup_custom.c @@ -767,7 +767,7 @@ _CloseArchive(ArchiveHandle *AH) /* Sync the output file if one is defined */ if (AH->dosync && AH->mode == archModeWrite && AH->fSpec) - (void) fsync_fname(AH->fSpec, false); + (void) fsync_fname(AH->fSpec, false, NULL); AH->FH = NULL; } diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index 41ee52b1d6..ecba27b623 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -847,7 +847,7 @@ _CloseArchive(ArchiveHandle *AH) /* Sync the output file if one is defined */ if (AH->dosync && AH->fSpec) - (void) fsync_fname(AH->fSpec, false); + (void) fsync_fname(AH->fSpec, false, NULL); } AH->FH = NULL; diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index e3ad8fb295..cbb1e3f9e4 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -621,7 +621,7 @@ main(int argc, char *argv[]) /* sync the resulting file, errors are not fatal */ if (dosync) - (void) fsync_fname(filename, false); + (void) fsync_fname(filename, false, NULL); } exit_nicely(0); diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 3f488bf5ec..dc90f35ae1 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -46,7 +46,7 @@ #define MINIMUM_VERSION_FOR_PG_WAL 100000 #ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir); +static int pre_sync_fname(const char *fname, bool isdir, void *arg); #endif #ifdef HAVE_SYNCFS @@ -184,10 +184,10 @@ sync_pgdata(const char *pg_data, * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false); + walkdir(pg_data, pre_sync_fname, false, NULL); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false); - walkdir(pg_tblspc, pre_sync_fname, true); + walkdir(pg_wal, pre_sync_fname, false, NULL); + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -200,10 +200,10 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false); + walkdir(pg_data, fsync_fname, false, NULL); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false); - walkdir(pg_tblspc, fsync_fname, true); + walkdir(pg_wal, fsync_fname, false, NULL); + walkdir(pg_tblspc, fsync_fname, true, NULL); } break; } @@ -242,10 +242,10 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(dir, pre_sync_fname, false); + walkdir(dir, pre_sync_fname, false, NULL); #endif - walkdir(dir, fsync_fname, false); + walkdir(dir, fsync_fname, false, NULL); } break; } @@ -267,8 +267,8 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) */ void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks) + int (*action) (const char *fname, bool isdir, void *arg), + bool process_symlinks, void *arg) { DIR *dir; struct dirent *de; @@ -293,10 +293,10 @@ walkdir(const char *path, switch (get_dirent_type(subpath, de, process_symlinks, PG_LOG_ERROR)) { case PGFILETYPE_REG: - (*action) (subpath, false); + (*action) (subpath, false, arg); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false); + walkdir(subpath, action, false, arg); break; default: @@ -320,7 +320,7 @@ walkdir(const char *path, * synced. Recent versions of ext4 have made the window much wider but * it's been an issue for ext3 and other filesystems in the past. */ - (*action) (path, true); + (*action) (path, true, arg); } /* @@ -332,7 +332,7 @@ walkdir(const char *path, #ifdef PG_FLUSH_DATA_WORKS static int -pre_sync_fname(const char *fname, bool isdir) +pre_sync_fname(const char *fname, bool isdir, void *arg) { int fd; @@ -373,7 +373,7 @@ pre_sync_fname(const char *fname, bool isdir) * are fatal. */ int -fsync_fname(const char *fname, bool isdir) +fsync_fname(const char *fname, bool isdir, void *arg) { int fd; int flags; @@ -444,7 +444,7 @@ fsync_parent_path(const char *fname) if (strlen(parentpath) == 0) strlcpy(parentpath, ".", MAXPGPATH); - if (fsync_fname(parentpath, true) != 0) + if (fsync_fname(parentpath, true, NULL) != 0) return -1; return 0; @@ -467,7 +467,7 @@ durable_rename(const char *oldfile, const char *newfile) * because it's then guaranteed that either source or target file exists * after a crash. */ - if (fsync_fname(oldfile, false) != 0) + if (fsync_fname(oldfile, false, NULL) != 0) return -1; fd = open(newfile, PG_BINARY | O_RDWR, 0); @@ -502,7 +502,7 @@ durable_rename(const char *oldfile, const char *newfile) * To guarantee renaming the file is persistent, fsync the file with its * new name, and its containing directory. */ - if (fsync_fname(newfile, false) != 0) + if (fsync_fname(newfile, false, NULL) != 0) return -1; if (fsync_parent_path(newfile) != 0) diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 5a9519acfe..c328f56a85 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -33,15 +33,15 @@ typedef enum DataDirSyncMethod struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND -extern int fsync_fname(const char *fname, bool isdir); +extern int fsync_fname(const char *fname, bool isdir, void *arg); extern void sync_pgdata(const char *pg_data, int serverVersion, DataDirSyncMethod sync_method); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); extern void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks); + int (*action) (const char *fname, bool isdir, void *arg), + bool process_symlinks, void *arg); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) From c248059f7276578f5e8abd00ea9efb007423f94d Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 16:38:19 -0600 Subject: [PATCH v1 3/8] Introduce catalog-swap mode for pg_upgrade. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This new mode moves the database directories from the old cluster to the new cluster and then swaps the pg_restore-generated catalog files in place. This can significantly increase the length of the following data synchronization step (due to the large number of unsynchronized pg_restore-generated files), but this problem will be handled in follow-up commits. --- src/bin/pg_upgrade/check.c | 2 + src/bin/pg_upgrade/option.c | 5 + src/bin/pg_upgrade/pg_upgrade.h | 1 + src/bin/pg_upgrade/relfilenumber.c | 167 +++++++++++++++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 5 files changed, 176 insertions(+) diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 94164f0472..a4bb365718 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -711,6 +711,8 @@ check_new_cluster(void) case TRANSFER_MODE_LINK: check_hard_link(); break; + case TRANSFER_MODE_CATALOG_SWAP: + break; } check_is_install_user(&new_cluster); diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 6f41d63eed..64091a54c4 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[]) {"copy", no_argument, NULL, 2}, {"copy-file-range", no_argument, NULL, 3}, {"sync-method", required_argument, NULL, 4}, + {"catalog-swap", no_argument, NULL, 5}, {NULL, 0, NULL, 0} }; @@ -212,6 +213,10 @@ parseCommandLine(int argc, char *argv[]) user_opts.sync_method = pg_strdup(optarg); break; + case 5: + user_opts.transfer_mode = TRANSFER_MODE_CATALOG_SWAP; + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index 53f693c2d4..19cb5a011e 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -256,6 +256,7 @@ typedef enum TRANSFER_MODE_COPY, TRANSFER_MODE_COPY_FILE_RANGE, TRANSFER_MODE_LINK, + TRANSFER_MODE_CATALOG_SWAP, } transferMode; /* diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 07baa49a02..9d8fce3c4a 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -11,11 +11,21 @@ #include <sys/stat.h> +#include "common/file_perm.h" +#include "common/file_utils.h" +#include "common/int.h" +#include "fe_utils/option_utils.h" #include "pg_upgrade.h" static void transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace); static void transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_frozenbit); +typedef struct move_catalog_file_context +{ + FileNameMap *maps; + int size; + char *target; +} move_catalog_file_context; /* * transfer_all_new_tablespaces() @@ -41,6 +51,9 @@ transfer_all_new_tablespaces(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, case TRANSFER_MODE_LINK: prep_status_progress("Linking user relation files"); break; + case TRANSFER_MODE_CATALOG_SWAP: + prep_status_progress("Swapping catalog files"); + break; } /* @@ -127,6 +140,144 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, } } +static int +FileNameMapCmp(const void *a, const void *b) +{ + return pg_cmp_u32(((const FileNameMap *) a)->relfilenumber, + ((const FileNameMap *) b)->relfilenumber); +} + +static RelFileNumber +parse_relfilenumber(const char *filename) +{ + char *endp; + unsigned long n; + + if (filename[0] < '1' || filename[0] > '9') + return InvalidRelFileNumber; + + errno = 0; + n = strtoul(filename, &endp, 10); + if (errno || filename == endp || n <= 0 || n > PG_UINT32_MAX) + return InvalidRelFileNumber; + + return (RelFileNumber) n; +} + +static int +move_catalog_file(const char *fname, bool isdir, void *arg) +{ + char dst[MAXPGPATH]; + const char *filename = last_dir_separator(fname) + 1; + RelFileNumber rfn = parse_relfilenumber(filename); + move_catalog_file_context *context = (move_catalog_file_context *) arg; + + /* + * XXX: Is this right? AFAICT we don't really expect there to be + * directories within database directories, so perhaps it would be better + * to either unconditionally rename or to fail. Further investigation is + * required. + */ + if (isdir) + return 0; + + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key; + + key.relfilenumber = (RelFileNumber) rfn; + if (bsearch(&key, context->maps, context->size, + sizeof(FileNameMap), FileNameMapCmp)) + return 0; + } + + snprintf(dst, sizeof(dst), "%s/%s", context->target, filename); + if (rename(fname, dst) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", fname, dst); + + return 0; +} + +/* + * XXX: This proof-of-concept patch doesn't yet handle non-default tablespaces. + */ +static void +do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) +{ + char old_tblspc[MAXPGPATH]; + char new_tblspc[MAXPGPATH]; + char old_dat[MAXPGPATH]; + char new_dat[MAXPGPATH]; + char moved_tblspc[MAXPGPATH]; + char moved_dat[MAXPGPATH]; + char old_cat[MAXPGPATH]; + move_catalog_file_context context; + DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; + + parse_sync_method(user_opts.sync_method, &sync_method); + + snprintf(old_tblspc, sizeof(old_tblspc), "%s%s", + maps[0].old_tablespace, maps[0].old_tablespace_suffix); + snprintf(new_tblspc, sizeof(new_tblspc), "%s%s", + maps[0].new_tablespace, maps[0].new_tablespace_suffix); + snprintf(old_dat, sizeof(old_dat), "%s/%u", old_tblspc, maps[0].db_oid); + snprintf(new_dat, sizeof(new_dat), "%s/%u", new_tblspc, maps[0].db_oid); + snprintf(moved_tblspc, sizeof(moved_tblspc), "%s_moved", old_tblspc); + snprintf(moved_dat, sizeof(moved_dat), "%s/%u", + moved_tblspc, maps[0].db_oid); + snprintf(old_cat, sizeof(old_cat), "%s/%u_old_cat", + moved_tblspc, maps[0].db_oid); + + qsort(maps, size, sizeof(FileNameMap), FileNameMapCmp); + + /* create dir for stuff that is moved aside */ + if (pg_mkdir_p(moved_tblspc, pg_dir_create_mode) && errno != EEXIST) + pg_fatal("could not create directory \"%s\": %m", moved_tblspc); + + /* move new cluster data dir aside */ + if (rename(new_dat, moved_dat)) + pg_fatal("could not rename \"%s\" to \"%s\": %m", new_dat, moved_dat); + + /* move old cluster data dir in place */ + if (rename(old_dat, new_dat)) + pg_fatal("could not rename \"%s\" to \"%s\": %m", old_dat, new_dat); + + /* create dir for old catalogs */ + if (pg_mkdir_p(old_cat, pg_dir_create_mode)) + pg_fatal("could not create directory \"%s\": %m", old_cat); + + /* move catalogs in new data dir aside */ + context.maps = maps; + context.size = size; + context.target = old_cat; + walkdir(new_dat, move_catalog_file, false, &context); + + /* move catalogs in moved-aside data dir in place */ + context.target = new_dat; + walkdir(moved_dat, move_catalog_file, false, &context); + + /* no need to sync things individually if we are going to syncfs() later */ + if (sync_method == DATA_DIR_SYNC_METHOD_SYNCFS) + return; + + /* fsync directory entries */ + if (fsync_fname(moved_dat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", moved_dat); + if (fsync_fname(old_cat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", old_cat); + + /* + * XXX: We could instead fsync() these directories once at the end instead + * of once per-database, but it doesn't affect performance meaningfully, + * and this is just a proof-of-concept patch, so I haven't bothered doing + * the required refactoring yet. + */ + if (fsync_fname(old_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", old_tblspc); + if (fsync_fname(moved_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", moved_tblspc); +} + /* * transfer_single_new_db() * @@ -145,6 +296,18 @@ transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace) new_cluster.controldata.cat_ver >= VISIBILITY_MAP_FROZEN_BIT_CAT_VER) vm_must_add_frozenbit = true; + /* + * XXX: In catalog-swap mode, vm_must_add_frozenbit isn't handled yet. We + * could either disallow using catalog-swap mode if the upgrade involves + * versions older than v9.6, or we could add code to handle rewriting the + * visibility maps in this mode (like the other modes do). + */ + if (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) + { + do_catalog_transfer(maps, size, old_tablespace); + return; + } + for (mapnum = 0; mapnum < size; mapnum++) { if (old_tablespace == NULL || @@ -259,6 +422,10 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"", old_file, new_file); linkFile(old_file, new_file, map->nspname, map->relname); + break; + case TRANSFER_MODE_CATALOG_SWAP: + pg_fatal("should never happen"); + break; } } } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 1847bbfa95..58c339af85 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3649,6 +3649,7 @@ mix_data_t mixedStruct mode_t movedb_failure_params +move_catalog_file_context multirange_bsearch_comparison multirange_unnest_fctx mxact -- 2.39.5 (Apple Git-154) From 1fd29fa00c777b2c04683394f54261b446e46a61 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 16:47:42 -0600 Subject: [PATCH v1 4/8] Add --no-sync-data-files flag to initdb. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This new mode caused 'initdb --sync-only' to synchronize everything except for the database directories. It will be used in a follow-up commit that aims to reduce the duration of the data synchronization step in pg_upgrade's catalog-swap mode. --- src/bin/initdb/initdb.c | 9 ++++-- src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 2 +- src/bin/pg_rewind/file_ops.c | 2 +- src/common/file_utils.c | 35 ++++++++++++++++----- src/include/common/file_utils.h | 3 +- 7 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 9a91830783..53c6e86a80 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -168,6 +168,7 @@ static bool data_checksums = true; static char *xlog_dir = NULL; static int wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024); static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; +static bool sync_data_files = true; /* internal vars */ @@ -3183,6 +3184,7 @@ main(int argc, char *argv[]) {"icu-rules", required_argument, NULL, 18}, {"sync-method", required_argument, NULL, 19}, {"no-data-checksums", no_argument, NULL, 20}, + {"no-sync-data-files", no_argument, NULL, 21}, {NULL, 0, NULL, 0} }; @@ -3377,6 +3379,9 @@ main(int argc, char *argv[]) case 20: data_checksums = false; break; + case 21: + sync_data_files = false; + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -3428,7 +3433,7 @@ main(int argc, char *argv[]) fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); return 0; } @@ -3491,7 +3496,7 @@ main(int argc, char *argv[]) { fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); } else diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index e41a6cfbda..43526e3246 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2310,7 +2310,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail, } else { - (void) sync_pgdata(basedir, serverVersion, sync_method); + (void) sync_pgdata(basedir, serverVersion, sync_method, true); } } diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index b86bc417c9..06ccaacfda 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -633,7 +633,7 @@ main(int argc, char *argv[]) if (do_sync) { pg_log_info("syncing data directory"); - sync_pgdata(DataDir, PG_VERSION_NUM, sync_method); + sync_pgdata(DataDir, PG_VERSION_NUM, sync_method, true); } pg_log_info("updating control file"); diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 5f1f62f1db..80a137be4e 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -420,7 +420,7 @@ main(int argc, char *argv[]) else { pg_log_debug("recursively fsyncing \"%s\"", opt.output); - sync_pgdata(opt.output, version * 10000, opt.sync_method); + sync_pgdata(opt.output, version * 10000, opt.sync_method, true); } } diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 67a86bb4c5..ceb1c3ac6d 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -296,7 +296,7 @@ sync_target_dir(void) if (!do_sync || dry_run) return; - sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method); + sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method, true); } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index dc90f35ae1..65cdf07ae7 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -94,7 +94,8 @@ do_syncfs(const char *path) void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method) + DataDirSyncMethod sync_method, + bool sync_data_files) { bool xlog_is_symlink; char pg_wal[MAXPGPATH]; @@ -184,10 +185,11 @@ sync_pgdata(const char *pg_data, * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false, NULL); + walkdir(pg_data, pre_sync_fname, false, &sync_data_files); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false, NULL); - walkdir(pg_tblspc, pre_sync_fname, true, NULL); + walkdir(pg_wal, pre_sync_fname, false, &sync_data_files); + if (sync_data_files) + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -200,10 +202,11 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false, NULL); + walkdir(pg_data, fsync_fname, false, &sync_data_files); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false, NULL); - walkdir(pg_tblspc, fsync_fname, true, NULL); + walkdir(pg_wal, fsync_fname, false, &sync_data_files); + if (sync_data_files) + walkdir(pg_tblspc, fsync_fname, true, NULL); } break; } @@ -296,7 +299,23 @@ walkdir(const char *path, (*action) (subpath, false, arg); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false, arg); + + /* + * XXX: Checking here for the "sync_data_files" case is quite + * hacky, but it's not clear how to do better. Another option + * would be to send "de" down to the function, but that would + * introduce a huge number of function pointer calls and + * directory reads that we are trying to avoid. + */ +#ifdef PG_FLUSH_DATA_WORKS + if ((action != pre_sync_fname && action != fsync_fname) || +#else + if (action != fsync_fname || +#endif + !arg || *((bool *) arg) || + strcmp(de->d_name, "base") != 0) + walkdir(subpath, action, false, arg); + break; default: diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index c328f56a85..3743caa63e 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -35,7 +35,8 @@ struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND extern int fsync_fname(const char *fname, bool isdir, void *arg); extern void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method); + DataDirSyncMethod sync_method, + bool sync_data_files); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); -- 2.39.5 (Apple Git-154) From e70eea50e81d8f40fb8db15c06b23305d4b8698f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 09:52:19 -0600 Subject: [PATCH v1 5/8] Export pre_sync_fname(). THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. A follow-up commit will use this function to alert the file system that we want a file's data on disk so that subsequent calls to fsync() are faster. --- src/common/file_utils.c | 18 +++++------------- src/include/common/file_utils.h | 1 + 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 65cdf07ae7..5c201ec6e8 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -45,10 +45,6 @@ */ #define MINIMUM_VERSION_FOR_PG_WAL 100000 -#ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir, void *arg); -#endif - #ifdef HAVE_SYNCFS /* @@ -307,11 +303,7 @@ walkdir(const char *path, * introduce a huge number of function pointer calls and * directory reads that we are trying to avoid. */ -#ifdef PG_FLUSH_DATA_WORKS if ((action != pre_sync_fname && action != fsync_fname) || -#else - if (action != fsync_fname || -#endif !arg || *((bool *) arg) || strcmp(de->d_name, "base") != 0) walkdir(subpath, action, false, arg); @@ -348,11 +340,12 @@ walkdir(const char *path, * Ignores errors trying to open unreadable files, and reports other errors * non-fatally. */ -#ifdef PG_FLUSH_DATA_WORKS - -static int +int pre_sync_fname(const char *fname, bool isdir, void *arg) { +#ifndef PG_FLUSH_DATA_WORKS + return 0; +#else int fd; fd = open(fname, O_RDONLY | PG_BINARY, 0); @@ -380,9 +373,8 @@ pre_sync_fname(const char *fname, bool isdir, void *arg) (void) close(fd); return 0; -} - #endif /* PG_FLUSH_DATA_WORKS */ +} /* * fsync_fname -- Try to fsync a file or directory diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 3743caa63e..e7a34d4c4e 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -43,6 +43,7 @@ extern int fsync_parent_path(const char *fname); extern void walkdir(const char *path, int (*action) (const char *fname, bool isdir, void *arg), bool process_symlinks, void *arg); +extern int pre_sync_fname(const char *fname, bool isdir, void *arg); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) From 5d17fd66e08612574ffe0c39ff7624259319059c Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:40:43 -0600 Subject: [PATCH v1 6/8] In pg_upgrade's catalog-swap mode, only sync files as necessary. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. In this mode, it can be much faster to use "--sync-method fsync", which now skips synchronizing data files moved from the old cluster (which we assumed were synchronized before pg_upgrade). --- src/bin/pg_upgrade/pg_upgrade.c | 6 ++-- src/bin/pg_upgrade/relfilenumber.c | 52 ++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 663235816f..f5946ac89a 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -210,10 +210,12 @@ main(int argc, char **argv) { prep_status("Sync data directory to disk"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/initdb\" --sync-only \"%s\" --sync-method %s", + "\"%s/initdb\" --sync-only \"%s\" --sync-method %s %s", new_cluster.bindir, new_cluster.pgdata, - user_opts.sync_method); + user_opts.sync_method, + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + "--no-sync-data-files" : ""); check_ok(); } diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 9d8fce3c4a..dcca4bb2e7 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -25,8 +25,49 @@ typedef struct move_catalog_file_context FileNameMap *maps; int size; char *target; + bool sync_moved; } move_catalog_file_context; +#define SYNC_QUEUE_MAX_LEN (1024) + +static char *sync_queue[SYNC_QUEUE_MAX_LEN]; +static bool sync_queue_inited; +static int sync_queue_len; + +static inline void +sync_queue_init(void) +{ + if (sync_queue_inited) + return; + + sync_queue_inited = true; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + sync_queue[i] = palloc(MAXPGPATH); +} + +static inline void +sync_queue_sync_all(void) +{ + if (!sync_queue_inited) + return; + + for (int i = 0; i < sync_queue_len; i++) + fsync_fname(sync_queue[i], false, NULL); + sync_queue_len = 0; +} + +static inline void +sync_queue_push(const char *fname) +{ + sync_queue_init(); + + pre_sync_fname(fname, false, NULL); + + strncpy(sync_queue[sync_queue_len++], fname, MAXPGPATH); + if (sync_queue_len >= SYNC_QUEUE_MAX_LEN) + sync_queue_sync_all(); +} + /* * transfer_all_new_tablespaces() * @@ -138,6 +179,8 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, /* We allocate something even for n_maps == 0 */ pg_free(mappings); } + + sync_queue_sync_all(); } static int @@ -195,6 +238,9 @@ move_catalog_file(const char *fname, bool isdir, void *arg) if (rename(fname, dst) != 0) pg_fatal("could not rename \"%s\" to \"%s\": %m", fname, dst); + if (context->sync_moved) + sync_queue_push(dst); + return 0; } @@ -250,10 +296,12 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) context.maps = maps; context.size = size; context.target = old_cat; + context.sync_moved = false; walkdir(new_dat, move_catalog_file, false, &context); /* move catalogs in moved-aside data dir in place */ context.target = new_dat; + context.sync_moved = (sync_method != DATA_DIR_SYNC_METHOD_SYNCFS); walkdir(moved_dat, move_catalog_file, false, &context); /* no need to sync things individually if we are going to syncfs() later */ @@ -265,6 +313,8 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) pg_fatal("could not synchronize directory \"%s\": %m", moved_dat); if (fsync_fname(old_cat, true, NULL) != 0) pg_fatal("could not synchronize directory \"%s\": %m", old_cat); + if (fsync_fname(new_dat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_dat); /* * XXX: We could instead fsync() these directories once at the end instead @@ -276,6 +326,8 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) pg_fatal("could not synchronize directory \"%s\": %m", old_tblspc); if (fsync_fname(moved_tblspc, true, NULL) != 0) pg_fatal("could not synchronize directory \"%s\": %m", moved_tblspc); + if (fsync_fname(new_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_tblspc); } /* -- 2.39.5 (Apple Git-154) From 234d40f4e45d56f40465037dcf83c8c980edf095 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:46:11 -0600 Subject: [PATCH v1 7/8] Add --sequence-data flag to pg_dump. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This flag can be used to optionally dump the sequence data even when --schema-only is used. It is primarily intended for use in a follow-up commit that will cause sequence data files to be carried over from the old cluster in pg_upgrade's new catalog-swap mode. --- src/bin/pg_dump/pg_dump.c | 9 +-------- src/bin/pg_upgrade/dump.c | 2 +- src/test/modules/test_pg_dump/t/001_base.pl | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index b2f4eb2c6d..2b57abd305 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -501,6 +501,7 @@ main(int argc, char **argv) {"sync-method", required_argument, NULL, 15}, {"filter", required_argument, NULL, 16}, {"exclude-extension", required_argument, NULL, 17}, + {"sequence-data", no_argument, &dopt.sequence_data, 1}, {NULL, 0, NULL, 0} }; @@ -768,14 +769,6 @@ main(int argc, char **argv) if (dopt.column_inserts && dopt.dump_inserts == 0) dopt.dump_inserts = DUMP_DEFAULT_ROWS_PER_INSERT; - /* - * Binary upgrade mode implies dumping sequence data even in schema-only - * mode. This is not exposed as a separate option, but kept separate - * internally for clarity. - */ - if (dopt.binary_upgrade) - dopt.sequence_data = 1; - if (dopt.dataOnly && dopt.schemaOnly) pg_fatal("options -s/--schema-only and -a/--data-only cannot be used together"); diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 8345f55be8..8453722833 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -53,7 +53,7 @@ generate_old_dump(void) parallel_exec_prog(log_file_name, NULL, "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers " - "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", + "--binary-upgrade --sequence-data --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), log_opts.verbose ? "--verbose" : "", log_opts.dumpdir, diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl index e2579e29cd..46231c93f1 100644 --- a/src/test/modules/test_pg_dump/t/001_base.pl +++ b/src/test/modules/test_pg_dump/t/001_base.pl @@ -48,7 +48,7 @@ my %pgdump_runs = ( dump_cmd => [ 'pg_dump', '--no-sync', "--file=$tempdir/binary_upgrade.sql", '--schema-only', - '--binary-upgrade', '--dbname=postgres', + '--binary-upgrade', '--sequence-data', '--dbname=postgres', ], }, clean => { -- 2.39.5 (Apple Git-154) From 51be6c09256272e1ce0360b5376b4a14cd1d9a61 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:53:40 -0600 Subject: [PATCH v1 8/8] Avoid copying sequence files in pg_upgrade's catalog-swap mode. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. On clusters with many sequences, this can further reduce the amount of time required to wire up the data files in the new cluster. If the sequence data file format changes, this optimization cannot be used, but that seems rare enough. --- src/bin/pg_upgrade/dump.c | 8 +++++++- src/bin/pg_upgrade/info.c | 6 +++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 8453722833..d5a81cc29c 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -51,10 +51,16 @@ generate_old_dump(void) snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid); snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); + /* + * XXX: We need to be sure that the sequence data format hasn't + * changed. + */ parallel_exec_prog(log_file_name, NULL, "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers " - "--binary-upgrade --sequence-data --format=custom %s --no-sync --file=\"%s/%s\" %s", + "--binary-upgrade %s --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + "" : "--sequence-data", log_opts.verbose ? "--verbose" : "", log_opts.dumpdir, sql_file_name, escaped_connstr.data); diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index f83ded89cb..786d17e32f 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -483,6 +483,8 @@ get_rel_infos_query(void) * pg_largeobject contains user data that does not appear in pg_dump * output, so we have to copy that system table. It's easiest to do that * by treating it as a user table. + * + * XXX: We need to be sure that the sequence data format hasn't changed. */ appendPQExpBuffer(&query, "WITH regular_heap (reloid, indtable, toastheap) AS ( " @@ -490,7 +492,7 @@ get_rel_infos_query(void) " FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n " " ON c.relnamespace = n.oid " " WHERE relkind IN (" CppAsString2(RELKIND_RELATION) ", " - CppAsString2(RELKIND_MATVIEW) ") AND " + CppAsString2(RELKIND_MATVIEW) "%s) AND " /* exclude possible orphaned temp tables */ " ((n.nspname !~ '^pg_temp_' AND " " n.nspname !~ '^pg_toast_temp_' AND " @@ -499,6 +501,8 @@ get_rel_infos_query(void) " c.oid >= %u::pg_catalog.oid) OR " " (n.nspname = 'pg_catalog' AND " " relname IN ('pg_largeobject') ))), ", + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + ", " CppAsString2(RELKIND_SEQUENCE) : "", FirstNormalObjectId); /* -- 2.39.5 (Apple Git-154) Attachments: [text/plain] v1-0001-Export-walkdir.patch (1.9K, ../../Zyvop-LxLXBLrZil@nathan/2-v1-0001-Export-walkdir.patch) download | inline diff: From f800010296b1749b57e0fe3dcde010cc2ba41973 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 15:59:51 -0600 Subject: [PATCH v1 1/8] Export walkdir(). THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. A follow-up commit will use this function to swap catalog files between database directories during pg_upgrade. --- src/common/file_utils.c | 5 +---- src/include/common/file_utils.h | 3 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 398fe1c334..3f488bf5ec 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -48,9 +48,6 @@ #ifdef PG_FLUSH_DATA_WORKS static int pre_sync_fname(const char *fname, bool isdir); #endif -static void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks); #ifdef HAVE_SYNCFS @@ -268,7 +265,7 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * * See also walkdir in fd.c, which is a backend version of this logic. */ -static void +void walkdir(const char *path, int (*action) (const char *fname, bool isdir), bool process_symlinks) diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index e4339fb7b6..5a9519acfe 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -39,6 +39,9 @@ extern void sync_pgdata(const char *pg_data, int serverVersion, extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); +extern void walkdir(const char *path, + int (*action) (const char *fname, bool isdir), + bool process_symlinks); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) [text/plain] v1-0002-Add-void-arg-parameter-to-walkdir-that-is-passed-.patch (8.4K, ../../Zyvop-LxLXBLrZil@nathan/3-v1-0002-Add-void-arg-parameter-to-walkdir-that-is-passed-.patch) download | inline diff: From 2d6b0d5708f07203ad2ffbd889404094d0c5969c Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 13:59:39 -0600 Subject: [PATCH v1 2/8] Add "void *arg" parameter to walkdir() that is passed to function. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This will be used in follow up commits to pass private state to the functions called by walkdir(). --- src/bin/pg_basebackup/walmethods.c | 8 +++---- src/bin/pg_dump/pg_backup_custom.c | 2 +- src/bin/pg_dump/pg_backup_tar.c | 2 +- src/bin/pg_dump/pg_dumpall.c | 2 +- src/common/file_utils.c | 38 +++++++++++++++--------------- src/include/common/file_utils.h | 6 ++--- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c index 215b24597f..51640cb493 100644 --- a/src/bin/pg_basebackup/walmethods.c +++ b/src/bin/pg_basebackup/walmethods.c @@ -251,7 +251,7 @@ dir_open_for_write(WalWriteMethod *wwmethod, const char *pathname, */ if (wwmethod->sync) { - if (fsync_fname(tmppath, false) != 0 || + if (fsync_fname(tmppath, false, NULL) != 0 || fsync_parent_path(tmppath) != 0) { wwmethod->lasterrno = errno; @@ -486,7 +486,7 @@ dir_close(Walfile *f, WalCloseMethod method) */ if (f->wwmethod->sync) { - r = fsync_fname(df->fullpath, false); + r = fsync_fname(df->fullpath, false, NULL); if (r == 0) r = fsync_parent_path(df->fullpath); } @@ -617,7 +617,7 @@ dir_finish(WalWriteMethod *wwmethod) * Files are fsynced when they are closed, but we need to fsync the * directory entry here as well. */ - if (fsync_fname(dir_data->basedir, true) != 0) + if (fsync_fname(dir_data->basedir, true, NULL) != 0) { wwmethod->lasterrno = errno; return false; @@ -1321,7 +1321,7 @@ tar_finish(WalWriteMethod *wwmethod) if (wwmethod->sync) { - if (fsync_fname(tar_data->tarfilename, false) != 0 || + if (fsync_fname(tar_data->tarfilename, false, NULL) != 0 || fsync_parent_path(tar_data->tarfilename) != 0) { wwmethod->lasterrno = errno; diff --git a/src/bin/pg_dump/pg_backup_custom.c b/src/bin/pg_dump/pg_backup_custom.c index ecaad7321a..6f750c916c 100644 --- a/src/bin/pg_dump/pg_backup_custom.c +++ b/src/bin/pg_dump/pg_backup_custom.c @@ -767,7 +767,7 @@ _CloseArchive(ArchiveHandle *AH) /* Sync the output file if one is defined */ if (AH->dosync && AH->mode == archModeWrite && AH->fSpec) - (void) fsync_fname(AH->fSpec, false); + (void) fsync_fname(AH->fSpec, false, NULL); AH->FH = NULL; } diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index 41ee52b1d6..ecba27b623 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -847,7 +847,7 @@ _CloseArchive(ArchiveHandle *AH) /* Sync the output file if one is defined */ if (AH->dosync && AH->fSpec) - (void) fsync_fname(AH->fSpec, false); + (void) fsync_fname(AH->fSpec, false, NULL); } AH->FH = NULL; diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index e3ad8fb295..cbb1e3f9e4 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -621,7 +621,7 @@ main(int argc, char *argv[]) /* sync the resulting file, errors are not fatal */ if (dosync) - (void) fsync_fname(filename, false); + (void) fsync_fname(filename, false, NULL); } exit_nicely(0); diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 3f488bf5ec..dc90f35ae1 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -46,7 +46,7 @@ #define MINIMUM_VERSION_FOR_PG_WAL 100000 #ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir); +static int pre_sync_fname(const char *fname, bool isdir, void *arg); #endif #ifdef HAVE_SYNCFS @@ -184,10 +184,10 @@ sync_pgdata(const char *pg_data, * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false); + walkdir(pg_data, pre_sync_fname, false, NULL); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false); - walkdir(pg_tblspc, pre_sync_fname, true); + walkdir(pg_wal, pre_sync_fname, false, NULL); + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -200,10 +200,10 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false); + walkdir(pg_data, fsync_fname, false, NULL); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false); - walkdir(pg_tblspc, fsync_fname, true); + walkdir(pg_wal, fsync_fname, false, NULL); + walkdir(pg_tblspc, fsync_fname, true, NULL); } break; } @@ -242,10 +242,10 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(dir, pre_sync_fname, false); + walkdir(dir, pre_sync_fname, false, NULL); #endif - walkdir(dir, fsync_fname, false); + walkdir(dir, fsync_fname, false, NULL); } break; } @@ -267,8 +267,8 @@ sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method) */ void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks) + int (*action) (const char *fname, bool isdir, void *arg), + bool process_symlinks, void *arg) { DIR *dir; struct dirent *de; @@ -293,10 +293,10 @@ walkdir(const char *path, switch (get_dirent_type(subpath, de, process_symlinks, PG_LOG_ERROR)) { case PGFILETYPE_REG: - (*action) (subpath, false); + (*action) (subpath, false, arg); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false); + walkdir(subpath, action, false, arg); break; default: @@ -320,7 +320,7 @@ walkdir(const char *path, * synced. Recent versions of ext4 have made the window much wider but * it's been an issue for ext3 and other filesystems in the past. */ - (*action) (path, true); + (*action) (path, true, arg); } /* @@ -332,7 +332,7 @@ walkdir(const char *path, #ifdef PG_FLUSH_DATA_WORKS static int -pre_sync_fname(const char *fname, bool isdir) +pre_sync_fname(const char *fname, bool isdir, void *arg) { int fd; @@ -373,7 +373,7 @@ pre_sync_fname(const char *fname, bool isdir) * are fatal. */ int -fsync_fname(const char *fname, bool isdir) +fsync_fname(const char *fname, bool isdir, void *arg) { int fd; int flags; @@ -444,7 +444,7 @@ fsync_parent_path(const char *fname) if (strlen(parentpath) == 0) strlcpy(parentpath, ".", MAXPGPATH); - if (fsync_fname(parentpath, true) != 0) + if (fsync_fname(parentpath, true, NULL) != 0) return -1; return 0; @@ -467,7 +467,7 @@ durable_rename(const char *oldfile, const char *newfile) * because it's then guaranteed that either source or target file exists * after a crash. */ - if (fsync_fname(oldfile, false) != 0) + if (fsync_fname(oldfile, false, NULL) != 0) return -1; fd = open(newfile, PG_BINARY | O_RDWR, 0); @@ -502,7 +502,7 @@ durable_rename(const char *oldfile, const char *newfile) * To guarantee renaming the file is persistent, fsync the file with its * new name, and its containing directory. */ - if (fsync_fname(newfile, false) != 0) + if (fsync_fname(newfile, false, NULL) != 0) return -1; if (fsync_parent_path(newfile) != 0) diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 5a9519acfe..c328f56a85 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -33,15 +33,15 @@ typedef enum DataDirSyncMethod struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND -extern int fsync_fname(const char *fname, bool isdir); +extern int fsync_fname(const char *fname, bool isdir, void *arg); extern void sync_pgdata(const char *pg_data, int serverVersion, DataDirSyncMethod sync_method); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); extern void walkdir(const char *path, - int (*action) (const char *fname, bool isdir), - bool process_symlinks); + int (*action) (const char *fname, bool isdir, void *arg), + bool process_symlinks, void *arg); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) [text/plain] v1-0003-Introduce-catalog-swap-mode-for-pg_upgrade.patch (9.5K, ../../Zyvop-LxLXBLrZil@nathan/4-v1-0003-Introduce-catalog-swap-mode-for-pg_upgrade.patch) download | inline diff: From c248059f7276578f5e8abd00ea9efb007423f94d Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 16:38:19 -0600 Subject: [PATCH v1 3/8] Introduce catalog-swap mode for pg_upgrade. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This new mode moves the database directories from the old cluster to the new cluster and then swaps the pg_restore-generated catalog files in place. This can significantly increase the length of the following data synchronization step (due to the large number of unsynchronized pg_restore-generated files), but this problem will be handled in follow-up commits. --- src/bin/pg_upgrade/check.c | 2 + src/bin/pg_upgrade/option.c | 5 + src/bin/pg_upgrade/pg_upgrade.h | 1 + src/bin/pg_upgrade/relfilenumber.c | 167 +++++++++++++++++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 5 files changed, 176 insertions(+) diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 94164f0472..a4bb365718 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -711,6 +711,8 @@ check_new_cluster(void) case TRANSFER_MODE_LINK: check_hard_link(); break; + case TRANSFER_MODE_CATALOG_SWAP: + break; } check_is_install_user(&new_cluster); diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index 6f41d63eed..64091a54c4 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -60,6 +60,7 @@ parseCommandLine(int argc, char *argv[]) {"copy", no_argument, NULL, 2}, {"copy-file-range", no_argument, NULL, 3}, {"sync-method", required_argument, NULL, 4}, + {"catalog-swap", no_argument, NULL, 5}, {NULL, 0, NULL, 0} }; @@ -212,6 +213,10 @@ parseCommandLine(int argc, char *argv[]) user_opts.sync_method = pg_strdup(optarg); break; + case 5: + user_opts.transfer_mode = TRANSFER_MODE_CATALOG_SWAP; + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index 53f693c2d4..19cb5a011e 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -256,6 +256,7 @@ typedef enum TRANSFER_MODE_COPY, TRANSFER_MODE_COPY_FILE_RANGE, TRANSFER_MODE_LINK, + TRANSFER_MODE_CATALOG_SWAP, } transferMode; /* diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 07baa49a02..9d8fce3c4a 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -11,11 +11,21 @@ #include <sys/stat.h> +#include "common/file_perm.h" +#include "common/file_utils.h" +#include "common/int.h" +#include "fe_utils/option_utils.h" #include "pg_upgrade.h" static void transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace); static void transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_frozenbit); +typedef struct move_catalog_file_context +{ + FileNameMap *maps; + int size; + char *target; +} move_catalog_file_context; /* * transfer_all_new_tablespaces() @@ -41,6 +51,9 @@ transfer_all_new_tablespaces(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, case TRANSFER_MODE_LINK: prep_status_progress("Linking user relation files"); break; + case TRANSFER_MODE_CATALOG_SWAP: + prep_status_progress("Swapping catalog files"); + break; } /* @@ -127,6 +140,144 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, } } +static int +FileNameMapCmp(const void *a, const void *b) +{ + return pg_cmp_u32(((const FileNameMap *) a)->relfilenumber, + ((const FileNameMap *) b)->relfilenumber); +} + +static RelFileNumber +parse_relfilenumber(const char *filename) +{ + char *endp; + unsigned long n; + + if (filename[0] < '1' || filename[0] > '9') + return InvalidRelFileNumber; + + errno = 0; + n = strtoul(filename, &endp, 10); + if (errno || filename == endp || n <= 0 || n > PG_UINT32_MAX) + return InvalidRelFileNumber; + + return (RelFileNumber) n; +} + +static int +move_catalog_file(const char *fname, bool isdir, void *arg) +{ + char dst[MAXPGPATH]; + const char *filename = last_dir_separator(fname) + 1; + RelFileNumber rfn = parse_relfilenumber(filename); + move_catalog_file_context *context = (move_catalog_file_context *) arg; + + /* + * XXX: Is this right? AFAICT we don't really expect there to be + * directories within database directories, so perhaps it would be better + * to either unconditionally rename or to fail. Further investigation is + * required. + */ + if (isdir) + return 0; + + if (RelFileNumberIsValid(rfn)) + { + FileNameMap key; + + key.relfilenumber = (RelFileNumber) rfn; + if (bsearch(&key, context->maps, context->size, + sizeof(FileNameMap), FileNameMapCmp)) + return 0; + } + + snprintf(dst, sizeof(dst), "%s/%s", context->target, filename); + if (rename(fname, dst) != 0) + pg_fatal("could not rename \"%s\" to \"%s\": %m", fname, dst); + + return 0; +} + +/* + * XXX: This proof-of-concept patch doesn't yet handle non-default tablespaces. + */ +static void +do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) +{ + char old_tblspc[MAXPGPATH]; + char new_tblspc[MAXPGPATH]; + char old_dat[MAXPGPATH]; + char new_dat[MAXPGPATH]; + char moved_tblspc[MAXPGPATH]; + char moved_dat[MAXPGPATH]; + char old_cat[MAXPGPATH]; + move_catalog_file_context context; + DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; + + parse_sync_method(user_opts.sync_method, &sync_method); + + snprintf(old_tblspc, sizeof(old_tblspc), "%s%s", + maps[0].old_tablespace, maps[0].old_tablespace_suffix); + snprintf(new_tblspc, sizeof(new_tblspc), "%s%s", + maps[0].new_tablespace, maps[0].new_tablespace_suffix); + snprintf(old_dat, sizeof(old_dat), "%s/%u", old_tblspc, maps[0].db_oid); + snprintf(new_dat, sizeof(new_dat), "%s/%u", new_tblspc, maps[0].db_oid); + snprintf(moved_tblspc, sizeof(moved_tblspc), "%s_moved", old_tblspc); + snprintf(moved_dat, sizeof(moved_dat), "%s/%u", + moved_tblspc, maps[0].db_oid); + snprintf(old_cat, sizeof(old_cat), "%s/%u_old_cat", + moved_tblspc, maps[0].db_oid); + + qsort(maps, size, sizeof(FileNameMap), FileNameMapCmp); + + /* create dir for stuff that is moved aside */ + if (pg_mkdir_p(moved_tblspc, pg_dir_create_mode) && errno != EEXIST) + pg_fatal("could not create directory \"%s\": %m", moved_tblspc); + + /* move new cluster data dir aside */ + if (rename(new_dat, moved_dat)) + pg_fatal("could not rename \"%s\" to \"%s\": %m", new_dat, moved_dat); + + /* move old cluster data dir in place */ + if (rename(old_dat, new_dat)) + pg_fatal("could not rename \"%s\" to \"%s\": %m", old_dat, new_dat); + + /* create dir for old catalogs */ + if (pg_mkdir_p(old_cat, pg_dir_create_mode)) + pg_fatal("could not create directory \"%s\": %m", old_cat); + + /* move catalogs in new data dir aside */ + context.maps = maps; + context.size = size; + context.target = old_cat; + walkdir(new_dat, move_catalog_file, false, &context); + + /* move catalogs in moved-aside data dir in place */ + context.target = new_dat; + walkdir(moved_dat, move_catalog_file, false, &context); + + /* no need to sync things individually if we are going to syncfs() later */ + if (sync_method == DATA_DIR_SYNC_METHOD_SYNCFS) + return; + + /* fsync directory entries */ + if (fsync_fname(moved_dat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", moved_dat); + if (fsync_fname(old_cat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", old_cat); + + /* + * XXX: We could instead fsync() these directories once at the end instead + * of once per-database, but it doesn't affect performance meaningfully, + * and this is just a proof-of-concept patch, so I haven't bothered doing + * the required refactoring yet. + */ + if (fsync_fname(old_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", old_tblspc); + if (fsync_fname(moved_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", moved_tblspc); +} + /* * transfer_single_new_db() * @@ -145,6 +296,18 @@ transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace) new_cluster.controldata.cat_ver >= VISIBILITY_MAP_FROZEN_BIT_CAT_VER) vm_must_add_frozenbit = true; + /* + * XXX: In catalog-swap mode, vm_must_add_frozenbit isn't handled yet. We + * could either disallow using catalog-swap mode if the upgrade involves + * versions older than v9.6, or we could add code to handle rewriting the + * visibility maps in this mode (like the other modes do). + */ + if (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) + { + do_catalog_transfer(maps, size, old_tablespace); + return; + } + for (mapnum = 0; mapnum < size; mapnum++) { if (old_tablespace == NULL || @@ -259,6 +422,10 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"", old_file, new_file); linkFile(old_file, new_file, map->nspname, map->relname); + break; + case TRANSFER_MODE_CATALOG_SWAP: + pg_fatal("should never happen"); + break; } } } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 1847bbfa95..58c339af85 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3649,6 +3649,7 @@ mix_data_t mixedStruct mode_t movedb_failure_params +move_catalog_file_context multirange_bsearch_comparison multirange_unnest_fctx mxact -- 2.39.5 (Apple Git-154) [text/plain] v1-0004-Add-no-sync-data-files-flag-to-initdb.patch (7.3K, ../../Zyvop-LxLXBLrZil@nathan/5-v1-0004-Add-no-sync-data-files-flag-to-initdb.patch) download | inline diff: From 1fd29fa00c777b2c04683394f54261b446e46a61 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 5 Nov 2024 16:47:42 -0600 Subject: [PATCH v1 4/8] Add --no-sync-data-files flag to initdb. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This new mode caused 'initdb --sync-only' to synchronize everything except for the database directories. It will be used in a follow-up commit that aims to reduce the duration of the data synchronization step in pg_upgrade's catalog-swap mode. --- src/bin/initdb/initdb.c | 9 ++++-- src/bin/pg_basebackup/pg_basebackup.c | 2 +- src/bin/pg_checksums/pg_checksums.c | 2 +- src/bin/pg_combinebackup/pg_combinebackup.c | 2 +- src/bin/pg_rewind/file_ops.c | 2 +- src/common/file_utils.c | 35 ++++++++++++++++----- src/include/common/file_utils.h | 3 +- 7 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 9a91830783..53c6e86a80 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -168,6 +168,7 @@ static bool data_checksums = true; static char *xlog_dir = NULL; static int wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024); static DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC; +static bool sync_data_files = true; /* internal vars */ @@ -3183,6 +3184,7 @@ main(int argc, char *argv[]) {"icu-rules", required_argument, NULL, 18}, {"sync-method", required_argument, NULL, 19}, {"no-data-checksums", no_argument, NULL, 20}, + {"no-sync-data-files", no_argument, NULL, 21}, {NULL, 0, NULL, 0} }; @@ -3377,6 +3379,9 @@ main(int argc, char *argv[]) case 20: data_checksums = false; break; + case 21: + sync_data_files = false; + break; default: /* getopt_long already emitted a complaint */ pg_log_error_hint("Try \"%s --help\" for more information.", progname); @@ -3428,7 +3433,7 @@ main(int argc, char *argv[]) fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); return 0; } @@ -3491,7 +3496,7 @@ main(int argc, char *argv[]) { fputs(_("syncing data to disk ... "), stdout); fflush(stdout); - sync_pgdata(pg_data, PG_VERSION_NUM, sync_method); + sync_pgdata(pg_data, PG_VERSION_NUM, sync_method, sync_data_files); check_ok(); } else diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index e41a6cfbda..43526e3246 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2310,7 +2310,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail, } else { - (void) sync_pgdata(basedir, serverVersion, sync_method); + (void) sync_pgdata(basedir, serverVersion, sync_method, true); } } diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index b86bc417c9..06ccaacfda 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -633,7 +633,7 @@ main(int argc, char *argv[]) if (do_sync) { pg_log_info("syncing data directory"); - sync_pgdata(DataDir, PG_VERSION_NUM, sync_method); + sync_pgdata(DataDir, PG_VERSION_NUM, sync_method, true); } pg_log_info("updating control file"); diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c index 5f1f62f1db..80a137be4e 100644 --- a/src/bin/pg_combinebackup/pg_combinebackup.c +++ b/src/bin/pg_combinebackup/pg_combinebackup.c @@ -420,7 +420,7 @@ main(int argc, char *argv[]) else { pg_log_debug("recursively fsyncing \"%s\"", opt.output); - sync_pgdata(opt.output, version * 10000, opt.sync_method); + sync_pgdata(opt.output, version * 10000, opt.sync_method, true); } } diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index 67a86bb4c5..ceb1c3ac6d 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -296,7 +296,7 @@ sync_target_dir(void) if (!do_sync || dry_run) return; - sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method); + sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method, true); } diff --git a/src/common/file_utils.c b/src/common/file_utils.c index dc90f35ae1..65cdf07ae7 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -94,7 +94,8 @@ do_syncfs(const char *path) void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method) + DataDirSyncMethod sync_method, + bool sync_data_files) { bool xlog_is_symlink; char pg_wal[MAXPGPATH]; @@ -184,10 +185,11 @@ sync_pgdata(const char *pg_data, * fsync the data directory and its contents. */ #ifdef PG_FLUSH_DATA_WORKS - walkdir(pg_data, pre_sync_fname, false, NULL); + walkdir(pg_data, pre_sync_fname, false, &sync_data_files); if (xlog_is_symlink) - walkdir(pg_wal, pre_sync_fname, false, NULL); - walkdir(pg_tblspc, pre_sync_fname, true, NULL); + walkdir(pg_wal, pre_sync_fname, false, &sync_data_files); + if (sync_data_files) + walkdir(pg_tblspc, pre_sync_fname, true, NULL); #endif /* @@ -200,10 +202,11 @@ sync_pgdata(const char *pg_data, * get fsync'd twice. That's not an expected case so we don't * worry about optimizing it. */ - walkdir(pg_data, fsync_fname, false, NULL); + walkdir(pg_data, fsync_fname, false, &sync_data_files); if (xlog_is_symlink) - walkdir(pg_wal, fsync_fname, false, NULL); - walkdir(pg_tblspc, fsync_fname, true, NULL); + walkdir(pg_wal, fsync_fname, false, &sync_data_files); + if (sync_data_files) + walkdir(pg_tblspc, fsync_fname, true, NULL); } break; } @@ -296,7 +299,23 @@ walkdir(const char *path, (*action) (subpath, false, arg); break; case PGFILETYPE_DIR: - walkdir(subpath, action, false, arg); + + /* + * XXX: Checking here for the "sync_data_files" case is quite + * hacky, but it's not clear how to do better. Another option + * would be to send "de" down to the function, but that would + * introduce a huge number of function pointer calls and + * directory reads that we are trying to avoid. + */ +#ifdef PG_FLUSH_DATA_WORKS + if ((action != pre_sync_fname && action != fsync_fname) || +#else + if (action != fsync_fname || +#endif + !arg || *((bool *) arg) || + strcmp(de->d_name, "base") != 0) + walkdir(subpath, action, false, arg); + break; default: diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index c328f56a85..3743caa63e 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -35,7 +35,8 @@ struct iovec; /* avoid including port/pg_iovec.h here */ #ifdef FRONTEND extern int fsync_fname(const char *fname, bool isdir, void *arg); extern void sync_pgdata(const char *pg_data, int serverVersion, - DataDirSyncMethod sync_method); + DataDirSyncMethod sync_method, + bool sync_data_files); extern void sync_dir_recurse(const char *dir, DataDirSyncMethod sync_method); extern int durable_rename(const char *oldfile, const char *newfile); extern int fsync_parent_path(const char *fname); -- 2.39.5 (Apple Git-154) [text/plain] v1-0005-Export-pre_sync_fname.patch (2.4K, ../../Zyvop-LxLXBLrZil@nathan/6-v1-0005-Export-pre_sync_fname.patch) download | inline diff: From e70eea50e81d8f40fb8db15c06b23305d4b8698f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 09:52:19 -0600 Subject: [PATCH v1 5/8] Export pre_sync_fname(). THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. A follow-up commit will use this function to alert the file system that we want a file's data on disk so that subsequent calls to fsync() are faster. --- src/common/file_utils.c | 18 +++++------------- src/include/common/file_utils.h | 1 + 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/common/file_utils.c b/src/common/file_utils.c index 65cdf07ae7..5c201ec6e8 100644 --- a/src/common/file_utils.c +++ b/src/common/file_utils.c @@ -45,10 +45,6 @@ */ #define MINIMUM_VERSION_FOR_PG_WAL 100000 -#ifdef PG_FLUSH_DATA_WORKS -static int pre_sync_fname(const char *fname, bool isdir, void *arg); -#endif - #ifdef HAVE_SYNCFS /* @@ -307,11 +303,7 @@ walkdir(const char *path, * introduce a huge number of function pointer calls and * directory reads that we are trying to avoid. */ -#ifdef PG_FLUSH_DATA_WORKS if ((action != pre_sync_fname && action != fsync_fname) || -#else - if (action != fsync_fname || -#endif !arg || *((bool *) arg) || strcmp(de->d_name, "base") != 0) walkdir(subpath, action, false, arg); @@ -348,11 +340,12 @@ walkdir(const char *path, * Ignores errors trying to open unreadable files, and reports other errors * non-fatally. */ -#ifdef PG_FLUSH_DATA_WORKS - -static int +int pre_sync_fname(const char *fname, bool isdir, void *arg) { +#ifndef PG_FLUSH_DATA_WORKS + return 0; +#else int fd; fd = open(fname, O_RDONLY | PG_BINARY, 0); @@ -380,9 +373,8 @@ pre_sync_fname(const char *fname, bool isdir, void *arg) (void) close(fd); return 0; -} - #endif /* PG_FLUSH_DATA_WORKS */ +} /* * fsync_fname -- Try to fsync a file or directory diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h index 3743caa63e..e7a34d4c4e 100644 --- a/src/include/common/file_utils.h +++ b/src/include/common/file_utils.h @@ -43,6 +43,7 @@ extern int fsync_parent_path(const char *fname); extern void walkdir(const char *path, int (*action) (const char *fname, bool isdir, void *arg), bool process_symlinks, void *arg); +extern int pre_sync_fname(const char *fname, bool isdir, void *arg); #endif extern PGFileType get_dirent_type(const char *path, -- 2.39.5 (Apple Git-154) [text/plain] v1-0006-In-pg_upgrade-s-catalog-swap-mode-only-sync-files.patch (4.4K, ../../Zyvop-LxLXBLrZil@nathan/7-v1-0006-In-pg_upgrade-s-catalog-swap-mode-only-sync-files.patch) download | inline diff: From 5d17fd66e08612574ffe0c39ff7624259319059c Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:40:43 -0600 Subject: [PATCH v1 6/8] In pg_upgrade's catalog-swap mode, only sync files as necessary. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. In this mode, it can be much faster to use "--sync-method fsync", which now skips synchronizing data files moved from the old cluster (which we assumed were synchronized before pg_upgrade). --- src/bin/pg_upgrade/pg_upgrade.c | 6 ++-- src/bin/pg_upgrade/relfilenumber.c | 52 ++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 663235816f..f5946ac89a 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -210,10 +210,12 @@ main(int argc, char **argv) { prep_status("Sync data directory to disk"); exec_prog(UTILITY_LOG_FILE, NULL, true, true, - "\"%s/initdb\" --sync-only \"%s\" --sync-method %s", + "\"%s/initdb\" --sync-only \"%s\" --sync-method %s %s", new_cluster.bindir, new_cluster.pgdata, - user_opts.sync_method); + user_opts.sync_method, + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + "--no-sync-data-files" : ""); check_ok(); } diff --git a/src/bin/pg_upgrade/relfilenumber.c b/src/bin/pg_upgrade/relfilenumber.c index 9d8fce3c4a..dcca4bb2e7 100644 --- a/src/bin/pg_upgrade/relfilenumber.c +++ b/src/bin/pg_upgrade/relfilenumber.c @@ -25,8 +25,49 @@ typedef struct move_catalog_file_context FileNameMap *maps; int size; char *target; + bool sync_moved; } move_catalog_file_context; +#define SYNC_QUEUE_MAX_LEN (1024) + +static char *sync_queue[SYNC_QUEUE_MAX_LEN]; +static bool sync_queue_inited; +static int sync_queue_len; + +static inline void +sync_queue_init(void) +{ + if (sync_queue_inited) + return; + + sync_queue_inited = true; + for (int i = 0; i < SYNC_QUEUE_MAX_LEN; i++) + sync_queue[i] = palloc(MAXPGPATH); +} + +static inline void +sync_queue_sync_all(void) +{ + if (!sync_queue_inited) + return; + + for (int i = 0; i < sync_queue_len; i++) + fsync_fname(sync_queue[i], false, NULL); + sync_queue_len = 0; +} + +static inline void +sync_queue_push(const char *fname) +{ + sync_queue_init(); + + pre_sync_fname(fname, false, NULL); + + strncpy(sync_queue[sync_queue_len++], fname, MAXPGPATH); + if (sync_queue_len >= SYNC_QUEUE_MAX_LEN) + sync_queue_sync_all(); +} + /* * transfer_all_new_tablespaces() * @@ -138,6 +179,8 @@ transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, /* We allocate something even for n_maps == 0 */ pg_free(mappings); } + + sync_queue_sync_all(); } static int @@ -195,6 +238,9 @@ move_catalog_file(const char *fname, bool isdir, void *arg) if (rename(fname, dst) != 0) pg_fatal("could not rename \"%s\" to \"%s\": %m", fname, dst); + if (context->sync_moved) + sync_queue_push(dst); + return 0; } @@ -250,10 +296,12 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) context.maps = maps; context.size = size; context.target = old_cat; + context.sync_moved = false; walkdir(new_dat, move_catalog_file, false, &context); /* move catalogs in moved-aside data dir in place */ context.target = new_dat; + context.sync_moved = (sync_method != DATA_DIR_SYNC_METHOD_SYNCFS); walkdir(moved_dat, move_catalog_file, false, &context); /* no need to sync things individually if we are going to syncfs() later */ @@ -265,6 +313,8 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) pg_fatal("could not synchronize directory \"%s\": %m", moved_dat); if (fsync_fname(old_cat, true, NULL) != 0) pg_fatal("could not synchronize directory \"%s\": %m", old_cat); + if (fsync_fname(new_dat, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_dat); /* * XXX: We could instead fsync() these directories once at the end instead @@ -276,6 +326,8 @@ do_catalog_transfer(FileNameMap *maps, int size, char *old_tablespace) pg_fatal("could not synchronize directory \"%s\": %m", old_tblspc); if (fsync_fname(moved_tblspc, true, NULL) != 0) pg_fatal("could not synchronize directory \"%s\": %m", moved_tblspc); + if (fsync_fname(new_tblspc, true, NULL) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", new_tblspc); } /* -- 2.39.5 (Apple Git-154) [text/plain] v1-0007-Add-sequence-data-flag-to-pg_dump.patch (2.8K, ../../Zyvop-LxLXBLrZil@nathan/8-v1-0007-Add-sequence-data-flag-to-pg_dump.patch) download | inline diff: From 234d40f4e45d56f40465037dcf83c8c980edf095 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:46:11 -0600 Subject: [PATCH v1 7/8] Add --sequence-data flag to pg_dump. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. This flag can be used to optionally dump the sequence data even when --schema-only is used. It is primarily intended for use in a follow-up commit that will cause sequence data files to be carried over from the old cluster in pg_upgrade's new catalog-swap mode. --- src/bin/pg_dump/pg_dump.c | 9 +-------- src/bin/pg_upgrade/dump.c | 2 +- src/test/modules/test_pg_dump/t/001_base.pl | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index b2f4eb2c6d..2b57abd305 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -501,6 +501,7 @@ main(int argc, char **argv) {"sync-method", required_argument, NULL, 15}, {"filter", required_argument, NULL, 16}, {"exclude-extension", required_argument, NULL, 17}, + {"sequence-data", no_argument, &dopt.sequence_data, 1}, {NULL, 0, NULL, 0} }; @@ -768,14 +769,6 @@ main(int argc, char **argv) if (dopt.column_inserts && dopt.dump_inserts == 0) dopt.dump_inserts = DUMP_DEFAULT_ROWS_PER_INSERT; - /* - * Binary upgrade mode implies dumping sequence data even in schema-only - * mode. This is not exposed as a separate option, but kept separate - * internally for clarity. - */ - if (dopt.binary_upgrade) - dopt.sequence_data = 1; - if (dopt.dataOnly && dopt.schemaOnly) pg_fatal("options -s/--schema-only and -a/--data-only cannot be used together"); diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 8345f55be8..8453722833 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -53,7 +53,7 @@ generate_old_dump(void) parallel_exec_prog(log_file_name, NULL, "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers " - "--binary-upgrade --format=custom %s --no-sync --file=\"%s/%s\" %s", + "--binary-upgrade --sequence-data --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), log_opts.verbose ? "--verbose" : "", log_opts.dumpdir, diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl index e2579e29cd..46231c93f1 100644 --- a/src/test/modules/test_pg_dump/t/001_base.pl +++ b/src/test/modules/test_pg_dump/t/001_base.pl @@ -48,7 +48,7 @@ my %pgdump_runs = ( dump_cmd => [ 'pg_dump', '--no-sync', "--file=$tempdir/binary_upgrade.sql", '--schema-only', - '--binary-upgrade', '--dbname=postgres', + '--binary-upgrade', '--sequence-data', '--dbname=postgres', ], }, clean => { -- 2.39.5 (Apple Git-154) [text/plain] v1-0008-Avoid-copying-sequence-files-in-pg_upgrade-s-cata.patch (3.1K, ../../Zyvop-LxLXBLrZil@nathan/9-v1-0008-Avoid-copying-sequence-files-in-pg_upgrade-s-cata.patch) download | inline diff: From 51be6c09256272e1ce0360b5376b4a14cd1d9a61 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 6 Nov 2024 10:53:40 -0600 Subject: [PATCH v1 8/8] Avoid copying sequence files in pg_upgrade's catalog-swap mode. THIS IS A PROOF OF CONCEPT AND IS NOT READY FOR SERIOUS REVIEW. On clusters with many sequences, this can further reduce the amount of time required to wire up the data files in the new cluster. If the sequence data file format changes, this optimization cannot be used, but that seems rare enough. --- src/bin/pg_upgrade/dump.c | 8 +++++++- src/bin/pg_upgrade/info.c | 6 +++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_upgrade/dump.c b/src/bin/pg_upgrade/dump.c index 8453722833..d5a81cc29c 100644 --- a/src/bin/pg_upgrade/dump.c +++ b/src/bin/pg_upgrade/dump.c @@ -51,10 +51,16 @@ generate_old_dump(void) snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid); snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid); + /* + * XXX: We need to be sure that the sequence data format hasn't + * changed. + */ parallel_exec_prog(log_file_name, NULL, "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers " - "--binary-upgrade --sequence-data --format=custom %s --no-sync --file=\"%s/%s\" %s", + "--binary-upgrade %s --format=custom %s --no-sync --file=\"%s/%s\" %s", new_cluster.bindir, cluster_conn_opts(&old_cluster), + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + "" : "--sequence-data", log_opts.verbose ? "--verbose" : "", log_opts.dumpdir, sql_file_name, escaped_connstr.data); diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index f83ded89cb..786d17e32f 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -483,6 +483,8 @@ get_rel_infos_query(void) * pg_largeobject contains user data that does not appear in pg_dump * output, so we have to copy that system table. It's easiest to do that * by treating it as a user table. + * + * XXX: We need to be sure that the sequence data format hasn't changed. */ appendPQExpBuffer(&query, "WITH regular_heap (reloid, indtable, toastheap) AS ( " @@ -490,7 +492,7 @@ get_rel_infos_query(void) " FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n " " ON c.relnamespace = n.oid " " WHERE relkind IN (" CppAsString2(RELKIND_RELATION) ", " - CppAsString2(RELKIND_MATVIEW) ") AND " + CppAsString2(RELKIND_MATVIEW) "%s) AND " /* exclude possible orphaned temp tables */ " ((n.nspname !~ '^pg_temp_' AND " " n.nspname !~ '^pg_toast_temp_' AND " @@ -499,6 +501,8 @@ get_rel_infos_query(void) " c.oid >= %u::pg_catalog.oid) OR " " (n.nspname = 'pg_catalog' AND " " relname IN ('pg_largeobject') ))), ", + (user_opts.transfer_mode == TRANSFER_MODE_CATALOG_SWAP) ? + ", " CppAsString2(RELKIND_SEQUENCE) : "", FirstNormalObjectId); /* -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2024-11-17 18:50 Greg Sabino Mullane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Greg Sabino Mullane @ 2024-11-17 18:50 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: pgsql-hackers On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <[email protected]> wrote: > Therefore, it can be much faster to instead move the entire data directory > from the old cluster > to the new cluster and to then swap the catalog relation files. > Thank you for breaking this up so clearly into separate commits. I think it is a very interesting idea, and anything to speed up pg_upgrade is always welcome. Some minor thoughts: > [PATCH v1 3/8] Introduce catalog-swap mode for pg_upgrade. > .. we don't really expect there to be directories within database directories, > so perhaps it would be better to either unconditionally rename or to fail. Failure seems the best option here, so we can cleanly handle any future cases in which we decide to put dirs in this directory. > if (RelFileNumberIsValid(rfn)) > { > FileNameMap key; > > key.relfilenumber = (RelFileNumber) rfn; > if (bsearch(&key, context->maps, context->size, > sizeof(FileNameMap), FileNameMapCmp)) > return 0; > } > > snprintf(dst, sizeof(dst), "%s/%s", context->target, filename); > if (rename(fname, dst) != 0) I'm not quite clear what we are doing here with falling through for InvalidOid entries, could you explain? > .. vm_must_add_frozenbit isn't handled yet. We could either disallow > using catalog-swap mode if the upgrade involves versions older than v9.6 Yes, this. No need for more code to handle super old versions when other options exist. with this problem is to introduce a special mode for "initdb --sync-only" > that calls fsync() for everything _except_ the actual data files. If we > fsync() the new catalog files as we move them into place, and if we assume > that the old catalog files will have been properly synchronized before > upgrading, there's no reason to synchronize them again at the end. > Very cool approach! Cheers, Greg ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: optimize file transfer in pg_upgrade @ 2024-11-19 18:20 Nathan Bossart <[email protected]> parent: Greg Sabino Mullane <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Nathan Bossart @ 2024-11-19 18:20 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: pgsql-hackers On Sun, Nov 17, 2024 at 01:50:53PM -0500, Greg Sabino Mullane wrote: > On Wed, Nov 6, 2024 at 5:07 PM Nathan Bossart <[email protected]> > wrote: >> Therefore, it can be much faster to instead move the entire data directory >> from the old cluster >> to the new cluster and to then swap the catalog relation files. > > Thank you for breaking this up so clearly into separate commits. I think it > is a very interesting idea, and anything to speed up pg_upgrade is always > welcome. Some minor thoughts: Thank you for reviewing! >> .. we don't really expect there to be directories within database > directories, >> so perhaps it would be better to either unconditionally rename or to fail. > Failure seems the best option here, so we can cleanly handle any future > cases in which we decide to put dirs in this directory. Good point. >> if (RelFileNumberIsValid(rfn)) >> { >> FileNameMap key; >> >> key.relfilenumber = (RelFileNumber) rfn; >> if (bsearch(&key, context->maps, context->size, >> sizeof(FileNameMap), FileNameMapCmp)) >> return 0; >> } >> >> snprintf(dst, sizeof(dst), "%s/%s", context->target, filename); >> if (rename(fname, dst) != 0) > > I'm not quite clear what we are doing here with falling through > for InvalidOid entries, could you explain? The idea is that if it looks like a data file that we might want to transfer (i.e., it starts with a RelFileNumber), we should consult our map to determine whether to move it. Otherwise, we want to unconditionally transfer it so that we always use the files generated during pg_restore in the new cluster (e.g., PG_VERSION and pg_filenode.map). In theory, this should result in the same end state as what --link mode does today (for the new cluster, at least). >> .. vm_must_add_frozenbit isn't handled yet. We could either disallow >> using catalog-swap mode if the upgrade involves versions older than v9.6 > > Yes, this. No need for more code to handle super old versions when other > options exist. I'm inclined to agree. >> with this problem is to introduce a special mode for "initdb --sync-only" >> that calls fsync() for everything _except_ the actual data files. If we >> fsync() the new catalog files as we move them into place, and if we assume >> that the old catalog files will have been properly synchronized before >> upgrading, there's no reason to synchronize them again at the end. > > Very cool approach! :) -- nathan ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-11-19 18:20 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-03 22:36 [PATCH 2/2] doc review Justin Pryzby <[email protected]> 2024-11-06 22:07 optimize file transfer in pg_upgrade Nathan Bossart <[email protected]> 2024-11-17 18:50 ` Re: optimize file transfer in pg_upgrade Greg Sabino Mullane <[email protected]> 2024-11-19 18:20 ` Re: optimize file transfer in pg_upgrade 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