Обсуждение: pg_upgrade fails with in-place tablespace

On Mon, Jul 31, 2023 at 5:36 PM Rui Zhao <xiyuan.zr@alibaba-inc.com> wrote:
>
> Hello postgres hackers,
> Recently I encountered an issue: pg_upgrade fails when dealing with in-place tablespace. As we know, pg_upgrade uses
pg_dumpallto dump objects and pg_restore to restore them. The problem seems to be that pg_dumpall is dumping in-place
tablespaceas relative path, which can't be restored. 
>
> Here is the error message of pg_upgrade:
>
psql:/home/postgres/postgresql/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20230729T210058.329/dump/pg_upgrade_dump_globals.sql:36:
ERROR: tablespace location must be an absolute path 
>
> To help reproduce the failure, I have attached a tap test. The test also fails with tablespace regression, and it
changethe default value of allow_in_place_tablespaces to true only for debug, so it may not be fit for production.
However,it is enough to reproduce this failure. 
> I have also identified a solution for this problem, which I have included in the patch. The solution has two
modifications:
>   1) Make the function pg_tablespace_location returns path "" with in-place tablespace, rather than relative path.
Becausethe path of the in-place tablespace is always 'pg_tblspc/<oid>'. 
>   2) Only check the tablespace with an absolute path in pg_upgrade.
>
>   There are also other solutions, such as supporting the creation of relative-path tablespace in function
CreateTableSpace.But do we really need relative-path tablespace? I think in-place tablespace is enough by now. My
solutionmay be more lightweight and harmless. 
>
> Thank you for your attention to this matter.
>
> Best regards,
> Rui Zhao

Seems like allow_in_place_tablespaces is a developer only guc, and it
is not for end user usage.
check this commit 7170f2159fb21b62c263acd458d781e2f3c3f8bb


--
Regards
Junwang Zhao

On Sat, Jul 29, 2023 at 11:10:22PM +0800, Rui Zhao wrote:
>  2) Only check the tablespace with an absolute path in pg_upgrade.
>  There are also other solutions, such as supporting the creation of
>  relative-path tablespace in function CreateTableSpace. But do we
>  really need relative-path tablespace? I think in-place tablespace
>  is enough by now. My solution may be more lightweight and
>  harmless.

+       /* The path of the in-place tablespace is always pg_tblspc/<oid>. */
        if (!S_ISLNK(st.st_mode))
-               PG_RETURN_TEXT_P(cstring_to_text(sourcepath));
+               PG_RETURN_TEXT_P(cstring_to_text(""));

I don't think that your solution is the correct move.  Having
pg_tablespace_location() return the physical location of the
tablespace is very useful because that's the location where the
physical files are, and client applications don't need to know the
logic behind the way a path is built.

-            "      spcname != 'pg_global'");
+            "      spcname != 'pg_global' AND "
+            "      pg_catalog.pg_tablespace_location(oid) ~ '^/'");
That may not work on Windows where the driver letter is appended at
the beginning of the path, no?  There is is_absolute_path() to do this
job in a more portable way.
--
Michael

On Wed, Aug 02, 2023 at 10:38:00PM +0800, Rui Zhao wrote:
> However, when using pg_dump to back up this in-place tablespace, it
> is dumped with a different path:
> CREATE TABLESPACE space_test LOCATION 'pg_tblspc/<oid>'
> This can be confusing because it does not match the initial path
> that we created. Additionally, pg_restore cannot restore this
> in-place tablespace because the CREATE TABLESPACE command only
> supports an empty path for creating in-place tablespaces.

Yes, the dump part is a different issue.  Using a relative path is not
fine when restoring the tablespace.

> You are correct. I have made the necessary modifications to ensure
> compatibility with in-place tablespaces. Please find the updated
> code attached.

+       /* Allow to create in-place tablespace. */
+       if (!is_absolute_path(spclocation))
+           appendPQExpBuffer(buf, "SET allow_in_place_tablespaces = true;\n");
+
        appendPQExpBuffer(buf, "CREATE TABLESPACE %s", fspcname);
        appendPQExpBuffer(buf, " OWNER %s", fmtId(spcowner));

As you say, This change in pg_dumpall.c takes care of an issue related
to the dumps of in-place tablespaces, not only pg_upgrade.  See for
example on HEAD:
$ psql -c "CREATE TABLESPACE popo LOCATION ''"
$ pg_dumpall  | grep TABLESPACE
CREATE TABLESPACE popo OWNER postgres LOCATION 'pg_tblspc/16385';

And restoring this dump would be incorrect.  I think that we'd better
be doing something like that for the dump part, as of:
@@ -1286,7 +1286,12 @@ dumpTablespaces(PGconn *conn)
        appendPQExpBuffer(buf, " OWNER %s", fmtId(spcowner));

        appendPQExpBufferStr(buf, " LOCATION ");
-       appendStringLiteralConn(buf, spclocation, conn);
+
+       /* In-place tablespaces use a relative path */
+       if (is_absolute_path(spclocation))
+           appendStringLiteralConn(buf, spclocation, conn);
+       else
+           appendPQExpBufferStr(buf, "'';\n");

I don't have a good feeling about enforcing allow_in_place_tablespaces
in the connection creating the tablespace, as it can be useful to let
the restore of the dump fail if this GUC is disabled in the new
cluster, so as one can check that no in-place tablespaces are left
around on the new cluster restoring the contents of the dump.
--
Michael

On Tue, Aug 08, 2023 at 12:35:38PM +0800, Rui Zhao wrote:
> I have only enabled allow_in_place_tablespaces to true during a binary upgrade.

The pg_dump part should be OK to allow the restore of in-place
tablespaces, so I'll get that fixed first, but I think that we should
add a regression test as well.

