On Mon, Jul 21, 2025 at 12:24 PM Dimitrios Apostolou <ji...@gmx.net> wrote:
> > > > FWIW I implemented a pg_restore --freeze patch, see attached. It needs > > another patch of mine from [1] that implements pg_restore --data-only > > --clean, which for parallel restores encases each COPY in its own > transaction > > and prepends it with a TRUNCATE. All feedback is welcome. > > > > [1] > https://www.postgresql.org/message-id/c61263f2-7472-5dd8-703d-01e683421f61%40gmx.net > > > > It works really fast for the data, and I see that some, but not all > items > > from section=post-data, start parallel plans. For example I see CREATE > INDEX > > spawns parallel workers. > > I added it to July's commitfest, mostly to trigger discussion around the > issue. > > https://commitfest.postgresql.org/patch/5826/ > > Not sure how to mark on the commitfest page that the patch requires > another patch from another commitfest entry. > > > Dimitris > > > > Hello Dimitris and fellow hackers, I'm And Warda(currently unavailable :( ) picking this patch up from the July CommitFest for review. Thanks, Dimitris, for submitting it and for tackling this well-known performance issue with restoring very large databases. The problem of `ALTER TABLE ... ADD FOREIGN KEY` taking days after a restore due to the lack of a visibility map is a significant pain point, and your proposed solution is a logical step. I've reviewed the patch and the discussion in the thread. I've attached a rebased version of the patch against the current master. The core idea of adding a `--freeze` option to `pg_restore` to leverage `COPY ... WITH (FREEZE)` is sound. It directly addresses the problem of needing a very long `VACUUM` run after data loading to make subsequent `post-data` steps, like foreign key validation, performant. The benefit isn't just a minor speedup in the `COPY` itself but the major time-saving of avoiding a full-table `VACUUM` on a massive dataset. I ran a simple performance script based on your patch. On a small test case, I saw a consistent 10-15% speedup during the data-loading phase. While this is encouraging, the main benefit, as noted, is in the steps that follow. === Summary === freeze average: 1.205 seconds nofreeze average: 1.324 seconds ~/.pg-vanilla/bin ᐅ ./tst.sh === Summary === freeze average: 1.362 seconds nofreeze average: 1.480 seconds #!/bin/bash set -euo pipefail DUMP_FILE="parttest.dump" DB_PREFIX="parttest" JOBS=$(nproc) ROUNDS=3 timings() { local mode=$1 local total_time=0 for i in $(seq 1 $ROUNDS); do DB_NAME="${DB_PREFIX}_${mode}_$i" psql -q -c "DROP DATABASE IF EXISTS $DB_NAME" psql -q -c "CREATE DATABASE $DB_NAME" start_time=$(date +%s.%N) if [ "$mode" == "freeze" ]; then pg_restore -d "$DB_NAME" -j "$JOBS" --clean --if-exists --freeze "$DUMP_FILE" > /dev/null else pg_restore -d "$DB_NAME" -j "$JOBS" --clean --if-exists "$DUMP_FILE" > /dev/null fi end_time=$(date +%s.%N) duration=$(echo "$end_time - $start_time" | bc) total_time=$(echo "$total_time + $duration" | bc) done average=$(echo "scale=3; $total_time / $ROUNDS" | bc) echo "$mode average: $average seconds" } freeze_avg=$(timings freeze) nofreeze_avg=$(timings nofreeze) echo "" echo "=== Summary ===" echo "$freeze_avg" echo "$nofreeze_avg" The main area of concern is the implementation method in `pg_backup_archiver.c`. The patch introduces a `do_copy` function that modifies the `COPY` statement string to inject the `WITH (FREEZE)` option. ```c if (cp_end - 10 > cp && strncmp(cp_end - 10, "FROM stdin", 10) == 0 ) cp_is_well_formed = true; ``` This approach of string parsing is quite fragile. It makes a strong assumption that the `COPY` statement generated by `pg_dump` will always end with `... FROM stdin;` (preceded by optional whitespace). While this is true now, it creates a tight and brittle coupling between `pg_dump`'s output format and `pg_restore`'s parsing logic. Any future changes to `pg_dump` that might add options or slightly alter the `COPY` syntax could break this feature silently or with a non-obvious warning. A more robust solution would be to avoid string manipulation entirely. `pg_restore` should assemble the `COPY` command from its constituent parts (table name, column list, etc.) and then conditionally append the `WITH (FREEZE)` clause before the final `FROM stdin;`. This would decouple the feature from the exact string representation in the dump archive. An alternative—and arguably cleaner—approach might be to shift this logic to pg_dump. One important consideration that needs to be highlighted in the documentation for this feature is the impact on WAL generation. `COPY ... WITH (FREEZE)` is known to generate significantly more WAL traffic because it must log the freezing of tuples, which can be a surprise for users. Maybe we can insert already frozen pages? Additionally, it should be noted that the freeze option only works correctly when performing a fresh load of data into empty tables. Thanks again for your work on this. Regards, Stepan Neretin
From 912a0cb3b31d4884fd6b2ae8869b96483d620641 Mon Sep 17 00:00:00 2001 From: Stepan Neretin <slp...@gmail.com> Date: Mon, 21 Jul 2025 12:22:02 +0700 Subject: [PATCH] pg_restore --freeze pg_restore now invokes COPY FROM ... WITH (FREEZE) Needs also options --section=data --clean -j N so that each pg_restore worker process wraps each COPY in a transaction and precedes it with TRUNCATE. That way the data insertion is optimized and the visibility table is written without needing VACUUMing. Rebased-by: Stepan Neretin <slp...@gmail.com> --- src/bin/pg_dump/pg_backup.h | 1 + src/bin/pg_dump/pg_backup_archiver.c | 43 +++++++++++++++++++++++++++- src/bin/pg_dump/pg_dump.c | 9 +++++- src/bin/pg_dump/pg_restore.c | 4 +++ 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h index af0007fb6d2..befb524c9d3 100644 --- a/src/bin/pg_dump/pg_backup.h +++ b/src/bin/pg_dump/pg_backup.h @@ -116,6 +116,7 @@ typedef struct _restoreOptions int no_security_labels; /* Skip security label entries */ int no_subscriptions; /* Skip subscription entries */ int strict_names; + int freeze; /* COPY FREEZE */ const char *filename; int dumpSections; diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 30e0da31aa3..f190ca77836 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -99,6 +99,9 @@ static void restore_toc_entries_parallel(ArchiveHandle *AH, TocEntry *pending_list); static void restore_toc_entries_postfork(ArchiveHandle *AH, TocEntry *pending_list); +static void do_copy(ArchiveHandle *AH, + const char *cp, + bool freeze); static void pending_list_header_init(TocEntry *l); static void pending_list_append(TocEntry *l, TocEntry *te); static void pending_list_remove(TocEntry *te); @@ -1021,7 +1024,7 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel) */ if (te->copyStmt && strlen(te->copyStmt) > 0) { - ahprintf(AH, "%s", te->copyStmt); + do_copy(AH, te->copyStmt, ropt->freeze); AH->outputKind = OUTPUT_COPYDATA; } else @@ -1084,6 +1087,44 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel) return status; } +static void +do_copy(ArchiveHandle *AH, const char *cp, bool freeze) +{ + const size_t cp_len = strlen(cp); + bool cp_is_well_formed = false; + const char *cp_end; + + Assert(cp_len > 0); /* Have checked it in caller */ + + /* + * Check if the COPY statement is well written so that we can inject the + * FREEZE option. + */ + if (freeze) + { + int i = cp_len - 1; + + /* Cut off the trailing semicolon and whitespace. */ + while (i > 0 && + (cp[i] == ' ' || cp[i] == '\n' || cp[i] == ';')) + i--; + cp_end = &cp[i + 1]; + + if (cp_end - 10 > cp && + strncmp(cp_end - 10, "FROM stdin", 10) == 0 + ) + cp_is_well_formed = true; + else + pg_log_warning("COPY statement from dump file is not in the right form; Will not inject the FREEZE option"); + } + + if (freeze && cp_is_well_formed) + ahprintf(AH, "%.*s WITH (FREEZE);\n", + (int) (cp_end - cp), cp); + else + ahprintf(AH, "%s", cp); +} + /* * Allocate a new RestoreOptions block. * This is mainly so we can initialize it, but also for future expansion, diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 604fc109416..e69c3d6b99b 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2856,7 +2856,14 @@ dumpTableData(Archive *fout, const TableDataInfo *tdinfo) if (dopt->dump_inserts == 0) { - /* Dump/restore using COPY */ + /* + * Dump/restore using COPY. + * + * NOTE: do not use options in the COPY statement, as the restore + * process appends its own options. In fact pg_restore always expects + * + * COPY ... FROM stdin; + */ dumpFn = dumpTableData_copy; /* must use 2 steps here 'cause fmtId is nonreentrant */ printfPQExpBuffer(copyBuf, "COPY %s ", diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index 6ef789cb06d..f9a659de16e 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -114,6 +114,7 @@ main(int argc, char **argv) static int with_data = 0; static int with_schema = 0; static int with_statistics = 0; + static int freeze = 0; struct option cmdopts[] = { {"clean", 0, NULL, 'c'}, @@ -175,6 +176,7 @@ main(int argc, char **argv) {"statistics-only", no_argument, &statistics_only, 1}, {"filter", required_argument, NULL, 4}, {"exclude-database", required_argument, NULL, 6}, + {"freeze", no_argument, &freeze, 1}, {NULL, 0, NULL, 0} }; @@ -467,6 +469,7 @@ main(int argc, char **argv) opts->no_publications = no_publications; opts->no_security_labels = no_security_labels; opts->no_subscriptions = no_subscriptions; + opts->freeze = freeze; if (if_exists && !opts->dropSchema) pg_fatal("option --if-exists requires option -c/--clean"); @@ -715,6 +718,7 @@ usage(const char *progname) printf(_(" --with-data restore the data\n")); printf(_(" --with-schema restore the schema\n")); printf(_(" --with-statistics restore the statistics\n")); + printf(_(" --freeze COPY FREEZE (read the manual for caveats)\n")); printf(_("\nConnection options:\n")); printf(_(" -h, --host=HOSTNAME database server host or socket directory\n")); -- 2.48.1