Thanks for reviewing, please find my response inline. On Wed, Jan 17, 2024 at 4:56 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Sat, Jan 13, 2024 at 12:36 AM Aleksander Alekseev > <aleksan...@timescale.com> wrote: > > > > Hi, > > > > Thanks for the patch. > > > > > Due to this behavior, it is not possible to add a test to show the > > > error message as it is done for CREATE SUBSCRIPTION. > > > Let me know if you think there is another way to add this test. > > > > I believe it can be done with TAP tests, see for instance: > > > > contrib/auto_explain/t/001_auto_explain.pl > > > > However I wouldn't insist on including the test in scope of this > > particular patch. Such a test doesn't currently exist, it can be added > > as a separate patch, and whether this is actually a useful test (all > > the tests consume time after all...) is somewhat debatable. Personally > > I agree that it would be nice to have though. > > > > This being said... > > > > > The ALTER SUBSCRIPTION command does not error out on the user > > > interface if updated with a bad connection string and the connection > > > failure error can only be seen in the respective log file. > > > > I wonder if we should fix this. Again, not necessarily in scope of > > this work, but if this is not a difficult task, again, it would be > > nice to have. > > > > Other than that v2 looks good. > > > > OK. I see now that any ALTER of the subscription's connection, even > to some value that fails, will restart a new worker (like ALTER of any > other subscription parameters). For a bad connection, it will continue > to relaunch-worker/ERROR over and over. > > test_sub=# \r2024-01-17 09:34:28.665 AEDT [11274] LOG: logical > replication apply worker for subscription "sub4" has started > 2024-01-17 09:34:28.666 AEDT [11274] ERROR: could not connect to the > publisher: invalid port number: "-1" > 2024-01-17 09:34:28.667 AEDT [928] LOG: background worker "logical > replication apply worker" (PID 11274) exited with exit code 1 > dRs su2024-01-17 09:34:33.669 AEDT [11391] LOG: logical replication > apply worker for subscription "sub4" has started > 2024-01-17 09:34:33.669 AEDT [11391] ERROR: could not connect to the > publisher: invalid port number: "-1" > 2024-01-17 09:34:33.670 AEDT [928] LOG: background worker "logical > replication apply worker" (PID 11391) exited with exit code 1 > b4 > ... > > I don't really have any opinion if that behaviour is good or bad, but > anyway, it is deliberate, and IMO it is outside the scope of your > patch, so v2 patch LGTM.
Upon code review, the ALTER SUBSCRIPTION updates the connection string after checking for parse and a few obvious errors and does not attempt to establish a connection. It is the apply worker running for the respective subscription that will try to connect and fail in case of a bad connection string. To me, it seems an intentional design behavior and I agree that deciding to change or maintain this behavior is out of this patch's scope. -- Thanks, Nisha
