Re: Handle infinite recursion in logical replication setup

On Wed, Aug 9, 2023 at 2:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 9, 2023 at 8:20 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Aug 8, 2023 at 6:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Aug 8, 2023 at 1:50 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > > >
> > > > This patch added the following error message:
> > > >
> > > > errdetail_plural("Subscribed publication %s is subscribing to other
> > > > publications.",
> > > > "Subscribed publications %s are subscribing to other publications.",
> > > > list_length(publist), pubnames->data),
> > > >
> > > > But in PostgreSQL, a publication cannot subscribe to a publication, so
> > > > this is not giving accurate information.  Apparently, what it is trying
> > > > to say is that
> > > >
> > > > The subscription that you are creating subscribes to publications that
> > > > contain tables that are written to by other subscriptions.
> > > >
> > > > Can we get to a more accurate wording like this?
> > > >
> > >
> > > +1 for changing the message as per your suggestion.
> > >
> >
> > PSA a patch to change this message text. The message now has wording
> > similar to the suggestion.
> >
> > > > There is also a translatability issue there, in the way the publication
> > > > list is pasted into the message.
> > > >
> >
> > The name/list substitution is now done within parentheses, which AFAIK
> > will be enough to eliminate any translation ambiguities.
> >
>
> A similar instance as below uses \"%s\" instead. So, isn't using the
> same a better idea?
> errdetail_plural("LDAP search for filter \"%s\" on server \"%s\"
> returned %d entry.",
>   "LDAP search for filter \"%s\" on server \"%s\" returned %d entries.",
>
>

Hmm. I don't see any similarity -- there the plural is for the %d, not
for a list of string items. And in our case the publication name (or
list of names) are already quoted by the function returning that list,
so quoting them again doesn't really make sense.

Example output using the patch look like this:

SINGLULAR
test_sub=# create subscription sub_test connection 'dbname=test_pub'
publication pub_all_at_pub with(origin=NONE);
WARNING:  subscription "sub_test" requested copy_data with origin =
NONE but might copy data that had a different origin
DETAIL:  The subscription that you are creating has a publication
("pub_all_at_pub") containing tables written to by other
subscriptions.
HINT:  Verify that initial data copied from the publisher tables did
not come from other origins.
NOTICE:  created replication slot "sub_test" on publisher
CREATE SUBSCRIPTION

PLURAL
test_sub=# create subscription sub_test3 connection 'dbname=test_pub'
publication pub_all_at_pub,pub_s1_at_pub,pub_s2_at_pub
with(origin=NONE);
WARNING:  subscription "sub_test3" requested copy_data with origin =
NONE but might copy data that had a different origin
DETAIL:  The subscription that you are creating has publications
("pub_s1_at_pub", "pub_all_at_pub") containing tables written to by
other subscriptions.
HINT:  Verify that initial data copied from the publisher tables did
not come from other origins.
NOTICE:  created replication slot "sub_test3" on publisher
CREATE SUBSCRIPTION

~

I thought above looked fine but if PeterE also does not like the ()
then the message can be rearranged slightly like below to put the
offending publications at the end of the message, like this:

DETAIL:  The subscription that you are creating has the following
publications containing tables written to by other subscriptions:
"pub_s1_at_pub", "pub_all_at_pub"

------
Kind Regards,
Peter Smith.
Fujitsu Australia