Date: Wed, 23 Jul 2025 23:23:47 +0200 (CEST)
From: Dimitrios Apostolou <jimis@gmx.net>
To: Stepan Neretin <slpmcf@gmail.com>
cc: =?UTF-8?Q?Fr=C3=A9d=C3=A9ric_Yhuel?= <frederic.yhuel@dalibo.com>,
    pgsql-performance@lists.postgresql.org, bruce@momjian.us,
    Christophe Courtois <christophe.courtois@dalibo.com>
Subject: Re: [PATCH] ALTER TABLE ADD FOREIGN KEY to partitioned table, is
 not parallelized
In-Reply-To: 
 <CA+Yyo5RA+KQPYZnsaKRRYpAKUpVKt2_xviKrqNrX_6pD7HkDpg@mail.gmail.com>
Message-ID: <q479op22-n5pp-n434-n25n-44or777p5r80@tzk.arg>
References: <5f002654-92e9-8032-3e1a-134461b4e819@gmx.net>
 <e89cfa42-edeb-431e-b33a-881556583666@dalibo.com>
 <ec31717f-511b-fbf2-f6b5-ade5b3c98899@gmx.net>
 <90e55c6b-3d79-4bf8-8874-a64dd10a0286@dalibo.com>
 <ff258f33-00d3-a944-9517-b3cccb662722@gmx.net>
 <4749eb1e-0fd6-f55b-034a-d3c0bbe5ba8b@gmx.net>
 <CA+Yyo5RA+KQPYZnsaKRRYpAKUpVKt2_xviKrqNrX_6pD7HkDpg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="0-1814841930-1753305849=:135269"
