On Tue, Feb 18, 2025 at 7:32 AM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > Dear Shubham, > > Thanks for updating the patch. Few comments. > > ``` > + If a database is present on the source but missing on the target, an > + error is raised. > ``` > > I'm not sure this description is accurate. I feel there is a case the command > can > succeed. > > Assuming that there are three databases (db1, db2, and db3) on the primary > but db3 > is missing on the standby when DBA is running the command. In > check_subscriber(), > pg_createsubscriber connects to the db1 and it says OK. Then we wait until all > changes are replicated, thus db3 is created on the target. Everything may go > well afterward. > > Possible handling is as follows: > > 1) Ignore the corner case and retain, > 2) Remove the description, > 3) Modify the description, or > 4) Modify codes to reject when the speficied database is missing on the > target. > > I feel 2) may be enough, but I want to ask others as well. >
I agree with you on this. Since there are cases where the command can still succeed even if a database is initially missing on the target, the description could be misleading. Based on this, I have removed that part of the description. > > ``` > + If a database exists on the target but not on the source, no > + subscription is created for it. > ``` > > I do not think this is needed. If the database exists on the target but not > on the > source, IIUC this means the DROP DATABASE was executed on the primary server > but > not replicated to the standby yet. In this case such databases would be > dropped > during the command because we ensure all changes are replicated before the > promotion. > Tell me if there are other situations... > I agree with your reasoning. Since any databases that exist on the target but not on the source would be dropped as part of ensuring all changes are replicated before promotion, explicitly mentioning this scenario isn’t necessary. Based on this, I have removed that part of the description. > ``` > + disconnect_database(conn, false); > + exit(1); > ``` > > This combination can be changed to disconnect_database(conn, true). > Fixed. The attached patch at [1] contains the suggested changes. [1] - https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com Thanks and regards, Shubham Khanna.
