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