UI-OutboundReport: notjunk:1;M01:P0:t7frPxVNYJo=;lJ35UK7lTt7BiP5V8v+5jsUZ9tU
 H3Q9aDB+WeTVVcZHX3LNgfEe7hXBO+Jb94wywws9GZPLLAZ2hK1JPu8ABLSMjuTb3uawrPhvv
 nBIRVX9Zr206MgHY6vLZiFRLpgCCHlZlsRjOELHeghDWeWeOXlwudeYamaVjdo1yhbndmDyyY
 GuGTWKpm+TGjyyCTeR9Aewk433pId9lY+QBXvLLh4309sLqrMID3a8UQG5O1yK70NiJcdVcHb
 S/mUbjTrLGKNXBaYIrgFVLNlDq0O4Ne72uP2iaT9HcT8xK2g1Qnt0NT9tAOohMWRF/r/2Z3Gg
 2ooiYxuCLFnH2SHGijeLC/6xeA74ssw9T/aSx5V9FtwkZwrinBvk7FM5eL2aipIQogKJJO6Zl
 xD1EzVjXNvD25mpTC4YVlAzACJJ9nMONB4H24cYHBNvONU3wYuMFOtBbsxHp+JDMdX3/7xCxm
 GOl2wkH4x1+l94qmBFSYJrbzuqJcpdUxcrA/Sft9pdC1xU2LL4l/GYsUZCDbwvz74Zm637Yb2
 2rDbiC/cAqoGcJ7J8lZqVMhGpSB21pZ7h9XI3ltOQLLE9ELepkcrRWxvetGTbkKSgeoWbsoSp
 BDtHP4sFSfkiaSneggXyaQfVJvhYRaFa3N/KziherU5X2UDjQ1OsJRvkCHOilBCOW5xQq8BvW
 r6eOgn1xESnYnEgU7zWE8CDUAfmLFHO1uozgH9INIWnMNO/qovm1cObbEhEyhKNt1EUvRE+LP
 h6X+NDvlwqQTPwxdj7CGaQRCjt5Vi9h7U+k6l0MxXgAsjUS0NDaU2Dy+nq6qPr2g42aZeulDS
 vZTMwH1RBssntfgn1h4FqP22JIKoSQ6tU1gz6xHQ+UtDmkfhcQhEft9k8p3a4aVvHJJaxknKv
 6Z6mUOurnQIVVanuaFt2mQlctLLn1d6JOtQ0qSCeJUCzw1eKySksrqsSWv+GcEC/JQlRJeEuJ
 yPcEkbRSuCkBL0LakaV4piCX/52jtRbLkDJFIBwEuLgC9rMs5ekil0gJ+qPKV+1VPLT4EDolo
 1WlFNmoFwQ+fsvk7aFcR/cb0oCwelRDrQ1IM1BtBqJfcX2hcc/PmD5bVIO/0LWmNQBNj9a+rj
 APlaUfyr2BuBXz9/UaaLKvdrN1eCdPA+OKdMccWA8hGZQu0LqOX0jWsuizocWxO8MzszpzJlr
 OnYGOvoljcKXVCoRiMBgl3hOGOIpxMvuG3kfSKWzh8T5YU2OEkCXTQIC/HmQM9cxAoL+gPsNA
 //kHSofhdWct1TNDqL4mfEwrM0roQOprMaF3K9kVuWWjHQDxsosuB+oAX0sMgzX/LOMaAFEyg
 CFczvE4m9PoghFBXGbonnRs2Wn1rY1Mvn+E7XoQnhTMS/fQ6lli4lY+HtyrP4WQ37mD1Iv96f
 Dg3T5ms3NkZ/rVAxqHrjCIkX5tSTxxBNCSju8qWX8PE2M58gvju/6earLK3W0nVpTioI4eZ5M
 prRz4XbNco820Rof3/Ymr8bflg1wdhz1L7kWWP9teainCWfg9vEZK9Fl/3J3tdQeMTygPF8la
 kGQfK04e5asf5gp+kD+5VSH5qdyqVtcybF7d9sAI82Drt3XvVmKinwHRZ7mtLkNqwIg9oQd75
 vo/3hpSMAT6Oio0r1iMV6MTD283PQbyXXX0baweY3Zd0DX+WRzAu63wF6ZhvoNCqP9RRlQhms
 oJgv3/BNNp00tKE+QN/qCxxfnq6h/ZfLck7qc+AmfSjzz1tfLmwlt4qwj730hZc2YEPkJMGAU
 HDFQx3hLvd61tHvcAkj4pX8cZyPt/K6PqNrEsMpic2Fu+4n6JC5JkBdEjAfv84dWIu8iFzTYq
 Ha8uwv5nVSp5V4690eAP5pjQ9AoWrOoH7bkYdPvqI9bFyqjQSUBXD8PhZObbI4i1vuX1N8oLK
 BrdCB1MDGxDm19hPnBn2Tg3EorTmisKeUQDrYObxG4oLB29bRmNAhCm1NjJHaPBNrgCjfBYmZ
 bHYee+FUZcX/RW2+GvZwrQEgjOVklGdNFobQXIqf7Wc0xK4NpjKRexTA3Dsyc5v/ywtZLc+6A
 YVjueq20f147JqnxLpus+G1BkhGJr1xzHopbP/jIO3fo2DfFugjCoyNtyKu6OgOzZo0ETLwbf
 k7eBszrR+nTXGFHV1OgA++69uL2kmVfnHEmu1AtIBG8CqSY57UA6HUEB8gyBsdGHP9+FX3e6s
 wDQWpcAeR79nKB4EPX7KCsdI13t+8wAaQWwJJsH1SaWftgcmJXd/H/qx9EHd/C1ARU0bcqKjs
 5JxPKYFBTdZpyGlqHOx0/Hq53JB4ie/T0fodlQoCIQtn2gHW3cIE28dHU1MU7+WbCq2+c3VZa
 54ppjNLeihtYp69WUsfNpN2YfbdzVv+syk/M/4GPnEBp41bYY+bh0YhTD6Z+MoD6v3il/pwqO
 F8xXVRQem77CIMsjwfLZIeNQKlIznBDBYpWmJwtiPZRDQ3OJPJeMVcWjiJHN7FuictMtxdVDV
 +QY9/q+nAJPY+GpLtN9vynj3uvG/fihkfqthmP6YiO9AdvnJMS361bwYfYbuzpn7DRrZm9pEH
 hkQcUbUIZFbKKFPVPvfP3gA3Cs+BIt6lcMLNfi9R7gVObPbPyXsdjKXtONdw6SKZdChCQG3dG
 Gw4p92dCk6ApbLuJoJUl0Hv4qdjdzFVSzKKSwS52DsbbpfytofkvOHVyZl6g61tuPbXkgUBlN
 NP+yMf4oaXcJRWs5ro/NUN4UDEMkHlwwtL+MnMWjM2MbIAeCNnmGnBTqf20AFFFqsB1MRSgEo
 TUDwUoTfyT9342t3q1IOAaDk+wJFBqTRVZs1VwUvBTy40VjyiUdAwlX392wi0oo0Jt/aaZWaQ
 +TScveryx8ZkDwjw+iQcqURxDAvIOwD425AO8XGeCkttAbGUWMVtmKUDs3jxqijX6FERVIgCt
 jhoyd3AxuU9CXUET8Tn7V4z2abqOhc/rRJgtZrQ0CuV3N0Anzcg8HhtmQY/zu/trNmQUoeKbw
 tx9xuBF7UrefLC0C/zKV/fYgDLvd8GG5oJ9PQvkricqp/WnmsNFC/II39dsc4aKIWMZx8qS/Q
 c07cWXcqZsHJ4uNc9KOmqrdMsLaKh/NpK4rJ6x5tb2ii5BvFdpIIM4ORjMytEpMtOImWQh2Cw
 ILII5pe/XY3dAaOFMbmLyfrmy5nPoxucBCOICswBgh2TPxWLcwcuV41O8SRxcTNQLAWuA8xHQ
 Ps6gw+OGts/JNyvCsZlGh+Ecr6Zjk22AyHtw4lt7EP2W/FUeO7ETDWL4jXXh8X4b2bHzulK2W
 k5OOSCH015jfHI5rXGGlkcQAEXoWpMjlWG9z+vUUX6rYwH4iP90S00V9HjHVDkexXLPDvT3Sg
 O7L5kaAiwcuijoUYRB5oPGPeFqkHExj1ZTGwPrTwBX65/VRCPbxRFSN/9YIZNBvQ18duV2dqq
 UXgmUW3OR3WoPWg+gMO3K7fr4/bBMkUwWaKt7Qpka2himRWHdZeTdAN4i7r4dKNX7vSWZwSv9
 qryJfxa/pIFYHEobJ1wWv+fSmKUTPV78c9DQ0WEaZ9ic2vfOoR+QJHaOm4l2tIQmQFmuo2lYY
 QuG4cw4N6c9uAXVPCaqfrbqVCy5MestaUlAfQZuH0fs5/I4tCriY/G1sqtFM8F+i5A==
