Re: Improve the connection failure error messages

Thanks for reviewing, please find my response inline.

On Wed, Jan 17, 2024 at 4:56 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sat, Jan 13, 2024 at 12:36 AM Aleksander Alekseev
> <aleksander@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