Обсуждение: teach pg_upgrade to handle in-place tablespaces

(Creating new thread from https://postgr.es/m/Z-MaPREQvH5YB0af%40nathan.)

On Tue, Mar 25, 2025 at 04:03:57PM -0500, Nathan Bossart wrote:
> I also wanted to draw attention to this note in 0003:
> 
>         /*
>          * XXX: The below line is a hack to deal with the fact that we
>           * presently don't have an easy way to find the corresponding new
>           * tablespace's path.  This will need to be fixed if/when we add
>           * pg_upgrade support for in-place tablespaces.
>           */
>          new_tablespace = old_tablespace;
> 
> I intend to address this in v19, primarily to enable same-version
> pg_upgrade testing with non-default tablespaces.  My current thinking is
> that we should have pg_upgrade also gather the new cluster tablespace
> information and map them to the corresponding tablespaces on the old
> cluster.  This might require some refactoring in pg_upgrade.  In any case,
> I didn't feel this should block the feature for v18.

Patch attached.

-- 
nathan

On Mon, Apr 28, 2025 at 04:07:16PM -0500, Nathan Bossart wrote:
> On Tue, Mar 25, 2025 at 04:03:57PM -0500, Nathan Bossart wrote:
>> I also wanted to draw attention to this note in 0003:
>> 
>>         /*
>>          * XXX: The below line is a hack to deal with the fact that we
>>           * presently don't have an easy way to find the corresponding new
>>           * tablespace's path.  This will need to be fixed if/when we add
>>           * pg_upgrade support for in-place tablespaces.
>>           */
>>          new_tablespace = old_tablespace;
>> 
>> I intend to address this in v19, primarily to enable same-version
>> pg_upgrade testing with non-default tablespaces.  My current thinking is
>> that we should have pg_upgrade also gather the new cluster tablespace
>> information and map them to the corresponding tablespaces on the old
>> cluster.  This might require some refactoring in pg_upgrade.  In any case,
>> I didn't feel this should block the feature for v18.
> 
> Patch attached.

And here is a new version of the patch that should hopefully build on
Windows.

-- 
nathan

rebased

-- 
nathan

On Mon, Jul 21, 2025 at 08:16:12PM -0400, Corey Huinker wrote:
> Everything here makes sense to me, but I do have one question:

Thanks for reviewing.

> In src/bin/pg_upgrade/info.c
> @@ -616,11 +630,21 @@ process_rel_infos(DbInfo *dbinfo, PGresult *res, void
> *arg)
> + if (inplace)
> + tablespace = psprintf("%s/%s",
> +  os_info.running_cluster->pgdata,
> +  PQgetvalue(res, relnum, i_spclocation));
> + else
> + tablespace = PQgetvalue(res, relnum, i_spclocation);
> 
> I'm sure it's no big deal, but we've already PQgetvalue() fetched that once
> for spcloc, and we're going to fetch it again no matter what the value of
> inplace is. Is there a reason to not reuse spcloc?

I can't think of any reason.  Fixed in v4.

-- 
nathan

On Mon, Jul 21, 2025 at 07:57:32PM -0500, Nathan Bossart wrote:
+    if (!defined($ENV{oldinstall}))
+    {
+        $new->append_conf('postgresql.conf',
+            "allow_in_place_tablespaces = true");
+        $old->append_conf('postgresql.conf',
+            "allow_in_place_tablespaces = true");
+    }

This would not choke as long as the old cluster is at least at v10,
but well why not.

-    # For cross-version tests, we can also check that pg_upgrade handles
-    # tablespaces.
+    my $tblspc = '';
     if (defined($ENV{oldinstall}))
     {
-        my $tblspc = PostgreSQL::Test::Utils::tempdir_short();
-        $old->safe_psql('postgres',
-            "CREATE TABLESPACE test_tblspc LOCATION '$tblspc'");
-        $old->safe_psql('postgres',
-            "CREATE DATABASE testdb2 TABLESPACE test_tblspc");
-        $old->safe_psql('postgres',
-            "CREATE TABLE test3 TABLESPACE test_tblspc AS SELECT generate_series(300, 401)"
-        );
-        $old->safe_psql('testdb2',
-            "CREATE TABLE test4 AS SELECT generate_series(400, 502)");
+        $tblspc = PostgreSQL::Test::Utils::tempdir_short();
     }
+    $old->safe_psql('postgres',
+        "CREATE TABLESPACE test_tblspc LOCATION '$tblspc'");
+    $old->safe_psql('postgres',
+        "CREATE DATABASE testdb2 TABLESPACE test_tblspc");
+    $old->safe_psql('postgres',
+        "CREATE TABLE test3 TABLESPACE test_tblspc AS SELECT generate_series(300, 401)"
+    );
+    $old->safe_psql('testdb2',
+        "CREATE TABLE test4 AS SELECT generate_series(400, 502)");
     $old->stop;

I would vote for having a total of two tablespaces when we can do so
safely: one non-inplace and one inplace, strengthening the
cross-checks for the prefixes assigned in the new paths when doing a
swap of the tablespace paths, even when an old installation is tested.

+       /*
+        * For now, we do not expect non-in-place tablespaces to move during
+        * upgrade.  If that changes, it will likely become necessary to run
+        * the above query on the new cluster, too.
+        */

Curiosity matter: has this ever been discussed?  Spoiler: I don't
recall so and why it would ever matter as tbspace OIDs should be fixed
across upgrades these days (right?).

@@ -53,19 +63,44 @@ get_tablespace_paths(void)
[...]
+       if (is_absolute_path(PQgetvalue(res, tblnum, i_spclocation)))
+       {

This use of is_absolute_path() should document that it is for in-place
tablespaces?

+   if (strcmp(new_tablespace, new_cluster.pgdata) == 0)
+       new_tblspc_suffix = "/base";

This knowledge is encapsulated in GetDatabasePath() currently.  Not
new, just saying that it could be nice to reduce the footprint of this
knowledge in pg_upgrade.  Perhaps not something to worry about here,
though.
--
Michael

On Tue, Jul 22, 2025 at 10:37:05AM +0900, Michael Paquier wrote:
> On Mon, Jul 21, 2025 at 07:57:32PM -0500, Nathan Bossart wrote:
> +    if (!defined($ENV{oldinstall}))
> +    {
> +        $new->append_conf('postgresql.conf',
> +            "allow_in_place_tablespaces = true");
> +        $old->append_conf('postgresql.conf',
> +            "allow_in_place_tablespaces = true");
> +    }
> 
> This would not choke as long as the old cluster is at least at v10,
> but well why not.

I'm not sure what you mean.  allow_in_place_tablespaces was added in v15,
right?

> I would vote for having a total of two tablespaces when we can do so
> safely: one non-inplace and one inplace, strengthening the
> cross-checks for the prefixes assigned in the new paths when doing a
> swap of the tablespace paths, even when an old installation is tested.

Seems reasonable, will add this.

> +       /*
> +        * For now, we do not expect non-in-place tablespaces to move during
> +        * upgrade.  If that changes, it will likely become necessary to run
> +        * the above query on the new cluster, too.
> +        */
> 
> Curiosity matter: has this ever been discussed?  Spoiler: I don't
> recall so and why it would ever matter as tbspace OIDs should be fixed
> across upgrades these days (right?).

I'm not aware of any such discussion.  I know there was a time before
version-specific tablespace subdirectories were a thing, but that is long
gone.  And yeah, we've preserved tablespace OIDs since v15.

> +       if (is_absolute_path(PQgetvalue(res, tblnum, i_spclocation)))
> +       {
> 
> This use of is_absolute_path() should document that it is for in-place
> tablespaces?

Yes.

> +   if (strcmp(new_tablespace, new_cluster.pgdata) == 0)
> +       new_tblspc_suffix = "/base";
> 
> This knowledge is encapsulated in GetDatabasePath() currently.  Not
> new, just saying that it could be nice to reduce the footprint of this
> knowledge in pg_upgrade.  Perhaps not something to worry about here,
> though.

I'll take a look and see what I can do.

-- 
nathan

On Mon, Jul 21, 2025 at 09:06:59PM -0500, Nathan Bossart wrote:
> On Tue, Jul 22, 2025 at 10:37:05AM +0900, Michael Paquier wrote:
>> This would not choke as long as the old cluster is at least at v10,
>> but well why not.
>
> I'm not sure what you mean.  allow_in_place_tablespaces was added in v15,
> right?

Yes initially, not so these days.  Here are the backpatches across
v10~14:
<16e7a8fd8e97> 2022-07-27 [Alvaro Herrera]  Allow "in place" tablespaces.
<961cab0a5a90> 2022-07-27 [Alvaro Herrera]  Allow "in place" tablespaces.
<7bdbbb87340f> 2022-07-27 [Alvaro Herrera]  Allow "in place" tablespaces.
<258c896418bf> 2022-07-27 [Alvaro Herrera]  Allow "in place" tablespaces.
<ca347f5433ed> 2022-07-27 [Alvaro Herrera]  Allow "in place" tablespaces.
--
Michael

On Tue, Jul 22, 2025 at 11:17:42AM +0900, Michael Paquier wrote:
> On Mon, Jul 21, 2025 at 09:06:59PM -0500, Nathan Bossart wrote:
>> On Tue, Jul 22, 2025 at 10:37:05AM +0900, Michael Paquier wrote:
>>> This would not choke as long as the old cluster is at least at v10,
>>> but well why not.
>> 
>> I'm not sure what you mean.  allow_in_place_tablespaces was added in v15,
>> right?
> 
> Yes initially, not so these days.  Here are the backpatches across
> v10~14:

Ah, I see.  Here's a new patch that should address all your feedback except
for the part about "/base".  I figure we can leave that for another patch.

-- 
nathan

Committed.

-- 
nathan