Archived-At: 
 <https://www.postgresql.org/message-id/q479op22-n5pp-n434-n25n-44or777p5r80%40tzk.arg>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--0-1814841930-1753305849=:135269
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable

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:

>=C2=A0 =C2=A0 =C2=A0 pg_restore -d "$DB_NAME" -j "$JOBS" --clean --if-exi=
sts --freeze "$DUMP_FILE" > /dev/null

On the other hand, I have tested combining --freeze with --data-only=20
=2D-clean, as implemented by another patch of mine at:

https://www.postgresql.org/message-id/flat/413c1cd8-1d6d-90ba-ac7b-b226a4d=
ad5ed%40gmx.net#44f06ce85a14a6238125a0ad8c9767db

In fact I was considering that patch as a requirement to this one, but I=
=20
see that you are using my patch independently to issue --clean --freeze=20
without --data-only. As far as I understand, this will also issue a=20
TRUNCATE before each COPY, and the benefits of FREEZE will be the same.=20
Thank you for revealing this to me.


>The main area of concern is the implementation method in `pg_backup_archi=
ver.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 &&
>=C2=A0 =C2=A0 strncmp(cp_end - 10, "FROM stdin", 10) =3D=3D 0
>=C2=A0 =C2=A0 )
>=C2=A0 =C2=A0 cp_is_well_formed =3D true;
>```
>
>This approach of string parsing is quite fragile. It makes a strong assum=
ption that the `COPY` statement generated by `pg_dump` will always end wit=
h `... FROM stdin;` (preceded by optional whitespace). While this is true =
now, it creates a tight and brittle coupling between `pg_dump`'s output fo=
rmat 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 s=
ilently or with a non-obvious warning.

It troubled me too. At least I have a pg_log_warning() message in this=20
case, and nothing detrimental will happen, it's just that the FREEZE=20
option will not be used and the message will be logged.

>
>A more robust solution would be to avoid string manipulation entirely. `p=
g_restore` should assemble the `COPY` command from its constituent parts (=
table name, column list, etc.) and then conditionally append the `WITH (FR=
EEZE)` clause before the final `FROM stdin;`. This would decouple the feat=
ure from
>the exact string representation in the dump archive.

Totally agree, but this sounds impossible to implement. Unfortunately=20
pg_dump generates SQL commands, and pg_restore edits them and executes=20
them. I wouldn't know where to start to change this whole architecture.

>
>An alternative=E2=80=94and arguably cleaner=E2=80=94approach might be to =
shift this logic to pg_dump.

Thinking it more thoroughly, pg_dump is not the place to compose the=20
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=20
think pg_dump should complicate its commands further (e.g. by adding=20
WITH FREEZE), as this is a choice to make during restore.
I think I've seen more places in pg_restore that edit the=20
commands before executing them.


>One important consideration that needs to be highlighted in the documenta=
tion for this feature is the impact on WAL generation. `COPY ... WITH (FRE=
EZE)` 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 ca=
n insert
>already frozen pages?

There should be no WAL traffic at all. If the transaction starts with a=20
TRUNCATE TABLE then COPY skips the wal completely. This gives a big=20
performance and stability gain when bulk-loading data, and this is what=20
my 2 patches try to leverage in more cases.

And as far as I can tell, if there is no TRUNCATE in the same=20
transaction, then pg_restore will output error like the following:

ERROR:  cannot perform COPY FREEZE because the table was not created or tr=
uncated in the current subtransaction

I hope such an erroris acceptable, since the sysadmin can just remove=20
=2D-freeze then and re-run the command.

>Additionally, it should be noted that the freeze option only works correc=
tly 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=20
yes, I should definitely update the docs and submit it again=20
individually.


Thank you for the rebase and review.

Dimitris

--0-1814841930-1753305849=:135269--