I'll try to look at the binary upgrade path and pg_upgrade after this
is done.
--
Michael

On Tue, Aug 08, 2023 at 04:32:45PM +0900, Michael Paquier wrote:
> The pg_dump part should be OK to allow the restore of in-place
> tablespaces, so I'll get that fixed first, but I think that we should
> add a regression test as well.

I have added a test for that in 002_pg_dump.pl, and applied the fix
for pg_dumpall.

> I'll try to look at the binary upgrade path and pg_upgrade after this
> is done.

+   /* No need to check in-place tablespace. */
    snprintf(query, sizeof(query),
             "SELECT pg_catalog.pg_tablespace_location(oid) AS spclocation "
             "FROM  pg_catalog.pg_tablespace "
             "WHERE spcname != 'pg_default' AND "
-            "      spcname != 'pg_global'");
+            "      spcname != 'pg_global' AND "
+            "      pg_catalog.pg_tablespace_location(oid) != 'pg_tblspc/' || oid");

This does not really explain the reason why in-place tablespaces need
to be skipped (in short they don't need a separate creation or check
like the others in create_script_for_old_cluster_deletion because they
are part of the data folder).  Anyway, the more I think about it, the
less excited I get about the need to support pg_upgrade with in-place
tablespaces, especially regarding the fact that the patch blindly
enforces allows_in_place_tablespaces, assuming that it is OK to do so.
So what about the case where one would want to be warned if these are
still laying around when doing upgrades?  And what's the actual use
case for supporting that?  There is something else that we could do
here: add a pre-run check to make pg_upgrade fail gracefully if we
find in-place tablespaces in the old cluster.

+# Test with in-place tablespace.
+$oldnode->append_conf('postgresql.conf', 'allow_in_place_tablespaces = on');
+$oldnode->reload;
+$oldnode->safe_psql('postgres', "CREATE TABLESPACE space_test LOCATION ''");

While on it, note that this breaks the case of cross-version upgrades
where the old cluster uses binaries where in-place tablespaces are not
supported.  So this requires an extra version check.
--
Michael

On Wed, Aug 09, 2023 at 11:47:58AM +0800, Rui Zhao wrote:
> I believe that utilizing appendPQExpBufferStr(buf, "''"); would be better and more meaningful than using
appendStringLiteralConn(buf,"", conn); in this scenario.
 
> I apologize for this wrong usage. Please help me fix it.
> I will try to respond to pg_upgrade after my deep dive.

That was my original suggestion in [1], and what you've added gives
exactly the same result.

[1]: https://www.postgresql.org/message-id/ZNGhF6iBpUKgNimI%40paquier.xyz
--
Michael

On Fri, Aug 18, 2023 at 10:47:04PM +0800, Rui Zhao wrote:
> I have implemented the changes you suggested in our previous
> discussion. I have added the necessary code to ensure that
> pg_upgrade fails gracefully with in-place tablespaces and reports a
> hint to let the check pass.

+    /*
+     * No need to check in-place tablespaces when allow_in_place_tablespaces is
+     * on because they are part of the data folder.
+     */
+    if (allow_in_place_tablespaces)
+        snprintf(query, sizeof(query),
+                 "%s AND pg_catalog.pg_tablespace_location(oid) != 'pg_tblspc/' || oid", query);

I am not sure to follow the meaning of this logic.  There could be
in-place tablespaces that got created with the GUC enabled during a
session, while the GUC has disabled the GUC.  Shouldn't this stuff be
more restrictive and not require a lookup at the GUC, only looking at
the paths returned by pg_tablespace_location()?
--
Michael

On Sat, Aug 19, 2023 at 08:11:28PM +0800, Rui Zhao wrote:
> Please refer to the TAP test I have included for a better understanding
> of my suggestion.

Sure, but it seems to me that my original question is not really
answered:  what's your use case for being able to support in-place
tablespaces in pg_upgrade?  The original use case being such
tablespaces is to ease the introduction of tests with primaries and
standbys, which is not something that really applies to pg_upgrade,
no?  Perhaps there is meaning in having more TAP tests with
tablespaces and a combination of primary/standbys, still having
in-place tablespaces does not really make much sense to me because, as
these are in the data folder, we don't use them to test the path
re-creation logic.

I think that we should just add a routine in check.c that scans
pg_tablespace, reporting all the non-absolute paths found with their
associated tablespace names.
--
Michael

On Tue, Sep 05, 2023 at 10:28:44AM +0800, Rui Zhao wrote:
> It would be highly appreciated if the official PG could also
> incorporate support for this feature.

This is a development option, and documented as such.  You may also
want to be aware of the fact that tablespaces in the data folder
itself are discouraged, because they don't really make sense, and that
we generate a WARNING if trying to do that since 33cb8ff6aa11.

I don't have really have more to add, but others interested in this
topic are of course free to share their opinions and/or comments.
--
Michael

On Mon, Sep 4, 2023 at 11:29 PM Rui Zhao <xiyuan.zr@alibaba-inc.com> wrote:
> Thank you for your response. It is evident that there is a need
> for this features in our system.
> Firstly, our customers express their desire to utilize tablespaces
> for table management, without necessarily being concerned about
> the directory location of these tablespaces.

That's not the purpose of that feature. I'm not sure that we should be
worrying about users who want to use features for a purpose other than
the one for which they were designed.

> Secondly, currently PG only supports absolute-path tablespaces, but
> in-place tablespaces are very likely to become popular in the future.

That's not really a reason. If you said why this was going to happen,
that might be a reason, but just asserting that it will happen isn't.

--
Robert Haas
EDB: http://www.enterprisedb.com