Re: pg_upgrade fails with in-place tablespace[
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. Secondly, currently PG only supports absolute-path tablespaces, but in-place tablespaces are very likely to become popular in the future. Therefore, it is essential to incorporate support for in-place tablespaces in the pg_upgrade feature. I intend to implement this functionality in our system to accommodate our customers' requirements. It would be highly appreciated if the official PG could also incorporate support for this feature. -- Best regards, Rui Zhao
Re: pg_upgrade fails with in-place tablespace
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. Secondly, currently PG only supports absolute-path tablespaces, but in-place tablespaces are very likely to become popular in the future. Therefore, it is essential to incorporate support for in-place tablespaces in the pg_upgrade feature. I intend to implement this functionality in our system to accommodate our customers' requirements. It would be highly appreciated if the official PG could also incorporate support for this feature. -- Best regards, Rui Zhao -- From:Michael Paquier Sent At:2023 Sep. 1 (Fri.) 12:58 To:Mark Cc:pgsql-hackers Subject:Re: pg_upgrade fails with in-place tablespace[ 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
Re: pg_upgrade fails with in-place tablespace
Could you please provide more thoughts about this issue? pg_upgrade is the final issue of in-place tablespace and we are on the cusp of success. -- Best regards, Rui Zhao
Re: pg_upgrade fails with in-place tablespace
On Sat, Aug 19, 2023 at 12:45 PM +0800, Michael Paquier wrote: > 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()? There are two cases when we check in-place tablespaces: 1. The GUC allow_in_place_tablespaces is turned on in the old node. In this case, we assume that the caller is aware of what he is doing and allow the check to pass. 2. The GUC allow_in_place_tablespaces is turned off in the old node. In this case, we will fail the check if we find any in-place tablespaces. However, we provide an option to pass the check by adding --old-options="-c allow_in_place_tablespaces=on" in pg_upgrade. This option will turn on allow_in_place_tablespaces like GUC does. So I lookup at the GUC and check if it is turned on. We add this check of in-place tablespaces, to deal with the situation where one would want to be warned if these are in-place tablespaces when doing upgrades. I think we can take it a step further by providing an option that allows the check to pass if the caller explicitly adds --old-options="-c allow_in_place_tablespaces=on". Please refer to the TAP test I have included for a better understanding of my suggestion. -- Best regards, Rui Zhao
Re: pg_upgrade fails with in-place tablespace
On Wed, Aug 09, 2023 at 09:20 AM +0800, Michael Paquier wrote: > 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. 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. Thank you for your guidance and support. Please review my latest patch. -- Best regards, Rui Zhao 0001-Fix-pg_upgrade-with-in-place-tablespaces.patch Description: Binary data
Re: pg_upgrade fails with in-place tablespace
I have found the patch and upon review, I believe the following code can be improved. + /* + * In-place tablespaces use a relative path, and need to be dumped + * with an empty string as location. + */ + if (is_absolute_path(spclocation)) + appendStringLiteralConn(buf, spclocation, conn); + else + appendStringLiteralConn(buf, "", conn); 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. -- Best regards, Rui Zhao
Re: pg_upgrade fails with in-place tablespace
Thank you for your reply. I have implemented the necessary changes in accordance with your suggestions. On Tue, Aug 08, 2023 at 09:57:00AM +0800, Michael Paquier wrote: > 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. I have only enabled allow_in_place_tablespaces to true during a binary upgrade. Please review my lastest patch. -- Best regards, Rui Zhao 0001-Fix-pg_upgrade-with-in-place-tablespaces.patch Description: Binary data
Re: pg_upgrade fails with in-place tablespace
> 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.
I understand your concern. I suggest defining the standard behavior of the
in-place tablespace to match that of the pg_default and pg_global tablespaces.
The location of the pg_default and pg_global tablespaces is not a concern
because they have default paths. Similarly, the location of the in-place
tablespace is not a concern because they are all pg_tblspc/.
Another reason is that, up until now, we have been creating in-place
tablespaces using the following command:
CREATE TABLESPACE space_test LOCATION ''
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/'
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.
> 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.
You are correct. I have made the necessary modifications to ensure
compatibility with in-place tablespaces. Please find the updated code attached.
--
Best regards,
Rui Zhao
--
From:Michael Paquier
Sent At:2023 Aug. 1 (Tue.) 08:57
To:Mark
Cc:pgsql-hackers
Subject:Re: pg_upgrade fails with in-place tablespace
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/. */
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
0001-Fix-pg_upgrade-with-in-place-tablespaces.patch
Description: Binary data
回复：pg_rewind fails with in-place tablespace
Sorry for the delay in responding to this matter as I have been waiting for another similar subject to approved by a moderator. Upon review, I am satisfied with the proposed solution and believe that checking absolute path is better than hard coding with "pg_tblspc/". I think we have successfully resolved this issue in the pg_rewind case. However, I would like to bring your attention to another issue: pg_upgrade fails with in-place tablespace. Another issue is still waiting for approved. I have tested all the tools in src/bin with in-place tablespace, and I believe this is the final issue. Thank you for your understanding and assistance. Best regard, Rui Zhao -- 发件人：Michael Paquier 发送时间：2023年7月31日(星期一) 06:49 收件人：赵锐(惜元) 抄 送：pgsql-hackers ; Thomas Munro 主 题：Re: pg_rewind fails with in-place tablespace On Fri, Jul 28, 2023 at 04:54:56PM +0900, Michael Paquier wrote: > I am finishing with the attached. Thoughts? Applied this one as bf22792 on HEAD, without a backpatch as in-place tablespaces are around for developers. If there are opinions in favor of a backpatch, feel free of course. -- Michael
pg_upgrade fails with in-place tablespace
Hello postgres hackers, Recently I encountered an issue: pg_upgrade fails when dealing with in-place tablespace. As we know, pg_upgrade uses pg_dumpall to dump objects and pg_restore to restore them. The problem seems to be that pg_dumpall is dumping in-place tablespace as 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 change the 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. Because the path of the in-place tablespace is always 'pg_tblspc/'. 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. Thank you for your attention to this matter. Best regards, Rui Zhao 0001-Fix-pg_upgrade-fails-with-in-place-tablespace.patch Description: Binary data
Re:Possible corruption by CreateRestartPoint at promotion
Kyotaro'spatch seems good to me and fixes the test case in my patch. Do you have interest in adding a test like one in my patch? + LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); +/* * Remember the prior checkpoint's redo ptr for * UpdateCheckPointDistanceEstimate()*/ PriorRedoPtr = ControlFile-checkPointCopy.redo;+Assert (PriorRedoPtr < RedoRecPtr);Maybe PriorRedoPtr does not need to be under LWLockAcquire? regards. -- Zhao Rui Alibaba Cloud: https://www.aliyun.com/ --Original-- From: "Kyotaro Horiguchi" https://www.postgresql.org/message-id/20220316.091913.806120467943749797.horikyota.ntt%40gmail.com [2] https://www.postgresql.org/message-id/7bfad665-db9c-0c2a-2604-9f54763c5f9e%40oss.nttdata.com [3] https://www.postgresql.org/message-id/20220222.174401.765586897814316743.horikyota.ntt%40gmail.com -- Kyotaro Horiguchi NTT Open Source Software Center 0001-Test-of-this-problem-v14.patch Description: Binary data
