public inbox for [email protected]
help / color / mirror / Atom feedALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
9+ messages / 3 participants
[nested] [flat]
* ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-03 15:34 Dimitrios Apostolou <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Dimitrios Apostolou @ 2025-06-03 15:34 UTC (permalink / raw)
To: [email protected]
Hello list,
I'm debugging the abysmal performance of pg_restoring a huge
(10TB) database, which includes a table with more than 1000 partitions.
As part of pg_restore -j... --section=post-data
I see *days* being spent in the sequential creation of foreign keys:
ALTER TABLE the_master_partitioned_table
ADD CONSTRAINT ...
FOREIGN KEY (columnX) REFERENCES another_table(columnX)
Each of the above queries take 1-2 days, and multiple such queries are
issued sequentially.
My questions:
+ This master_partitioned_table contains at least 5 foreign key
constraints from different columns to different tables.
Despite pg_restore being invoked with -j, they all run sequentially.
I guess pg_restore dependency resolution decides so. Is there a
reason for that?
+ The backend process for each of the above ALTER TABLE commands, does not
parallelize the foreign key checks for the different partitions. I
know, because in the logs I see gigabytes of temporary files being
written, with the CONTEXT showing queries issued incrementally on
all the different partitions:
:LOG: temporary file: path "pg_tblspc/16390/PG_17_202406281/pgsql_tmp/pgsql_tmp3363462.579", size 1073741824
:CONTEXT: SQL statement "SELECT fk."columnX" FROM ONLY "public"."table_partition_214" fk
LEFT OUTER JOIN ONLY "public"."another_table" pk
ON ( pk."columnX" OPERATOR(pg_catalog.=) fk."columnX")
WHERE pk."columnX" IS NULL AND (fk."columnX" IS NOT NULL)"
Why can't the backend issue these queries in parallel workers?
+ Based on the pg_restore manual, I am experimenting with manually issuing
DISABLE TRIGGERS before the restoration, but I can't see a difference
when I'm doing the section=post-data separately. Is it supposed to speed
things up?
Thanks in advance,
Dimitris
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-04 07:00 Frédéric Yhuel <[email protected]>
parent: Dimitrios Apostolou <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Frédéric Yhuel @ 2025-06-04 07:00 UTC (permalink / raw)
To: Dimitrios Apostolou <[email protected]>; [email protected]
On 6/3/25 17:34, Dimitrios Apostolou wrote:
> The backend process for each of the above ALTER TABLE commands, does not
> parallelize the foreign key checks for the different partitions. I
> know, because in the logs I see gigabytes of temporary files being
> written, with the CONTEXT showing queries issued incrementally on
> all the different partitions:
>
> :LOG: temporary file: path "pg_tblspc/16390/PG_17_202406281/
> pgsql_tmp/pgsql_tmp3363462.579", size 1073741824
> :CONTEXT: SQL statement "SELECT fk."columnX" FROM ONLY
> "public"."table_partition_214" fk
> LEFT OUTER JOIN ONLY "public"."another_table" pk
> ON ( pk."columnX" OPERATOR(pg_catalog.=) fk."columnX")
> WHERE pk."columnX" IS NULL AND (fk."columnX" IS NOT NULL)"
>
> Why can't the backend issue these queries in parallel workers?
This has been discussed here:
https://www.postgresql.org/message-id/flat/0d21e3b4-dcde-290c-875e-6ed5013e8e52%40dalibo.com
Perhaps we should exhume this patch, but I believe the optimal strategy
is to perform a VACUUM between the data and post-data to build the
visibility map. The anti-join can then use an efficient index-only scan.
Best regards,
Frédéric
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-04 14:12 Dimitrios Apostolou <[email protected]>
parent: Frédéric Yhuel <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Dimitrios Apostolou @ 2025-06-04 14:12 UTC (permalink / raw)
To: Frédéric Yhuel <[email protected]>; +Cc: [email protected]
On Wed, 4 Jun 2025, Frédéric Yhuel wrote:
> On 6/3/25 17:34, Dimitrios Apostolou wrote:
>> The backend process for each of the above ALTER TABLE commands, does not
>> parallelize the foreign key checks for the different partitions. I
>> know, because in the logs I see gigabytes of temporary files being
>> written, with the CONTEXT showing queries issued incrementally on
>> all the different partitions:
>>
>> :LOG: temporary file: path "pg_tblspc/16390/PG_17_202406281/
>> pgsql_tmp/pgsql_tmp3363462.579", size 1073741824
>> :CONTEXT: SQL statement "SELECT fk."columnX" FROM ONLY
>> "public"."table_partition_214" fk
>> LEFT OUTER JOIN ONLY "public"."another_table" pk
>> ON ( pk."columnX" OPERATOR(pg_catalog.=) fk."columnX")
>> WHERE pk."columnX" IS NULL AND (fk."columnX" IS NOT NULL)"
>>
>> Why can't the backend issue these queries in parallel workers?
>
> This has been discussed here:
> https://www.postgresql.org/message-id/flat/0d21e3b4-dcde-290c-875e-6ed5013e8e52%40dalibo.com
>
> Perhaps we should exhume this patch, but I believe the optimal strategy is to
> perform a VACUUM between the data and post-data to build the visibility map.
> The anti-join can then use an efficient index-only scan.
Thanks for pointing to this patch.
Since I run each of the pg_restore sections separately, I will try to
manually do a VACUUM after the "data" and before the "post-data" section.
In general I have noticed most operations are slower after a succesful
pg_restore until VACUUM is complete, which is unfortunate as the database
is huge and it takes days to run. Something I have on my list to try, is
whether a COPY FREEZE would alleviate all this trouble, since all tuples
are immediately visible then. Maybe a patch for a new pg_restore
option --freeze is a better solution. Are my assumptions right?
Thanks,
Dimitris
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-05 14:13 Frédéric Yhuel <[email protected]>
parent: Dimitrios Apostolou <[email protected]>
0 siblings, 2 replies; 9+ messages in thread
From: Frédéric Yhuel @ 2025-06-05 14:13 UTC (permalink / raw)
To: Dimitrios Apostolou <[email protected]>; +Cc: [email protected]; [email protected]; Christophe Courtois <[email protected]>
On 6/4/25 16:12, Dimitrios Apostolou wrote:
> In general I have noticed most operations are slower after a succesful
> pg_restore until VACUUM is complete, which is unfortunate as the
> database is huge and it takes days to run. Something I have on my list
> to try, is whether a COPY FREEZE would alleviate all this trouble, since
> all tuples are immediately visible then. Maybe a patch for a new
> pg_restore option --freeze is a better solution. Are my assumptions right?
It seems that the idea has already been discussed:
https://www.postgresql.org/message-id/flat/CA%2BU5nM%2BXvkUu9ran%2B5cY%3DTWQquLTpvzte4KVMK%3DaDfbr-x...
I've CCed Bruce Mojman, in the hope that he can tell us more about it.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-05 14:51 Frédéric Yhuel <[email protected]>
parent: Frédéric Yhuel <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Frédéric Yhuel @ 2025-06-05 14:51 UTC (permalink / raw)
To: Dimitrios Apostolou <[email protected]>; +Cc: [email protected]; [email protected]; Christophe Courtois <[email protected]>
On 6/5/25 16:13, Frédéric Yhuel wrote:
>
>
> On 6/4/25 16:12, Dimitrios Apostolou wrote:
>> In general I have noticed most operations are slower after a succesful
>> pg_restore until VACUUM is complete, which is unfortunate as the
>> database is huge and it takes days to run. Something I have on my list
>> to try, is whether a COPY FREEZE would alleviate all this trouble,
>> since all tuples are immediately visible then. Maybe a patch for a new
>> pg_restore option --freeze is a better solution. Are my assumptions
>> right?
>
> It seems that the idea has already been discussed: https://
> www.postgresql.org/message-id/flat/
> CA%2BU5nM%2BXvkUu9ran%2B5cY%3DTWQquLTpvzte4KVMK%3DaDfbr-
> xfNXA%40mail.gmail.com#b61a7fee06e10e61afa68712bc0b3c5b
>
> I've CCed Bruce Mojman, in the hope that he can tell us more about it.
>
>
(It might be more interesting now than 12 years ago thanks to this
patch:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=7db0cd2145f2bce84cac92402e205e4d2b045...)
^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH] ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-10 00:51 Dimitrios Apostolou <[email protected]>
parent: Frédéric Yhuel <[email protected]>
1 sibling, 1 reply; 9+ messages in thread
From: Dimitrios Apostolou @ 2025-06-10 00:51 UTC (permalink / raw)
To: Frédéric Yhuel <[email protected]>; +Cc: [email protected]; [email protected]; Christophe Courtois <[email protected]>
On Thu, 5 Jun 2025, Frédéric Yhuel wrote:
>
> On 6/4/25 16:12, Dimitrios Apostolou wrote:
>> In general I have noticed most operations are slower after a succesful
>> pg_restore until VACUUM is complete, which is unfortunate as the database
>> is huge and it takes days to run. Something I have on my list to try, is
>> whether a COPY FREEZE would alleviate all this trouble, since all tuples
>> are immediately visible then. Maybe a patch for a new pg_restore option
>> --freeze is a better solution. Are my assumptions right?
>
> It seems that the idea has already been discussed:
> https://www.postgresql.org/message-id/flat/CA%2BU5nM%2BXvkUu9ran%2B5cY%3DTWQquLTpvzte4KVMK%3DaDfbr-x...
>
> I've CCed Bruce Mojman, in the hope that he can tell us more about it.
Thanks for all the pointers, it shows that changes in postgres are harder
than they appear.
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.
But unfortunately the item in question (ADD FOREIGN KEY) is not parallel
(probably because the discussion [2] you posted in your previous email
never concluded). I /think/ though it's reading all the data faster than
before, but still has to go through terabytes of data and this takes a
long time, for each of the foreign keys it adds.
[2] https://www.postgresql.org/message-id/flat/[email protected]
Still I wonder why pg_restore can't issue many ADD FOREIGN
KEY for the same table in parallel.
Regards,
Dimitris
Attachments:
[text/x-patch] v1-0001-pg_restore-freeze.patch (8.8K, ../../[email protected]/2-v1-0001-pg_restore-freeze.patch)
download | inline diff:
From aa5df1f9e5f45aa45a8d7ab3e1cd6e96fee82f17 Mon Sep 17 00:00:00 2001
From: Dimitrios Apostolou <[email protected]>
Date: Fri, 6 Jun 2025 19:03:51 +0200
Subject: [PATCH v1] 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.
---
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 | 5 ++++
4 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 9428b362c92..f43d2c196f7 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -112,16 +112,17 @@ typedef struct _restoreOptions
int column_inserts;
int if_exists;
int no_comments; /* Skip comments */
int no_policies; /* Skip row security policies */
int no_publications; /* Skip publication entries */
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;
int verbose;
int aclsSkip;
const char *lockWaitTimeout;
int include_everything;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index d48c6d11e7c..8a620fa74ae 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -92,16 +92,19 @@ static void RestoreOutput(ArchiveHandle *AH, CompressFileHandle *savedOutput);
static int restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel);
static void restore_toc_entries_prefork(ArchiveHandle *AH,
TocEntry *pending_list);
static void restore_toc_entries_parallel(ArchiveHandle *AH,
ParallelState *pstate,
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);
static int TocEntrySizeCompareQsort(const void *p1, const void *p2);
static int TocEntrySizeCompareBinaryheap(void *p1, void *p2, void *arg);
static void move_to_ready_heap(TocEntry *pending_list,
binaryheap *ready_heap,
RestorePass pass);
@@ -1023,17 +1026,17 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
fmtQualifiedId(te->namespace, te->tag));
}
/*
* If we have a copy statement, use it.
*/
if (te->copyStmt && strlen(te->copyStmt) > 0)
{
- ahprintf(AH, "%s", te->copyStmt);
+ do_copy(AH, te->copyStmt, ropt->freeze);
AH->outputKind = OUTPUT_COPYDATA;
}
else
AH->outputKind = OUTPUT_OTHERDATA;
AH->PrintTocDataPtr(AH, te);
/*
@@ -1086,16 +1089,54 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel)
}
if (AH->public.n_errors > 0 && status == WORKER_OK)
status = WORKER_IGNORED_ERRORS;
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,
*/
RestoreOptions *
NewRestoreOptions(void)
{
RestoreOptions *opts;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 37432e66efd..fd57457c080 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2811,17 +2811,24 @@ dumpTableData(Archive *fout, const TableDataInfo *tdinfo)
copyFrom);
tdDefn = pg_strdup(copyBuf->data);
}
else
copyFrom = fmtQualifiedDumpable(tbinfo);
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 ",
copyFrom);
appendPQExpBuffer(copyBuf, "%s FROM stdin;\n",
fmtCopyColumnList(tbinfo, clistBuf));
copyStmt = copyBuf->data;
}
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index d08d3979c41..c7cf5c1490e 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -109,16 +109,17 @@ main(int argc, char **argv)
static int no_security_labels = 0;
static int no_statistics = 0;
static int no_subscriptions = 0;
static int strict_names = 0;
static int statistics_only = 0;
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'},
{"create", 0, NULL, 'C'},
{"data-only", 0, NULL, 'a'},
{"globals-only", 0, NULL, 'g'},
{"dbname", 1, NULL, 'd'},
{"exit-on-error", 0, NULL, 'e'},
@@ -170,16 +171,17 @@ main(int argc, char **argv)
{"no-subscriptions", no_argument, &no_subscriptions, 1},
{"no-statistics", no_argument, &no_statistics, 1},
{"with-data", no_argument, &with_data, 1},
{"with-schema", no_argument, &with_schema, 1},
{"with-statistics", no_argument, &with_statistics, 1},
{"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}
};
pg_logging_init(argv[0]);
pg_logging_set_level(PG_LOG_WARNING);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_dump"));
@@ -462,16 +464,17 @@ main(int argc, char **argv)
opts->noTableAm = outputNoTableAm;
opts->noTablespace = outputNoTablespaces;
opts->use_setsessauth = use_setsessauth;
opts->no_comments = no_comments;
opts->no_policies = no_policies;
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");
opts->if_exists = if_exists;
opts->strict_names = strict_names;
if (opts->formatName)
{
@@ -710,16 +713,18 @@ usage(const char *progname)
" match at least one entity each\n"));
printf(_(" --transaction-size=N commit after every N objects\n"));
printf(_(" --use-set-session-authorization\n"
" use SET SESSION AUTHORIZATION commands instead of\n"
" ALTER OWNER commands to set ownership\n"));
printf(_(" --with-data dump the data\n"));
printf(_(" --with-schema dump the schema\n"));
printf(_(" --with-statistics dump 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"));
printf(_(" -p, --port=PORT database server port number\n"));
printf(_(" -U, --username=NAME connect as specified database user\n"));
printf(_(" -w, --no-password never prompt for password\n"));
printf(_(" -W, --password force password prompt (should happen automatically)\n"));
printf(_(" --role=ROLENAME do SET ROLE before restore\n"));
--
2.49.0
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-06-17 16:23 Dimitrios Apostolou <[email protected]>
parent: Dimitrios Apostolou <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Dimitrios Apostolou @ 2025-06-17 16:23 UTC (permalink / raw)
To: Frédéric Yhuel <[email protected]>; +Cc: [email protected]; [email protected]; Christophe Courtois <[email protected]>
>
> 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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-07-21 05:58 Stepan Neretin <[email protected]>
parent: Dimitrios Apostolou <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Stepan Neretin @ 2025-07-21 05:58 UTC (permalink / raw)
To: Dimitrios Apostolou <[email protected]>; +Cc: Frédéric Yhuel <[email protected]>; [email protected]; [email protected]; Christophe Courtois <[email protected]>
On Mon, Jul 21, 2025 at 12:24 PM Dimitrios Apostolou <[email protected]> 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
Attachments:
[text/x-patch] 0001-pg_restore-freeze.patch (5.4K, ../../CA+Yyo5RA+KQPYZnsaKRRYpAKUpVKt2_xviKrqNrX_6pD7HkDpg@mail.gmail.com/3-0001-pg_restore-freeze.patch)
download | inline diff:
From 912a0cb3b31d4884fd6b2ae8869b96483d620641 Mon Sep 17 00:00:00 2001
From: Stepan Neretin <[email protected]>
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 <[email protected]>
---
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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized
@ 2025-07-23 21:23 Dimitrios Apostolou <[email protected]>
parent: Stepan Neretin <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Dimitrios Apostolou @ 2025-07-23 21:23 UTC (permalink / raw)
To: Stepan Neretin <[email protected]>; +Cc: Frédéric Yhuel <[email protected]>; [email protected]; [email protected]; Christophe Courtois <[email protected]>
Hello Stepan!
I can see you tested my patch by itself, and with the following command:
On Monday 2025-07-21 07:58, Stepan Neretin wrote:
> pg_restore -d "$DB_NAME" -j "$JOBS" --clean --if-exists --freeze "$DUMP_FILE" > /dev/null
On the other hand, I have tested combining --freeze with --data-only
--clean, as implemented by another patch of mine at:
https://www.postgresql.org/message-id/flat/413c1cd8-1d6d-90ba-ac7b-b226a4dad5ed%40gmx.net#44f06ce85a...
In fact I was considering that patch as a requirement to this one, but I
see that you are using my patch independently to issue --clean --freeze
without --data-only. As far as I understand, this will also issue a
TRUNCATE before each COPY, and the benefits of FREEZE will be the same.
Thank you for revealing this to me.
>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.
It troubled me too. At least I have a pg_log_warning() message in this
case, and nothing detrimental will happen, it's just that the FREEZE
option will not be used and the message will be logged.
>
>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.
Totally agree, but this sounds impossible to implement. Unfortunately
pg_dump generates SQL commands, and pg_restore edits them and executes
them. I wouldn't know where to start to change this whole architecture.
>
>An alternative—and arguably cleaner—approach might be to shift this logic to pg_dump.
Thinking it more thoroughly, pg_dump is not the place to compose the
restoration SQL commands. The proper place is pg_restore.
I guess it's being done in pg_dump for historical reasons, but I don't
think pg_dump should complicate its commands further (e.g. by adding
WITH FREEZE), as this is a choice to make during restore.
I think I've seen more places in pg_restore that edit the
commands before executing them.
>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?
There should be no WAL traffic at all. If the transaction starts with a
TRUNCATE TABLE then COPY skips the wal completely. This gives a big
performance and stability gain when bulk-loading data, and this is what
my 2 patches try to leverage in more cases.
And as far as I can tell, if there is no TRUNCATE in the same
transaction, then pg_restore will output error like the following:
ERROR: cannot perform COPY FREEZE because the table was not created or truncated in the current subtransaction
I hope such an erroris acceptable, since the sysadmin can just remove
--freeze then and re-run the command.
>Additionally, it should be noted that the freeze option only works correctly when performing a fresh load of data into empty tables.
Do you think this patch has chances of going in by itself? If so then
yes, I should definitely update the docs and submit it again
individually.
Thank you for the rebase and review.
Dimitris
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2025-07-23 21:23 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-06-03 15:34 ALTER TABLE ADD FOREIGN KEY to partitioned table, is not parallelized Dimitrios Apostolou <[email protected]>
2025-06-04 07:00 ` Frédéric Yhuel <[email protected]>
2025-06-04 14:12 ` Dimitrios Apostolou <[email protected]>
2025-06-05 14:13 ` Frédéric Yhuel <[email protected]>
2025-06-05 14:51 ` Frédéric Yhuel <[email protected]>
2025-06-10 00:51 ` Dimitrios Apostolou <[email protected]>
2025-06-17 16:23 ` Dimitrios Apostolou <[email protected]>
2025-07-21 05:58 ` Stepan Neretin <[email protected]>
2025-07-23 21:23 ` Dimitrios Apostolou <[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