Обсуждение: Identify missing publications from publisher while create/alter subscription.

Hi,

Creating/altering subscription is successful when we specify a
publication which does not exist in the publisher. I felt we should
throw an error in this case, that will help the user to check if there
is any typo in the create subscription command or to create the
publication before the subscription is created.
If the above analysis looks correct, then please find a patch that
checks if the specified publications are present in the publisher and
throws an error if any of the publications is missing in the
publisher.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> Creating/altering subscription is successful when we specify a
> publication which does not exist in the publisher. I felt we should
> throw an error in this case, that will help the user to check if there
> is any typo in the create subscription command or to create the
> publication before the subscription is created.
> If the above analysis looks correct, then please find a patch that
> checks if the specified publications are present in the publisher and
> throws an error if any of the publications is missing in the
> publisher.
> Thoughts?

I was having similar thoughts (while working on  the logical
replication bug on alter publication...drop table behaviour) on why
create subscription succeeds without checking the publication
existence. I checked in documentation, to find if there's a strong
reason for that, but I couldn't. Maybe it's because of the principle
"first let users create subscriptions, later the publications can be
created on the publisher system", similar to this behaviour
"publications can be created without any tables attached to it
initially, later they can be added".

Others may have better thoughts.

If we do check publication existence for CREATE SUBSCRIPTION, I think
we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.

I wonder, why isn't dropping a publication from a list of publications
that are with subscription is not allowed?

Some comments so far on the patch:

1) I see most of the code in the new function check_publications() and
existing fetch_table_list() is the same. Can we have a generic
function, with maybe a flag to separate out the logic specific for
checking publication and fetching table list from the publisher.
2) Can't we know whether the publications exist on the publisher with
the existing (or modifying it a bit if required) query in
fetch_table_list(), so that we can avoid making another connection to
the publisher system from the subscriber?
3) If multiple publications are specified in the CREATE SUBSCRIPTION
query, IIUC, with your patch, the query fails even if at least one of
the publications doesn't exist. Should we throw a warning in this case
and allow the subscription be created for other existing
publications?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, 22 Jan 2021 at 00:51, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
>>
>> Hi,
>>
>> Creating/altering subscription is successful when we specify a
>> publication which does not exist in the publisher. I felt we should
>> throw an error in this case, that will help the user to check if there
>> is any typo in the create subscription command or to create the
>> publication before the subscription is created.
>> If the above analysis looks correct, then please find a patch that
>> checks if the specified publications are present in the publisher and
>> throws an error if any of the publications is missing in the
>> publisher.
>> Thoughts?
>
> I was having similar thoughts (while working on  the logical
> replication bug on alter publication...drop table behaviour) on why
> create subscription succeeds without checking the publication
> existence. I checked in documentation, to find if there's a strong
> reason for that, but I couldn't. Maybe it's because of the principle
> "first let users create subscriptions, later the publications can be
> created on the publisher system", similar to this behaviour
> "publications can be created without any tables attached to it
> initially, later they can be added".
>
> Others may have better thoughts.
>
> If we do check publication existence for CREATE SUBSCRIPTION, I think
> we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
>

Agreed. Current patch do not check publication existence for
ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).

> I wonder, why isn't dropping a publication from a list of publications
> that are with subscription is not allowed?
>
> Some comments so far on the patch:
>
> 1) I see most of the code in the new function check_publications() and
> existing fetch_table_list() is the same. Can we have a generic
> function, with maybe a flag to separate out the logic specific for
> checking publication and fetching table list from the publisher.

+1

> 2) Can't we know whether the publications exist on the publisher with
> the existing (or modifying it a bit if required) query in
> fetch_table_list(), so that we can avoid making another connection to
> the publisher system from the subscriber?

IIUC, the patch does not make another connection, it just execute a new
query in already connection.  If we want to check publication existence
for ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)
we should make another connection.

> 3) If multiple publications are specified in the CREATE SUBSCRIPTION
> query, IIUC, with your patch, the query fails even if at least one of
> the publications doesn't exist. Should we throw a warning in this case
> and allow the subscription be created for other existing
> publications?
>

+1. If all the publications do not exist, we should throw an error.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, Jan 22, 2021 at 10:14 AM japin <japinli@hotmail.com> wrote:
> > 2) Can't we know whether the publications exist on the publisher with
> > the existing (or modifying it a bit if required) query in
> > fetch_table_list(), so that we can avoid making another connection to
> > the publisher system from the subscriber?
>
> IIUC, the patch does not make another connection, it just execute a new
> query in already connection.  If we want to check publication existence
> for ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)
> we should make another connection.

Actually, I meant that we can avoid submitting another SQL query to
the publisher if we could manage to submit a single query that first
checks if a given publication exists in pg_publication and if yes
returns the tables associated with it from pg_publication_tables. Can
we modify the existing query in fetch_table_list that gets only the
table list from pg_publcation_tables to see if the given publication
exists in the pg_publication?

Yes you are right, if we were to check the existence of publications
provided with ALTER SUBSCRIPTION statements, we need to do
walrcv_connect, walrcv_exec. We could just call a common function from
there.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jan 22, 2021 at 12:14 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 10:14 AM japin <japinli@hotmail.com> wrote:
> > > 2) Can't we know whether the publications exist on the publisher with
> > > the existing (or modifying it a bit if required) query in
> > > fetch_table_list(), so that we can avoid making another connection to
> > > the publisher system from the subscriber?
> >
> > IIUC, the patch does not make another connection, it just execute a new
> > query in already connection.  If we want to check publication existence
> > for ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)
> > we should make another connection.
>
> Actually, I meant that we can avoid submitting another SQL query to
> the publisher if we could manage to submit a single query that first
> checks if a given publication exists in pg_publication and if yes
> returns the tables associated with it from pg_publication_tables. Can
> we modify the existing query in fetch_table_list that gets only the
> table list from pg_publcation_tables to see if the given publication
> exists in the pg_publication?
>
When I was implementing this, I had given it a thought on this. To do
that we might need some function/procedure to do this. I felt this
approach is more simpler and chose this approach.
Thoughts?

> Yes you are right, if we were to check the existence of publications
> provided with ALTER SUBSCRIPTION statements, we need to do
> walrcv_connect, walrcv_exec. We could just call a common function from
> there.
>
Yes I agree this should be done in ALTER SUBSCRIPTION SET PUBLICATION
case also, currently we do if refresh is enabled, it should also be
done in ALTER SUBSCRIPTION mysub SET PUBLICATION mypub WITH (REFRESH =
FALSE) also. I will include this in my next version of the patch.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jan 22, 2021 at 10:14 AM japin <japinli@hotmail.com> wrote:
>
>
> On Fri, 22 Jan 2021 at 00:51, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> >>
> >> Hi,
> >>
> >> Creating/altering subscription is successful when we specify a
> >> publication which does not exist in the publisher. I felt we should
> >> throw an error in this case, that will help the user to check if there
> >> is any typo in the create subscription command or to create the
> >> publication before the subscription is created.
> >> If the above analysis looks correct, then please find a patch that
> >> checks if the specified publications are present in the publisher and
> >> throws an error if any of the publications is missing in the
> >> publisher.
> >> Thoughts?
> >
> > I was having similar thoughts (while working on  the logical
> > replication bug on alter publication...drop table behaviour) on why
> > create subscription succeeds without checking the publication
> > existence. I checked in documentation, to find if there's a strong
> > reason for that, but I couldn't. Maybe it's because of the principle
> > "first let users create subscriptions, later the publications can be
> > created on the publisher system", similar to this behaviour
> > "publications can be created without any tables attached to it
> > initially, later they can be added".
> >
> > Others may have better thoughts.
> >
> > If we do check publication existence for CREATE SUBSCRIPTION, I think
> > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
> >
>
> Agreed. Current patch do not check publication existence for
> ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).
>
> > I wonder, why isn't dropping a publication from a list of publications
> > that are with subscription is not allowed?
> >
> > Some comments so far on the patch:
> >
> > 1) I see most of the code in the new function check_publications() and
> > existing fetch_table_list() is the same. Can we have a generic
> > function, with maybe a flag to separate out the logic specific for
> > checking publication and fetching table list from the publisher.
>
> +1
>
> > 2) Can't we know whether the publications exist on the publisher with
> > the existing (or modifying it a bit if required) query in
> > fetch_table_list(), so that we can avoid making another connection to
> > the publisher system from the subscriber?
>
> IIUC, the patch does not make another connection, it just execute a new
> query in already connection.  If we want to check publication existence
> for ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)
> we should make another connection.
>
> > 3) If multiple publications are specified in the CREATE SUBSCRIPTION
> > query, IIUC, with your patch, the query fails even if at least one of
> > the publications doesn't exist. Should we throw a warning in this case
> > and allow the subscription be created for other existing
> > publications?
> >
>
> +1. If all the publications do not exist, we should throw an error.

I also felt if any of the publications are not there, we should throw an error.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > Creating/altering subscription is successful when we specify a
> > publication which does not exist in the publisher. I felt we should
> > throw an error in this case, that will help the user to check if there
> > is any typo in the create subscription command or to create the
> > publication before the subscription is created.
> > If the above analysis looks correct, then please find a patch that
> > checks if the specified publications are present in the publisher and
> > throws an error if any of the publications is missing in the
> > publisher.
> > Thoughts?
>
> I was having similar thoughts (while working on  the logical
> replication bug on alter publication...drop table behaviour) on why
> create subscription succeeds without checking the publication
> existence. I checked in documentation, to find if there's a strong
> reason for that, but I couldn't. Maybe it's because of the principle
> "first let users create subscriptions, later the publications can be
> created on the publisher system", similar to this behaviour
> "publications can be created without any tables attached to it
> initially, later they can be added".
>
> Others may have better thoughts.
>
> If we do check publication existence for CREATE SUBSCRIPTION, I think
> we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
>
> I wonder, why isn't dropping a publication from a list of publications
> that are with subscription is not allowed?
>
> Some comments so far on the patch:
>
> 1) I see most of the code in the new function check_publications() and
> existing fetch_table_list() is the same. Can we have a generic
> function, with maybe a flag to separate out the logic specific for
> checking publication and fetching table list from the publisher.

I have made the common code between the check_publications and
fetch_table_list into a common function
get_appended_publications_query. I felt the rest of the code is better
off kept as it is.
The Attached patch has the changes for the same and also the change to
check publication exists during alter subscription set publication.
Thoughts?


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > Creating/altering subscription is successful when we specify a
> > > publication which does not exist in the publisher. I felt we should
> > > throw an error in this case, that will help the user to check if there
> > > is any typo in the create subscription command or to create the
> > > publication before the subscription is created.
> > > If the above analysis looks correct, then please find a patch that
> > > checks if the specified publications are present in the publisher and
> > > throws an error if any of the publications is missing in the
> > > publisher.
> > > Thoughts?
> >
> > I was having similar thoughts (while working on  the logical
> > replication bug on alter publication...drop table behaviour) on why
> > create subscription succeeds without checking the publication
> > existence. I checked in documentation, to find if there's a strong
> > reason for that, but I couldn't. Maybe it's because of the principle
> > "first let users create subscriptions, later the publications can be
> > created on the publisher system", similar to this behaviour
> > "publications can be created without any tables attached to it
> > initially, later they can be added".
> >
> > Others may have better thoughts.
> >
> > If we do check publication existence for CREATE SUBSCRIPTION, I think
> > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
> >
> > I wonder, why isn't dropping a publication from a list of publications
> > that are with subscription is not allowed?
> >
> > Some comments so far on the patch:
> >
> > 1) I see most of the code in the new function check_publications() and
> > existing fetch_table_list() is the same. Can we have a generic
> > function, with maybe a flag to separate out the logic specific for
> > checking publication and fetching table list from the publisher.
>
> I have made the common code between the check_publications and
> fetch_table_list into a common function
> get_appended_publications_query. I felt the rest of the code is better
> off kept as it is.
> The Attached patch has the changes for the same and also the change to
> check publication exists during alter subscription set publication.
> Thoughts?
>

So basically, the create subscription will throw an error if the
publication does not exist.  So will you throw an error if we try to
drop the publication which is subscribed by some subscription?  I mean
basically, you are creating a dependency that if you are creating a
subscription then there must be a publication that is not completely
insane but then we will have to disallow dropping the publication as
well.  Am I missing something?


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Creating/altering subscription is successful when we specify a
> > > > publication which does not exist in the publisher. I felt we should
> > > > throw an error in this case, that will help the user to check if there
> > > > is any typo in the create subscription command or to create the
> > > > publication before the subscription is created.
> > > > If the above analysis looks correct, then please find a patch that
> > > > checks if the specified publications are present in the publisher and
> > > > throws an error if any of the publications is missing in the
> > > > publisher.
> > > > Thoughts?
> > >
> > > I was having similar thoughts (while working on  the logical
> > > replication bug on alter publication...drop table behaviour) on why
> > > create subscription succeeds without checking the publication
> > > existence. I checked in documentation, to find if there's a strong
> > > reason for that, but I couldn't. Maybe it's because of the principle
> > > "first let users create subscriptions, later the publications can be
> > > created on the publisher system", similar to this behaviour
> > > "publications can be created without any tables attached to it
> > > initially, later they can be added".
> > >
> > > Others may have better thoughts.
> > >
> > > If we do check publication existence for CREATE SUBSCRIPTION, I think
> > > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
> > >
> > > I wonder, why isn't dropping a publication from a list of publications
> > > that are with subscription is not allowed?
> > >
> > > Some comments so far on the patch:
> > >
> > > 1) I see most of the code in the new function check_publications() and
> > > existing fetch_table_list() is the same. Can we have a generic
> > > function, with maybe a flag to separate out the logic specific for
> > > checking publication and fetching table list from the publisher.
> >
> > I have made the common code between the check_publications and
> > fetch_table_list into a common function
> > get_appended_publications_query. I felt the rest of the code is better
> > off kept as it is.
> > The Attached patch has the changes for the same and also the change to
> > check publication exists during alter subscription set publication.
> > Thoughts?
> >
>
> So basically, the create subscription will throw an error if the
> publication does not exist.  So will you throw an error if we try to
> drop the publication which is subscribed by some subscription?  I mean
> basically, you are creating a dependency that if you are creating a
> subscription then there must be a publication that is not completely
> insane but then we will have to disallow dropping the publication as
> well.  Am I missing something?

Do you mean DROP PUBLICATION non_existent_publication;?

Or

Do you mean when we drop publications from a subscription? If yes, do
we have a way to drop a publication from the subscription? See below
one of my earlier questions on this.
"I wonder, why isn't dropping a publication from a list of
publications that are with subscription is not allowed?"
At least, I see no ALTER SUBSCRIPTION ... DROP PUBLICATION mypub1 or
something similar?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 2:48 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > >
> > > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Creating/altering subscription is successful when we specify a
> > > > > publication which does not exist in the publisher. I felt we should
> > > > > throw an error in this case, that will help the user to check if there
> > > > > is any typo in the create subscription command or to create the
> > > > > publication before the subscription is created.
> > > > > If the above analysis looks correct, then please find a patch that
> > > > > checks if the specified publications are present in the publisher and
> > > > > throws an error if any of the publications is missing in the
> > > > > publisher.
> > > > > Thoughts?
> > > >
> > > > I was having similar thoughts (while working on  the logical
> > > > replication bug on alter publication...drop table behaviour) on why
> > > > create subscription succeeds without checking the publication
> > > > existence. I checked in documentation, to find if there's a strong
> > > > reason for that, but I couldn't. Maybe it's because of the principle
> > > > "first let users create subscriptions, later the publications can be
> > > > created on the publisher system", similar to this behaviour
> > > > "publications can be created without any tables attached to it
> > > > initially, later they can be added".
> > > >
> > > > Others may have better thoughts.
> > > >
> > > > If we do check publication existence for CREATE SUBSCRIPTION, I think
> > > > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
> > > >
> > > > I wonder, why isn't dropping a publication from a list of publications
> > > > that are with subscription is not allowed?
> > > >
> > > > Some comments so far on the patch:
> > > >
> > > > 1) I see most of the code in the new function check_publications() and
> > > > existing fetch_table_list() is the same. Can we have a generic
> > > > function, with maybe a flag to separate out the logic specific for
> > > > checking publication and fetching table list from the publisher.
> > >
> > > I have made the common code between the check_publications and
> > > fetch_table_list into a common function
> > > get_appended_publications_query. I felt the rest of the code is better
> > > off kept as it is.
> > > The Attached patch has the changes for the same and also the change to
> > > check publication exists during alter subscription set publication.
> > > Thoughts?
> > >
> >
> > So basically, the create subscription will throw an error if the
> > publication does not exist.  So will you throw an error if we try to
> > drop the publication which is subscribed by some subscription?  I mean
> > basically, you are creating a dependency that if you are creating a
> > subscription then there must be a publication that is not completely
> > insane but then we will have to disallow dropping the publication as
> > well.  Am I missing something?
>
> Do you mean DROP PUBLICATION non_existent_publication;?
>
> Or
>
> Do you mean when we drop publications from a subscription?

I mean it doesn’t seem right to disallow to create the subscription if
the publisher doesn't exist, and my reasoning was even though the
publisher exists while creating the subscription you might drop it
later right?.  So basically, now also we can create the same scenario
that a subscription may exist for the publication which does not
exist.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > So basically, the create subscription will throw an error if the
> > > publication does not exist.  So will you throw an error if we try to
> > > drop the publication which is subscribed by some subscription?  I mean
> > > basically, you are creating a dependency that if you are creating a
> > > subscription then there must be a publication that is not completely
> > > insane but then we will have to disallow dropping the publication as
> > > well.  Am I missing something?
> >
> > Do you mean DROP PUBLICATION non_existent_publication;?
> >
> > Or
> >
> > Do you mean when we drop publications from a subscription?
>
> I mean it doesn’t seem right to disallow to create the subscription if
> the publisher doesn't exist, and my reasoning was even though the
> publisher exists while creating the subscription you might drop it
> later right?.  So basically, now also we can create the same scenario
> that a subscription may exist for the publication which does not
> exist.

Yes, the above scenario can be created even now. If a publication is
dropped in the publisher system, then it will not replicate/publish
the changes for that publication (publication_invalidation_cb,
rel_sync_cache_publication_cb, LoadPublications in
get_rel_sync_entry), so subscriber doesn't receive them. But the
subscription can still contain that dropped publication in it's list
of publications.

The patch proposed in this thread, just checks while creation/altering
of the subscription on the subscriber system whether or not the
publication exists on the publisher system. This is one way
dependency. But given the above scenario, there can exist another
dependency i.e. publisher dropping the publisher at any time. So to
make it a complete solution i.e. not allowing non-existent
publications from the list of publications in the subscription, we
need to detect when the publications are dropped in the publisher and
we should, may be on a next connection to the subscriber, also look at
the subscription for that dropped publication, if exists remove it.
But that's an overkill and impractical I feel. Thoughts?

I also feel the best way to remove the confusion is to document why we
allow creating subscriptions even when the specified publications
don't exist on the publisher system? Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 3:38 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > So basically, the create subscription will throw an error if the
> > > > publication does not exist.  So will you throw an error if we try to
> > > > drop the publication which is subscribed by some subscription?  I mean
> > > > basically, you are creating a dependency that if you are creating a
> > > > subscription then there must be a publication that is not completely
> > > > insane but then we will have to disallow dropping the publication as
> > > > well.  Am I missing something?
> > >
> > > Do you mean DROP PUBLICATION non_existent_publication;?
> > >
> > > Or
> > >
> > > Do you mean when we drop publications from a subscription?
> >
> > I mean it doesn’t seem right to disallow to create the subscription if
> > the publisher doesn't exist, and my reasoning was even though the
> > publisher exists while creating the subscription you might drop it
> > later right?.  So basically, now also we can create the same scenario
> > that a subscription may exist for the publication which does not
> > exist.
>
> Yes, the above scenario can be created even now. If a publication is
> dropped in the publisher system, then it will not replicate/publish
> the changes for that publication (publication_invalidation_cb,
> rel_sync_cache_publication_cb, LoadPublications in
> get_rel_sync_entry), so subscriber doesn't receive them. But the
> subscription can still contain that dropped publication in it's list
> of publications.
>
> The patch proposed in this thread, just checks while creation/altering
> of the subscription on the subscriber system whether or not the
> publication exists on the publisher system. This is one way
> dependency. But given the above scenario, there can exist another
> dependency i.e. publisher dropping the publisher at any time. So to
> make it a complete solution i.e. not allowing non-existent
> publications from the list of publications in the subscription, we
> need to detect when the publications are dropped in the publisher and
> we should, may be on a next connection to the subscriber, also look at
> the subscription for that dropped publication, if exists remove it.
> But that's an overkill and impractical I feel. Thoughts?
>
> I also feel the best way to remove the confusion is to document why we
> allow creating subscriptions even when the specified publications
> don't exist on the publisher system? Thoughts?

Yes, that was my point that there is no point in solving it in some
cases and it can exist in other cases.  So I am fine with documenting
the behavior.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, 25 Jan 2021 at 17:18, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21@gmail.com> wrote:
>> >
>> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
>> > <bharath.rupireddyforpostgres@gmail.com> wrote:
>> > >
>> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
>> > > >
>> > > > Hi,
>> > > >
>> > > > Creating/altering subscription is successful when we specify a
>> > > > publication which does not exist in the publisher. I felt we should
>> > > > throw an error in this case, that will help the user to check if there
>> > > > is any typo in the create subscription command or to create the
>> > > > publication before the subscription is created.
>> > > > If the above analysis looks correct, then please find a patch that
>> > > > checks if the specified publications are present in the publisher and
>> > > > throws an error if any of the publications is missing in the
>> > > > publisher.
>> > > > Thoughts?
>> > >
>> > > I was having similar thoughts (while working on  the logical
>> > > replication bug on alter publication...drop table behaviour) on why
>> > > create subscription succeeds without checking the publication
>> > > existence. I checked in documentation, to find if there's a strong
>> > > reason for that, but I couldn't. Maybe it's because of the principle
>> > > "first let users create subscriptions, later the publications can be
>> > > created on the publisher system", similar to this behaviour
>> > > "publications can be created without any tables attached to it
>> > > initially, later they can be added".
>> > >
>> > > Others may have better thoughts.
>> > >
>> > > If we do check publication existence for CREATE SUBSCRIPTION, I think
>> > > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
>> > >
>> > > I wonder, why isn't dropping a publication from a list of publications
>> > > that are with subscription is not allowed?
>> > >
>> > > Some comments so far on the patch:
>> > >
>> > > 1) I see most of the code in the new function check_publications() and
>> > > existing fetch_table_list() is the same. Can we have a generic
>> > > function, with maybe a flag to separate out the logic specific for
>> > > checking publication and fetching table list from the publisher.
>> >
>> > I have made the common code between the check_publications and
>> > fetch_table_list into a common function
>> > get_appended_publications_query. I felt the rest of the code is better
>> > off kept as it is.
>> > The Attached patch has the changes for the same and also the change to
>> > check publication exists during alter subscription set publication.
>> > Thoughts?
>> >
>>
>> So basically, the create subscription will throw an error if the
>> publication does not exist.  So will you throw an error if we try to
>> drop the publication which is subscribed by some subscription?  I mean
>> basically, you are creating a dependency that if you are creating a
>> subscription then there must be a publication that is not completely
>> insane but then we will have to disallow dropping the publication as
>> well.  Am I missing something?
>
> Do you mean DROP PUBLICATION non_existent_publication;?
>
> Or
>
> Do you mean when we drop publications from a subscription? If yes, do
> we have a way to drop a publication from the subscription? See below
> one of my earlier questions on this.
> "I wonder, why isn't dropping a publication from a list of
> publications that are with subscription is not allowed?"
> At least, I see no ALTER SUBSCRIPTION ... DROP PUBLICATION mypub1 or
> something similar?
>

Why we do not support ALTER SUBSCRIPTION...ADD/DROP PUBLICATION?  When we
have multiple publications in subscription, but I want to add/drop a single
publication, it is conveient.  The ALTER SUBSCRIPTION...SET PUBLICATION...
should supply the completely publications.

Sorry, this question is unrelated with this subject.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Jan 25, 2021 at 5:18 PM japin <japinli@hotmail.com> wrote:
> > Do you mean when we drop publications from a subscription? If yes, do
> > we have a way to drop a publication from the subscription? See below
> > one of my earlier questions on this.
> > "I wonder, why isn't dropping a publication from a list of
> > publications that are with subscription is not allowed?"
> > At least, I see no ALTER SUBSCRIPTION ... DROP PUBLICATION mypub1 or
> > something similar?
> >
>
> Why we do not support ALTER SUBSCRIPTION...ADD/DROP PUBLICATION?  When we
> have multiple publications in subscription, but I want to add/drop a single
> publication, it is conveient.  The ALTER SUBSCRIPTION...SET PUBLICATION...
> should supply the completely publications.

Looks like the way to drop/add publication from the list of
publications in subscription requires users to specify all the list of
publications currently exists +/- the new publication that needs to be
added/dropped:

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432
dbname=postgres' PUBLICATION mypub1, mypub2, mypu3, mypub4, mypub5;
postgres=# select subpublications from pg_subscription;
           subpublications
-------------------------------------
 {mypub1,mypub2,mypu3,mypub4,mypub5}
(1 row)

Say, I want to drop mypub4:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypu3, mypub5;
postgres=# select subpublications from pg_subscription;
       subpublications
------------------------------
 {mypub1,mypub2,mypu3,mypub5}

Say, I want toa dd mypub4 and mypub6:
ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypu3,
mypub5, mypub4, mypub6;
postgres=# select subpublications from pg_subscription;
              subpublications
--------------------------------------------
 {mypub1,mypub2,mypu3,mypub5,mypub4,mypub6}
(1 row)

It will be good to have something like:

ALTER SUBSCRIPTION mysub1 ADD PUBLICATION mypub1, mypub3; which will
the publications to subscription if not added previously.

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub1, mypub3; which will
drop the publications from subscription if they exist in the
subscription's list of publications.

But I'm really not sure why the above syntax was not added earlier. We
may be missing something here.

> Sorry, this question is unrelated with this subject.

Yes, IMO it can definitely be discussed in another thread. It will be
good to get a separate opinion for this.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jan 25, 2021 at 5:18 PM japin <japinli@hotmail.com> wrote:
>
>
> On Mon, 25 Jan 2021 at 17:18, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >>
> >> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21@gmail.com> wrote:
> >> >
> >> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> >> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >> > >
> >> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21@gmail.com> wrote:
> >> > > >
> >> > > > Hi,
> >> > > >
> >> > > > Creating/altering subscription is successful when we specify a
> >> > > > publication which does not exist in the publisher. I felt we should
> >> > > > throw an error in this case, that will help the user to check if there
> >> > > > is any typo in the create subscription command or to create the
> >> > > > publication before the subscription is created.
> >> > > > If the above analysis looks correct, then please find a patch that
> >> > > > checks if the specified publications are present in the publisher and
> >> > > > throws an error if any of the publications is missing in the
> >> > > > publisher.
> >> > > > Thoughts?
> >> > >
> >> > > I was having similar thoughts (while working on  the logical
> >> > > replication bug on alter publication...drop table behaviour) on why
> >> > > create subscription succeeds without checking the publication
> >> > > existence. I checked in documentation, to find if there's a strong
> >> > > reason for that, but I couldn't. Maybe it's because of the principle
> >> > > "first let users create subscriptions, later the publications can be
> >> > > created on the publisher system", similar to this behaviour
> >> > > "publications can be created without any tables attached to it
> >> > > initially, later they can be added".
> >> > >
> >> > > Others may have better thoughts.
> >> > >
> >> > > If we do check publication existence for CREATE SUBSCRIPTION, I think
> >> > > we should also do it for ALTER SUBSCRIPTION ... SET PUBLICATION.
> >> > >
> >> > > I wonder, why isn't dropping a publication from a list of publications
> >> > > that are with subscription is not allowed?
> >> > >
> >> > > Some comments so far on the patch:
> >> > >
> >> > > 1) I see most of the code in the new function check_publications() and
> >> > > existing fetch_table_list() is the same. Can we have a generic
> >> > > function, with maybe a flag to separate out the logic specific for
> >> > > checking publication and fetching table list from the publisher.
> >> >
> >> > I have made the common code between the check_publications and
> >> > fetch_table_list into a common function
> >> > get_appended_publications_query. I felt the rest of the code is better
> >> > off kept as it is.
> >> > The Attached patch has the changes for the same and also the change to
> >> > check publication exists during alter subscription set publication.
> >> > Thoughts?
> >> >
> >>
> >> So basically, the create subscription will throw an error if the
> >> publication does not exist.  So will you throw an error if we try to
> >> drop the publication which is subscribed by some subscription?  I mean
> >> basically, you are creating a dependency that if you are creating a
> >> subscription then there must be a publication that is not completely
> >> insane but then we will have to disallow dropping the publication as
> >> well.  Am I missing something?
> >
> > Do you mean DROP PUBLICATION non_existent_publication;?
> >
> > Or
> >
> > Do you mean when we drop publications from a subscription? If yes, do
> > we have a way to drop a publication from the subscription? See below
> > one of my earlier questions on this.
> > "I wonder, why isn't dropping a publication from a list of
> > publications that are with subscription is not allowed?"
> > At least, I see no ALTER SUBSCRIPTION ... DROP PUBLICATION mypub1 or
> > something similar?
> >
>
> Why we do not support ALTER SUBSCRIPTION...ADD/DROP PUBLICATION?  When we
> have multiple publications in subscription, but I want to add/drop a single
> publication, it is conveient.  The ALTER SUBSCRIPTION...SET PUBLICATION...
> should supply the completely publications.
>
> Sorry, this question is unrelated with this subject.

Please start a new thread for this, let's discuss this separately.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Mon, 25 Jan 2021 at 21:55, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Jan 25, 2021 at 5:18 PM japin <japinli@hotmail.com> wrote:
>> > Do you mean when we drop publications from a subscription? If yes, do
>> > we have a way to drop a publication from the subscription? See below
>> > one of my earlier questions on this.
>> > "I wonder, why isn't dropping a publication from a list of
>> > publications that are with subscription is not allowed?"
>> > At least, I see no ALTER SUBSCRIPTION ... DROP PUBLICATION mypub1 or
>> > something similar?
>> >
>>
>> Why we do not support ALTER SUBSCRIPTION...ADD/DROP PUBLICATION?  When we
>> have multiple publications in subscription, but I want to add/drop a single
>> publication, it is conveient.  The ALTER SUBSCRIPTION...SET PUBLICATION...
>> should supply the completely publications.
>
> Looks like the way to drop/add publication from the list of
> publications in subscription requires users to specify all the list of
> publications currently exists +/- the new publication that needs to be
> added/dropped:
>
> CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432
> dbname=postgres' PUBLICATION mypub1, mypub2, mypu3, mypub4, mypub5;
> postgres=# select subpublications from pg_subscription;
>            subpublications
> -------------------------------------
>  {mypub1,mypub2,mypu3,mypub4,mypub5}
> (1 row)
>
> Say, I want to drop mypub4:
>
> ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypu3, mypub5;
> postgres=# select subpublications from pg_subscription;
>        subpublications
> ------------------------------
>  {mypub1,mypub2,mypu3,mypub5}
>
> Say, I want toa dd mypub4 and mypub6:
> ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypu3,
> mypub5, mypub4, mypub6;
> postgres=# select subpublications from pg_subscription;
>               subpublications
> --------------------------------------------
>  {mypub1,mypub2,mypu3,mypub5,mypub4,mypub6}
> (1 row)
>
> It will be good to have something like:
>
> ALTER SUBSCRIPTION mysub1 ADD PUBLICATION mypub1, mypub3; which will
> the publications to subscription if not added previously.
>
> ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub1, mypub3; which will
> drop the publications from subscription if they exist in the
> subscription's list of publications.
>
> But I'm really not sure why the above syntax was not added earlier. We
> may be missing something here.
>
>> Sorry, this question is unrelated with this subject.
>
> Yes, IMO it can definitely be discussed in another thread. It will be
> good to get a separate opinion for this.
>

I started a new thread [1] for this, please have a look.

[1] -
https://www.postgresql.org/message-id/MEYP282MB166939D0D6C480B7FBE7EFFBB6BC0@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Jan 25, 2021 at 10:32 PM vignesh C <vignesh21@gmail.com> wrote:
> > I mean it doesn’t seem right to disallow to create the subscription if
> > the publisher doesn't exist, and my reasoning was even though the
> > publisher exists while creating the subscription you might drop it
> > later right?.  So basically, now also we can create the same scenario
> > that a subscription may exist for the publication which does not
> > exist.
> >
>
> I would like to defer on documentation for this.
> I feel we should have the behavior similar to publication tables as given below, then it will be consistent and
easierfor the users: 
>
> This is the behavior in case of table:
> Step 1:
> PUBLISHER SIDE:
> create table t1(c1 int);
> create table t2(c1 int);
> CREATE PUBLICATION mypub1 for table t1,t2;
> -- All above commands succeeds
> Step 2:
> SUBSCRIBER SIDE:
> -- Create subscription without creating tables will result in error:
> CREATE SUBSCRIPTION mysub1 CONNECTION 'dbname=source_rep host=localhost user=vignesh port=5432' PUBLICATION mypub1;
> ERROR:  relation "public.t2" does not exist
> create table t1(c1 int);
> create table t2(c1 int);
>
> CREATE SUBSCRIPTION mysub1 CONNECTION 'dbname=source_rep host=localhost user=vignesh port=5432' PUBLICATION mypub1;
>
> postgres=# select * from pg_subscription;
>   oid  | subdbid | subname | subowner | subenabled | subbinary | substream |                       subconninfo
              | subslotname | subsynccommit | subpublications 
>
-------+---------+---------+----------+------------+-----------+-----------+---------------------------------------------------------+-------------+---------------+-----------------
>  16392 |   13756 | mysub1  |       10 | t          | f         | f         | dbname=source_rep host=localhost
user=vigneshport=5432 | mysub1      | off           | {mypub1} 
> (1 row)
>
> postgres=# select *,srrelid::oid::regclass from pg_subscription_rel;
>  srsubid | srrelid | srsubstate | srsublsn  | srrelid
> ---------+---------+------------+-----------+---------
>    16392 |   16389 | r          | 0/1608BD0 | t2
>    16392 |   16384 | r          | 0/1608BD0 | t1
>
> (2 rows)
> Step 3:
> PUBLISHER:
> drop table t2;
> create table t3;
> CREATE PUBLICATION mypub2 for table t1,t3;
>
> Step 4:
> SUBSCRIBER:
> postgres=# select *,srrelid::oid::regclass from pg_subscription_rel;
>  srsubid | srrelid | srsubstate | srsublsn  | srrelid
> ---------+---------+------------+-----------+---------
>    16392 |   16389 | r          | 0/1608BD0 | t2
>    16392 |   16384 | r          | 0/1608BD0 | t1
>
> (2 rows)
>
> postgres=#  alter subscription mysub1 refresh publication ;
> ALTER SUBSCRIPTION
>
> -- Subscription relation will be updated.
> postgres=# select *,srrelid::oid::regclass from pg_subscription_rel;
>  srsubid | srrelid | srsubstate | srsublsn  | srrelid
> ---------+---------+------------+-----------+---------
>    16392 |   16384 | r          | 0/1608BD0 | t1
> (1 row)
>
>
> -- Alter subscription fails while setting publication having a table that does not exist
> postgres=#  alter subscription mysub1 set publication mysub2;
> ERROR:  relation "public.t3" does not exist
>
> To maintain consistency, we should have similar behavior in case of publication too.
> If a publication which does not exist is specified during create subscription, then we should throw an error similar
tostep 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then we
shouldthrow an error similar to step 4 behavior. If publication is dropped after subscription is created, this should
beremoved when an alter subscription subname refresh publication is performed similar to step 4. 
> Thoughts?

IIUC, your idea is to check if the publications (that are associated
with a subscription) are present in the publisher or not during ALTER
SUBSCRIPTION ... REFRESH PUBLICATION;. If that's the case, then I have

scenario:
1) subscription is created with pub1, pub2 and assume both the
publications are present in the publisher
2) pub1 and pub2 tables data is replicated properly
3) pub2 is dropped on the publisher
4) run alter subscription .. refresh publication on the subscriber, so
that the pub2 tables will be removed from the subscriber
5) for some reason, user creates pub2 again on the publisher and want
to replicated some tables
6) run alter subscription .. refresh publication on the subscriber, so
that the pub2 tables will be added to the subscriber table list

Now, if we remove the dropped publication pub2 in step 4 from the
subscription list(as per your above analysis and suggestion), then
after step 5, users will need to add the publication pub2 to the
subscription again. I feel this is a change in the current behaviour.
The existing behaviour on master doesn't mandate this as the dropped
publications are not removed from the subscription list at all.

To not mandate any new behaviour, I would suggest to have a new option
for ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH
(remove_dropped_publications = false). The new option
remove_dropped_publications will have a default value false, when set
to true it will check if the publications that are present in the
subscription list are actually existing on the publisher or not, if
not remove them from the list. And also in the documentation we need
to clearly mention the consequence of this new option setting to true,
that is, the dropped publications if created again will need to be
added to the subscription list again.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 4, 2021 at 1:04 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Mar 3, 2021 at 8:59 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Wed, Feb 3, 2021, at 2:13 AM, Bharath Rupireddy wrote:
> >
> > On Mon, Jan 25, 2021 at 10:32 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > > If a publication which does not exist is specified during create subscription, then we should throw an error
similarto step 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then
weshould throw an error similar to step 4 behavior. If publication is dropped after subscription is created, this
shouldbe removed when an alter subscription subname refresh publication is performed similar to step 4. 
> > > Thoughts?
> >
> > Postgres implemented a replication mechanism that is decoupled which means that
> > both parties can perform "uncoordinated" actions. As you shown, the CREATE
> > SUBSCRIPTION informing a non-existent publication is one of them. I think that
> > being permissive in some situations can prevent you from painting yourself into
> > a corner. Even if you try to be strict on the subscriber side, the other side
> > (publisher) can impose you additional complexity.
> >
> > You are arguing that in the initial phase, the CREATE SUBSCRIPTION has a strict
> > mechanism. It is a fair point. However, impose this strictness for the other
> > SUBSCRIPTION commands should be carefully forethought. If we go that route, I
> > suggest to have the current behavior as an option. The reasons are: (i) it is
> > backward-compatible, (ii) it allows us some known flexibility (non-existent
> > publication), and (iii) it would probably allow us to fix a scenario created by
> > the strict mode. This strict mode can be implemented via new parameter
> > validate_publication (ENOCAFFEINE to propose a better name) that checks if the
> > publication is available when you executed the CREATE SUBSCRIPTION. Similar
> > parameter can be used in ALTER SUBSCRIPTION ... SET PUBLICATION and ALTER
> > SUBSCRIPTION ... REFRESH PUBLICATION.
>
> IIUC the validate_publication is kind of the feature enable/disable
> flag. With the default value being false, when set to true, it checks
> whether the publications exists or not while CREATE/ALTER SUBSCRIPTION
> and throw error. If I'm right, then the validate_publication option
> looks good to me. So, whoever wants to impose strict restrictions to
> CREATE/ALTER subscription can set it to true. All others will not see
> any new behaviour or such.
>
> > To not mandate any new behaviour, I would suggest to have a new option
> > for ALTER SUBSCRIPTION ... REFRESH PUBLICATION WITH
> > (remove_dropped_publications = false). The new option
> > remove_dropped_publications will have a default value false, when set
> > to true it will check if the publications that are present in the
> > subscription list are actually existing on the publisher or not, if
> > not remove them from the list. And also in the documentation we need
> > to clearly mention the consequence of this new option setting to true,
> > that is, the dropped publications if created again will need to be
> > added to the subscription list again.
> >
> > REFRESH PUBLICATION is not the right command to remove publications. There is a
> > command for it: ALTER SUBSCRIPTION ... SET PUBLICATION.
> >
> > The other alternative is to document that non-existent publication names can be
> > in the subscription catalog and it is ignored while executing SUBSCRIPTION
> > commands. You could possibly propose a NOTICE/WARNING that informs the user
> > that the SUBSCRIPTION command contains non-existent publication.
>
> I think, we can also have validate_publication option allowed for
> ALTER SUBSCRIPTION SET PUBLICATION and REFRESH PUBLICATION commands
> with the same behaviour i.e. error out when specified publications
> don't exist in the publisher. Thoughts?

Sorry for the delayed reply, I was working on a few other projects so
I was not able to reply quickly.
Since we are getting the opinion that if we make the check
publications by default it might affect the existing users, I'm fine
with having an option validate_option to check if the publication
exists in the publisher, that way there is no change for the existing
user. I have made a patch in similar lines, attached patch has the
changes for the same.
Thoughts?

Regards,
Vignesh

On Wed, Apr 7, 2021 at 10:37 PM vignesh C <vignesh21@gmail.com> wrote:
> > I think, we can also have validate_publication option allowed for
> > ALTER SUBSCRIPTION SET PUBLICATION and REFRESH PUBLICATION commands
> > with the same behaviour i.e. error out when specified publications
> > don't exist in the publisher. Thoughts?
>
> Sorry for the delayed reply, I was working on a few other projects so
> I was not able to reply quickly.
> Since we are getting the opinion that if we make the check
> publications by default it might affect the existing users, I'm fine
> with having an option validate_option to check if the publication
> exists in the publisher, that way there is no change for the existing
> user. I have made a patch in similar lines, attached patch has the
> changes for the same.
> Thoughts?

Here are some comments on v3 patch:

1) Please mention what's the default value of the option
+       <varlistentry>
+        <term><literal>validate_publication</literal>
(<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether the subscriber must verify if the specified
+          publications are present in the publisher. By default, the subscriber
+          will not check if the publications are present in the publisher.
+         </para>
+        </listitem>
+       </varlistentry>

2) How about
+          Specifies whether the subscriber must verify the
publications that are
+       being subscribed to are present in the publisher. By default,
the subscriber
instead of
+          Specifies whether the subscriber must verify if the specified
+          publications are present in the publisher. By default, the subscriber

3) I think we can make below common code into a single function with
flags to differentiate processing for both, something like:
StringInfoData *get_publist_str(List *publicaitons, bool use_quotes,
bool is_fetch_table_list);
check_publications:
+        /* Convert the publications which does not exist into a string. */
+        initStringInfo(&nonExistentPublications);
+        foreach(lc, publicationsCopy)
+        {
and get_appended_publications_query:
     foreach(lc, publications)

With the new function that only prepares comma separated list of
publications, you can get rid of get_appended_publications_query and
just append the returned list to the query.
fetch_table_list: get_publist_str(publications, true, true);
check_publications: for select query preparation
get_publist_str(publications, true, false); and for error string
preparation get_publist_str(publications, false, false);

And also let the new function get_publist_str allocate the string and
just mention as a note in the function comment that the callers should
pfree the returned string.

4) We could do following,
        ereport(ERROR,
                (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
                 errmsg_plural("publication %s does not exist in the publisher",
                           "publications %s do not exist in the publisher",
                        list_length(publicationsCopy),
                        nonExistentPublications.data)));
instead of
+        ereport(ERROR,
+                (errmsg("publication(s) %s does not exist in the publisher",
+                        nonExistentPublications.data)));

        if (list_member(cte->ctecolnames,
makeString(cte->search_clause->search_seq_column)))

5) I think it's better with
+ * Check the specified publication(s) is(are) present in the publisher.
instead of
+ * Verify that the specified publication(s) exists in the publisher.

6) Instead of such a longer variable name "nonExistentPublications"
how about just "pubnames" and add a comment there saying "going to
error out with the list of non-existent publications" with that the
variable and that part of code's context is clear.

7) You can just do
publications = list_copy(publications);
instead of using another variable publicationsCopy
    publicationsCopy = list_copy(publications);

8) If you have done StringInfoData *cmd = makeStringInfo();, then no
need of initStringInfo(cmd);

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 8, 2021 at 12:13 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Apr 7, 2021 at 10:37 PM vignesh C <vignesh21@gmail.com> wrote:
> > > I think, we can also have validate_publication option allowed for
> > > ALTER SUBSCRIPTION SET PUBLICATION and REFRESH PUBLICATION commands
> > > with the same behaviour i.e. error out when specified publications
> > > don't exist in the publisher. Thoughts?
> >
> > Sorry for the delayed reply, I was working on a few other projects so
> > I was not able to reply quickly.
> > Since we are getting the opinion that if we make the check
> > publications by default it might affect the existing users, I'm fine
> > with having an option validate_option to check if the publication
> > exists in the publisher, that way there is no change for the existing
> > user. I have made a patch in similar lines, attached patch has the
> > changes for the same.
> > Thoughts?
>
> Here are some comments on v3 patch:
>

Thanks for the comments

> 1) Please mention what's the default value of the option
> +       <varlistentry>
> +        <term><literal>validate_publication</literal>
> (<type>boolean</type>)</term>
> +        <listitem>
> +         <para>
> +          Specifies whether the subscriber must verify if the specified
> +          publications are present in the publisher. By default, the subscriber
> +          will not check if the publications are present in the publisher.
> +         </para>
> +        </listitem>
> +       </varlistentry>
>

Modified.

> 2) How about
> +          Specifies whether the subscriber must verify the
> publications that are
> +       being subscribed to are present in the publisher. By default,
> the subscriber
> instead of
> +          Specifies whether the subscriber must verify if the specified
> +          publications are present in the publisher. By default, the subscriber
>

Slightly reworded and modified.

> 3) I think we can make below common code into a single function with
> flags to differentiate processing for both, something like:
> StringInfoData *get_publist_str(List *publicaitons, bool use_quotes,
> bool is_fetch_table_list);
> check_publications:
> +        /* Convert the publications which does not exist into a string. */
> +        initStringInfo(&nonExistentPublications);
> +        foreach(lc, publicationsCopy)
> +        {
> and get_appended_publications_query:
>      foreach(lc, publications)
>
> With the new function that only prepares comma separated list of
> publications, you can get rid of get_appended_publications_query and
> just append the returned list to the query.
> fetch_table_list: get_publist_str(publications, true, true);
> check_publications: for select query preparation
> get_publist_str(publications, true, false); and for error string
> preparation get_publist_str(publications, false, false);
>
> And also let the new function get_publist_str allocate the string and
> just mention as a note in the function comment that the callers should
> pfree the returned string.
>

I felt the existing code looks better, if we have a common function,
we will have to lot of if conditions as both the functions is not same
to same, they operate on different data types and do the preparation
appropriately. Like fetch_table_list get nspname & relname and
converts it to RangeVar and adds to the list other function prepares a
text and deletes the entries present from the list. So I did not fix
this. Thoughts?

> 4) We could do following,
>         ereport(ERROR,
>                 (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
>                  errmsg_plural("publication %s does not exist in the publisher",
>                            "publications %s do not exist in the publisher",
>                         list_length(publicationsCopy),
>                         nonExistentPublications.data)));
> instead of
> +        ereport(ERROR,
> +                (errmsg("publication(s) %s does not exist in the publisher",
> +                        nonExistentPublications.data)));
>
>         if (list_member(cte->ctecolnames,
> makeString(cte->search_clause->search_seq_column)))
>

Modified.

> 5) I think it's better with
> + * Check the specified publication(s) is(are) present in the publisher.
> instead of
> + * Verify that the specified publication(s) exists in the publisher.
>

Modified.

> 6) Instead of such a longer variable name "nonExistentPublications"
> how about just "pubnames" and add a comment there saying "going to
> error out with the list of non-existent publications" with that the
> variable and that part of code's context is clear.
>

Modified.

> 7) You can just do
> publications = list_copy(publications);
> instead of using another variable publicationsCopy
>     publicationsCopy = list_copy(publications);

publications is an input list to this function, I did not want this
function to change this list. I felt existing is fine. Thoughts?

> 8) If you have done StringInfoData *cmd = makeStringInfo();, then no
> need of initStringInfo(cmd);

Modified.

Attached v4 patch has the fixes for the comments.

Regards,
Vignesh

On Tue, Apr 13, 2021 at 6:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > 2) How about
> > +          Specifies whether the subscriber must verify the
> > publications that are
> > +       being subscribed to are present in the publisher. By default,
> > the subscriber
> > instead of
> > +          Specifies whether the subscriber must verify if the specified
> > +          publications are present in the publisher. By default, the subscriber
> >
>
> Slightly reworded and modified.

+         <para>
+          When true, the command will try to verify if the specified
+          publications that are subscribed is present in the publisher.
+          The default is <literal>false</literal>.
+         </para>

"publications that are subscribed" is not right as the subscriber is
not yet subscribed, it is "trying to subscribing", and it's not that
the command "will try to verify", it actually verifies. So you can
modify as follows:

+         <para>
+          When true, the command verifies if all the specified
publications that are being subscribed to are present in the publisher
and throws an error if any of the publication doesn't exist. The
default is <literal>false</literal>.
+         </para>

> > 3) I think we can make below common code into a single function with
> > flags to differentiate processing for both, something like:
> > StringInfoData *get_publist_str(List *publicaitons, bool use_quotes,
> > bool is_fetch_table_list);
> > check_publications:
> > +        /* Convert the publications which does not exist into a string. */
> > +        initStringInfo(&nonExistentPublications);
> > +        foreach(lc, publicationsCopy)
> > +        {
> > and get_appended_publications_query:
> >      foreach(lc, publications)
> >
> > With the new function that only prepares comma separated list of
> > publications, you can get rid of get_appended_publications_query and
> > just append the returned list to the query.
> > fetch_table_list: get_publist_str(publications, true, true);
> > check_publications: for select query preparation
> > get_publist_str(publications, true, false); and for error string
> > preparation get_publist_str(publications, false, false);
> >
> > And also let the new function get_publist_str allocate the string and
> > just mention as a note in the function comment that the callers should
> > pfree the returned string.
> >
>
> I felt the existing code looks better, if we have a common function,
> we will have to lot of if conditions as both the functions is not same
> to same, they operate on different data types and do the preparation
> appropriately. Like fetch_table_list get nspname & relname and
> converts it to RangeVar and adds to the list other function prepares a
> text and deletes the entries present from the list. So I did not fix
> this. Thoughts?

I was actually thinking we could move the following duplicate code
into a function:
        foreach(lc, publicationsCopy)
        {
            char       *pubname = strVal(lfirst(lc));

            if (first)
                first = false;
            else
                appendStringInfoString(&pubnames, ", ");
            appendStringInfoString(&pubnames, "\"");
            appendStringInfoString(&pubnames, pubname);
            appendStringInfoString(&pubnames, "\"");
        }
and
    foreach(lc, publications)
    {
        char       *pubname = strVal(lfirst(lc));

        if (first)
            first = false;
        else
            appendStringInfoString(cmd, ", ");

        appendStringInfoString(cmd, quote_literal_cstr(pubname));
    }
that function can be:
static void
get_publications_str(List *publications, StringInfo dest, bool quote_literal)
{
    ListCell   *lc;
    bool        first = true;

    Assert(list_length(publications) > 0);

    foreach(lc, publications)
    {
        char       *pubname = strVal(lfirst(lc));

        if (first)
            first = false;
        else
            appendStringInfoString(dest, ", ");

        if (quote_literal)
            appendStringInfoString(pubnames, quote_literal_cstr(pubname));
        else
        {
            appendStringInfoString(&dest, "\"");
            appendStringInfoString(&dest, pubname);
            appendStringInfoString(&dest, "\"");
        }
    }
}

This way, we can get rid of get_appended_publications_query and use
the above function to return the appended list of publications. We
need to just pass quote_literal as true while preparing the publist
string for publication query and append it to the query outside the
function. While preparing publist str for error, pass quote_literal as
false. Thoughts?

> > 7) You can just do
> > publications = list_copy(publications);
> > instead of using another variable publicationsCopy
> >     publicationsCopy = list_copy(publications);
>
> publications is an input list to this function, I did not want this
> function to change this list. I felt existing is fine. Thoughts?

Okay.

Typo - it's not "subcription" +# Create subcription for a publication
which does not exist.

I think we can remove extra { } by moving the comment above if clause
much like you did in AlterSubscription_refresh. And it's not "exists",
it is "exist" change in both AlterSubscription_refresh and
CreateSubscription.
+            if (validate_publication)
+            {
+                /* Verify specified publications exists in the publisher. */
+                check_publications(wrconn, publications);
+            }
+

Move /*no streaming */ to above NULL, NULL line:
+                                           NULL, NULL,
                                            NULL, NULL); /* no streaming */

Can we have a new function for below duplicate code? Something like:
void connect_and_check_pubs(Subscription *sub, List *publications);?
+                if (validate_publication)
+                {
+                    /* Load the library providing us libpq calls. */
+                    load_file("libpqwalreceiver", false);
+
+                    /* Try to connect to the publisher. */
+                    wrconn = walrcv_connect(sub->conninfo, true,
sub->name, &err);
+                    if (!wrconn)
+                        ereport(ERROR,
+                                (errmsg("could not connect to the
publisher: %s", err)));
+
+                    /* Verify specified publications exists in the
publisher. */
+                    check_publications(wrconn, stmt->publication);
+
+                    /* We are done with the remote side, close connection. */
+                    walrcv_disconnect(wrconn);
+                }

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 13, 2021 at 8:01 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 6:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > 2) How about
> > > +          Specifies whether the subscriber must verify the
> > > publications that are
> > > +       being subscribed to are present in the publisher. By default,
> > > the subscriber
> > > instead of
> > > +          Specifies whether the subscriber must verify if the specified
> > > +          publications are present in the publisher. By default, the subscriber
> > >
> >
> > Slightly reworded and modified.
>
> +         <para>
> +          When true, the command will try to verify if the specified
> +          publications that are subscribed is present in the publisher.
> +          The default is <literal>false</literal>.
> +         </para>
>
> "publications that are subscribed" is not right as the subscriber is
> not yet subscribed, it is "trying to subscribing", and it's not that
> the command "will try to verify", it actually verifies. So you can
> modify as follows:
>
> +         <para>
> +          When true, the command verifies if all the specified
> publications that are being subscribed to are present in the publisher
> and throws an error if any of the publication doesn't exist. The
> default is <literal>false</literal>.
> +         </para>
>
> > > 3) I think we can make below common code into a single function with
> > > flags to differentiate processing for both, something like:
> > > StringInfoData *get_publist_str(List *publicaitons, bool use_quotes,
> > > bool is_fetch_table_list);
> > > check_publications:
> > > +        /* Convert the publications which does not exist into a string. */
> > > +        initStringInfo(&nonExistentPublications);
> > > +        foreach(lc, publicationsCopy)
> > > +        {
> > > and get_appended_publications_query:
> > >      foreach(lc, publications)
> > >
> > > With the new function that only prepares comma separated list of
> > > publications, you can get rid of get_appended_publications_query and
> > > just append the returned list to the query.
> > > fetch_table_list: get_publist_str(publications, true, true);
> > > check_publications: for select query preparation
> > > get_publist_str(publications, true, false); and for error string
> > > preparation get_publist_str(publications, false, false);
> > >
> > > And also let the new function get_publist_str allocate the string and
> > > just mention as a note in the function comment that the callers should
> > > pfree the returned string.
> > >
> >
> > I felt the existing code looks better, if we have a common function,
> > we will have to lot of if conditions as both the functions is not same
> > to same, they operate on different data types and do the preparation
> > appropriately. Like fetch_table_list get nspname & relname and
> > converts it to RangeVar and adds to the list other function prepares a
> > text and deletes the entries present from the list. So I did not fix
> > this. Thoughts?
>
> I was actually thinking we could move the following duplicate code
> into a function:
>         foreach(lc, publicationsCopy)
>         {
>             char       *pubname = strVal(lfirst(lc));
>
>             if (first)
>                 first = false;
>             else
>                 appendStringInfoString(&pubnames, ", ");
>             appendStringInfoString(&pubnames, "\"");
>             appendStringInfoString(&pubnames, pubname);
>             appendStringInfoString(&pubnames, "\"");
>         }
> and
>     foreach(lc, publications)
>     {
>         char       *pubname = strVal(lfirst(lc));
>
>         if (first)
>             first = false;
>         else
>             appendStringInfoString(cmd, ", ");
>
>         appendStringInfoString(cmd, quote_literal_cstr(pubname));
>     }
> that function can be:
> static void
> get_publications_str(List *publications, StringInfo dest, bool quote_literal)
> {
>     ListCell   *lc;
>     bool        first = true;
>
>     Assert(list_length(publications) > 0);
>
>     foreach(lc, publications)
>     {
>         char       *pubname = strVal(lfirst(lc));
>
>         if (first)
>             first = false;
>         else
>             appendStringInfoString(dest, ", ");
>
>         if (quote_literal)
>             appendStringInfoString(pubnames, quote_literal_cstr(pubname));
>         else
>         {
>             appendStringInfoString(&dest, "\"");
>             appendStringInfoString(&dest, pubname);
>             appendStringInfoString(&dest, "\"");
>         }
>     }
> }
>
> This way, we can get rid of get_appended_publications_query and use
> the above function to return the appended list of publications. We
> need to just pass quote_literal as true while preparing the publist
> string for publication query and append it to the query outside the
> function. While preparing publist str for error, pass quote_literal as
> false. Thoughts?
>

Modified.

> > > 7) You can just do
> > > publications = list_copy(publications);
> > > instead of using another variable publicationsCopy
> > >     publicationsCopy = list_copy(publications);
> >
> > publications is an input list to this function, I did not want this
> > function to change this list. I felt existing is fine. Thoughts?
>
> Okay.
>
> Typo - it's not "subcription" +# Create subcription for a publication
> which does not exist.
>

Modified

> I think we can remove extra { } by moving the comment above if clause
> much like you did in AlterSubscription_refresh. And it's not "exists",
> it is "exist" change in both AlterSubscription_refresh and
> CreateSubscription.
> +            if (validate_publication)
> +            {
> +                /* Verify specified publications exists in the publisher. */
> +                check_publications(wrconn, publications);
> +            }
> +

Modified.

>
> Move /*no streaming */ to above NULL, NULL line:
> +                                           NULL, NULL,
>                                             NULL, NULL); /* no streaming */
>

Modified.

> Can we have a new function for below duplicate code? Something like:
> void connect_and_check_pubs(Subscription *sub, List *publications);?
> +                if (validate_publication)
> +                {
> +                    /* Load the library providing us libpq calls. */
> +                    load_file("libpqwalreceiver", false);
> +
> +                    /* Try to connect to the publisher. */
> +                    wrconn = walrcv_connect(sub->conninfo, true,
> sub->name, &err);
> +                    if (!wrconn)
> +                        ereport(ERROR,
> +                                (errmsg("could not connect to the
> publisher: %s", err)));
> +
> +                    /* Verify specified publications exists in the
> publisher. */
> +                    check_publications(wrconn, stmt->publication);
> +
> +                    /* We are done with the remote side, close connection. */
> +                    walrcv_disconnect(wrconn);
> +                }
Modified.

Thanks for the comments, Attached patch has the fixes for the same.
Thoughts?

Regards,
Vignesh

On Sat, May 1, 2021 at 12:49 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the comments, Attached patch has the fixes for the same.
> Thoughts?

Few more comments on v5:

1) Deletion of below empty new line is spurious:
-
 /*
  * Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
  *

2) I think we could just do as below to save indentation of the code
for validate_publication == true.
static void
+connect_and_check_pubs(Subscription *sub, List *publications,
+                       bool validate_publication)
+{
+    char       *err;
+
+    if (validate_pulication == false )
+        return;
+
+        /* Load the library providing us libpq calls. */
+        load_file("libpqwalreceiver", false);

3) To be consistent, either we pass in validate_publication to both
connect_and_check_pubs and check_publications, return immediately from
them if it is false or do the checks outside. I suggest to pass in the
bool parameter to check_publications like you did for
connect_and_check_pubs. Or remove validate_publication from
connect_and_check_pubs and do the check outside.
+            if (validate_publication)
+                check_publications(wrconn, publications);
+        if (check_pub)
+            check_publications(wrconn, sub->publications);

4) Below line of code is above 80-char limit:
+        else if (strcmp(defel->defname, "validate_publication") == 0
&& validate_publication)

5) Instead of adding a new file 021_validate_publications.pl for
tests, spawning a new test database which would make the overall
regression slower, can't we add with the existing database nodes in
0001_rep_changes.pl? I would suggest adding the tests in there even if
the number of tests are many, I don't mind.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 1, 2021 at 7:58 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, May 1, 2021 at 12:49 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the comments, Attached patch has the fixes for the same.
> > Thoughts?
>
> Few more comments on v5:
>
> 1) Deletion of below empty new line is spurious:
> -
>  /*
>   * Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
>   *
>

Modified.

> 2) I think we could just do as below to save indentation of the code
> for validate_publication == true.
> static void
> +connect_and_check_pubs(Subscription *sub, List *publications,
> +                       bool validate_publication)
> +{
> +    char       *err;
> +
> +    if (validate_pulication == false )
> +        return;
> +
> +        /* Load the library providing us libpq calls. */
> +        load_file("libpqwalreceiver", false);
>

Modified.

> 3) To be consistent, either we pass in validate_publication to both
> connect_and_check_pubs and check_publications, return immediately from
> them if it is false or do the checks outside. I suggest to pass in the
> bool parameter to check_publications like you did for
> connect_and_check_pubs. Or remove validate_publication from
> connect_and_check_pubs and do the check outside.
> +            if (validate_publication)
> +                check_publications(wrconn, publications);
> +        if (check_pub)
> +            check_publications(wrconn, sub->publications);
>

Modified.

> 4) Below line of code is above 80-char limit:
> +        else if (strcmp(defel->defname, "validate_publication") == 0
> && validate_publication)
>

Modified

> 5) Instead of adding a new file 021_validate_publications.pl for
> tests, spawning a new test database which would make the overall
> regression slower, can't we add with the existing database nodes in
> 0001_rep_changes.pl? I would suggest adding the tests in there even if
> the number of tests are many, I don't mind.

001_rep_changes.pl has the changes mainly for checking the replicated
data. I did not find an appropriate file in the current tap tests, I
preferred these tests to be in a separate file. Thoughts?

Thanks for the comments.
The Attached patch has the fixes for the same.

Regards,
Vignesh

On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments.
> The Attached patch has the fixes for the same.

I was reviewing the documentation part, I think in the below paragraph
we should include validate_publication as well?

       <varlistentry>
        <term><literal>connect</literal> (<type>boolean</type>)</term>
        <listitem>
         <para>
          Specifies whether the <command>CREATE SUBSCRIPTION</command>
          should connect to the publisher at all.  Setting this to
          <literal>false</literal> will change default values of
          <literal>enabled</literal>, <literal>create_slot</literal> and
          <literal>copy_data</literal> to <literal>false</literal>.
         </para>

I will review/test the other parts of the patch and let you know.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> > 5) Instead of adding a new file 021_validate_publications.pl for
> > tests, spawning a new test database which would make the overall
> > regression slower, can't we add with the existing database nodes in
> > 0001_rep_changes.pl? I would suggest adding the tests in there even if
> > the number of tests are many, I don't mind.
>
> 001_rep_changes.pl has the changes mainly for checking the replicated
> data. I did not find an appropriate file in the current tap tests, I
> preferred these tests to be in a separate file. Thoughts?

If 001_rep_changes.pl is not the right place, how about adding them
into 007_ddl.pl? That file seems to be only for DDL changes, and since
the feature tests cases are for CREATE/ALTER SUBSCRIPTION, it's the
right place. I strongly feel that we don't need a new file for these
tests.

Comment on the tests:
1) Instead of "pub_doesnt_exist" name, how about "non_existent_pub" or
just pub_non_existent" or some other?
+       "CREATE SUBSCRIPTION testsub2 CONNECTION '$publisher_connstr'
PUBLICATION pub_doesnt_exist WITH (VALIDATE_PUBLICATION = TRUE)"
The error message with this name looks a bit odd to me.
+         /ERROR:  publication "pub_doesnt_exist" does not exist in
the publisher/,

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 3, 2021 at 10:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments.
> > The Attached patch has the fixes for the same.
>
> I was reviewing the documentation part, I think in the below paragraph
> we should include validate_publication as well?
>
>        <varlistentry>
>         <term><literal>connect</literal> (<type>boolean</type>)</term>
>         <listitem>
>          <para>
>           Specifies whether the <command>CREATE SUBSCRIPTION</command>
>           should connect to the publisher at all.  Setting this to
>           <literal>false</literal> will change default values of
>           <literal>enabled</literal>, <literal>create_slot</literal> and
>           <literal>copy_data</literal> to <literal>false</literal>.
>          </para>
>
> I will review/test the other parts of the patch and let you know.

I have reviewed it and it mostly looks good to me.  I have some minor
suggestions though.

1.
+/*
+ * Check the specified publication(s) is(are) present in the publisher.
+ */

vs

+
+/*
+ * Connect to the publisher and check if the publications exist.
+ */

I think the formatting of the comments are not uniform.  Some places
we are using "publication(s) is(are)" whereas other places are just
"publications".

2. Add a error case for connect=false and VALIDATE_PUBLICATION = true



--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 3, 2021 at 1:46 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 10:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments.
> > > The Attached patch has the fixes for the same.
> >
> > I was reviewing the documentation part, I think in the below paragraph
> > we should include validate_publication as well?
> >
> >        <varlistentry>
> >         <term><literal>connect</literal> (<type>boolean</type>)</term>
> >         <listitem>
> >          <para>
> >           Specifies whether the <command>CREATE SUBSCRIPTION</command>
> >           should connect to the publisher at all.  Setting this to
> >           <literal>false</literal> will change default values of
> >           <literal>enabled</literal>, <literal>create_slot</literal> and
> >           <literal>copy_data</literal> to <literal>false</literal>.
> >          </para>
> >

Modified.

> > I will review/test the other parts of the patch and let you know.
>
> I have reviewed it and it mostly looks good to me.  I have some minor
> suggestions though.
>
> 1.
> +/*
> + * Check the specified publication(s) is(are) present in the publisher.
> + */
>
> vs
>
> +
> +/*
> + * Connect to the publisher and check if the publications exist.
> + */
>
> I think the formatting of the comments are not uniform.  Some places
> we are using "publication(s) is(are)" whereas other places are just
> "publications".
>

Modified.

> 2. Add a error case for connect=false and VALIDATE_PUBLICATION = true

Added.

Thanks for the comments, attached v7 patch has the fixes for the same.
Thoughts?

Regards,
Vignesh

On Mon, May 3, 2021 at 11:11 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sun, May 2, 2021 at 10:04 PM vignesh C <vignesh21@gmail.com> wrote:
> > > 5) Instead of adding a new file 021_validate_publications.pl for
> > > tests, spawning a new test database which would make the overall
> > > regression slower, can't we add with the existing database nodes in
> > > 0001_rep_changes.pl? I would suggest adding the tests in there even if
> > > the number of tests are many, I don't mind.
> >
> > 001_rep_changes.pl has the changes mainly for checking the replicated
> > data. I did not find an appropriate file in the current tap tests, I
> > preferred these tests to be in a separate file. Thoughts?
>
> If 001_rep_changes.pl is not the right place, how about adding them
> into 007_ddl.pl? That file seems to be only for DDL changes, and since
> the feature tests cases are for CREATE/ALTER SUBSCRIPTION, it's the
> right place. I strongly feel that we don't need a new file for these
> tests.
>

Modified.

> Comment on the tests:
> 1) Instead of "pub_doesnt_exist" name, how about "non_existent_pub" or
> just pub_non_existent" or some other?
> +       "CREATE SUBSCRIPTION testsub2 CONNECTION '$publisher_connstr'
> PUBLICATION pub_doesnt_exist WITH (VALIDATE_PUBLICATION = TRUE)"
> The error message with this name looks a bit odd to me.
> +         /ERROR:  publication "pub_doesnt_exist" does not exist in
> the publisher/,

Modified.

Thanks for the comments, these comments are handle in the v7 patch
posted in my earlier mail.

Regards,
Vignesh

On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the comments, these comments are handle in the v7 patch
> posted in my earlier mail.

Thanks. Some comments on v7 patch:

1) How about "Add publication names from the list to a string."
 instead of
 * Append the list of publication to dest string.

2) How about "Connect to the publisher and see if the given
publication(s) is(are) present."
instead of
 * Connect to the publisher and check if the publication(s) exist.

3) Below comments are unnecessary as the functions/code following them
will tell what the code does.
    /* Verify specified publication(s) exist in the publisher. */
    /* We are done with the remote side, close connection. */

    /* Verify specified publication(s) exist in the publisher. */
    PG_TRY();
    {
        check_publications(wrconn, publications, true);
    }
    PG_FINALLY();
    {
        /* We are done with the remote side, close connection. */
        walrcv_disconnect(wrconn);
    }

4) And also the comment below that's there before check_publications
is unnecessary, as the function name and description would say it all.
/* Verify specified publication(s) exist in the publisher. */

5) A typo -  it is "do not exist"
# Multiple publications does not exist.

6) Should we use "m" specified in all the test cases something like we
do for $stderr =~ m/threads are not supported on this platform/ or
m/replication slot "test_slot" was not created in this database/?
$stderr =~
         /ERROR:  publication "non_existent_pub" does not exist in the
publisher/,

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 4, 2021 at 2:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the comments, these comments are handle in the v7 patch
> > posted in my earlier mail.
>
> Thanks. Some comments on v7 patch:
>
> 1) How about "Add publication names from the list to a string."
>  instead of
>  * Append the list of publication to dest string.
>

Modified.

> 2) How about "Connect to the publisher and see if the given
> publication(s) is(are) present."
> instead of
>  * Connect to the publisher and check if the publication(s) exist.
>

Modified.

> 3) Below comments are unnecessary as the functions/code following them
> will tell what the code does.
>     /* Verify specified publication(s) exist in the publisher. */
>     /* We are done with the remote side, close connection. */
>
>     /* Verify specified publication(s) exist in the publisher. */
>     PG_TRY();
>     {
>         check_publications(wrconn, publications, true);
>     }
>     PG_FINALLY();
>     {
>         /* We are done with the remote side, close connection. */
>         walrcv_disconnect(wrconn);
>     }
>

Modified.

> 4) And also the comment below that's there before check_publications
> is unnecessary, as the function name and description would say it all.
> /* Verify specified publication(s) exist in the publisher. */
>

Modified.

> 5) A typo -  it is "do not exist"
> # Multiple publications does not exist.
>

Modified.

> 6) Should we use "m" specified in all the test cases something like we
> do for $stderr =~ m/threads are not supported on this platform/ or
> m/replication slot "test_slot" was not created in this database/?
> $stderr =~
>          /ERROR:  publication "non_existent_pub" does not exist in the
> publisher/,

Modified.

Thanks for the comments, Attached patch has the fixes for the same.

Regards,
Vignesh

On Tue, May 4, 2021 at 6:50 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the comments, Attached patch has the fixes for the same.

Thanks! I took a final look over the v8 patch, it looks good to me and
regression tests were passed with it. I have no further comments at
this moment. I will make it "ready for committer" if others have no
comments.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, 04 May 2021 at 21:20, vignesh C <vignesh21@gmail.com> wrote:
> On Tue, May 4, 2021 at 2:37 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21@gmail.com> wrote:
>> > Thanks for the comments, these comments are handle in the v7 patch
>> > posted in my earlier mail.
>>
>> Thanks. Some comments on v7 patch:
>>
>> 1) How about "Add publication names from the list to a string."
>>  instead of
>>  * Append the list of publication to dest string.
>>
>
> Modified.
>
>> 2) How about "Connect to the publisher and see if the given
>> publication(s) is(are) present."
>> instead of
>>  * Connect to the publisher and check if the publication(s) exist.
>>
>
> Modified.
>
>> 3) Below comments are unnecessary as the functions/code following them
>> will tell what the code does.
>>     /* Verify specified publication(s) exist in the publisher. */
>>     /* We are done with the remote side, close connection. */
>>
>>     /* Verify specified publication(s) exist in the publisher. */
>>     PG_TRY();
>>     {
>>         check_publications(wrconn, publications, true);
>>     }
>>     PG_FINALLY();
>>     {
>>         /* We are done with the remote side, close connection. */
>>         walrcv_disconnect(wrconn);
>>     }
>>
>
> Modified.
>
>> 4) And also the comment below that's there before check_publications
>> is unnecessary, as the function name and description would say it all.
>> /* Verify specified publication(s) exist in the publisher. */
>>
>
> Modified.
>
>> 5) A typo -  it is "do not exist"
>> # Multiple publications does not exist.
>>
>
> Modified.
>
>> 6) Should we use "m" specified in all the test cases something like we
>> do for $stderr =~ m/threads are not supported on this platform/ or
>> m/replication slot "test_slot" was not created in this database/?
>> $stderr =~
>>          /ERROR:  publication "non_existent_pub" does not exist in the
>> publisher/,
>
> Modified.
>
> Thanks for the comments, Attached patch has the fixes for the same.

Thanks for updating the patch.  Some comments on v8 patch.

1) How about use appendStringInfoChar() to replace the first and last one,
since it more faster.
+                       appendStringInfoString(dest, "\"");
+                       appendStringInfoString(dest, pubname);
+                       appendStringInfoString(dest, "\"");

2) How about use if (!validate_publication) to keep the code style consistent?
+       if (validate_publication == false)
+               return;

3) Should we free the memory when finish the check_publications()?
+       publicationsCopy = list_copy(publications);

4) It is better wrap the word "streaming" with quote.  Also, should we add
'no "validate_publication"' comment for validate_publication parameters?
                                                                                   NULL, NULL,  /* no "binary" */
-                                                                                  NULL, NULL); /* no streaming */
+                                                                                  NULL, NULL,  /* no streaming */
+                                                                                  NULL, NULL);

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, May 6, 2021 at 9:08 AM Japin Li <japinli@hotmail.com> wrote:
>
>
> On Tue, 04 May 2021 at 21:20, vignesh C <vignesh21@gmail.com> wrote:
> > On Tue, May 4, 2021 at 2:37 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> On Mon, May 3, 2021 at 7:59 PM vignesh C <vignesh21@gmail.com> wrote:
> >> > Thanks for the comments, these comments are handle in the v7 patch
> >> > posted in my earlier mail.
> >>
> >> Thanks. Some comments on v7 patch:
> >>
> >> 1) How about "Add publication names from the list to a string."
> >>  instead of
> >>  * Append the list of publication to dest string.
> >>
> >
> > Modified.
> >
> >> 2) How about "Connect to the publisher and see if the given
> >> publication(s) is(are) present."
> >> instead of
> >>  * Connect to the publisher and check if the publication(s) exist.
> >>
> >
> > Modified.
> >
> >> 3) Below comments are unnecessary as the functions/code following them
> >> will tell what the code does.
> >>     /* Verify specified publication(s) exist in the publisher. */
> >>     /* We are done with the remote side, close connection. */
> >>
> >>     /* Verify specified publication(s) exist in the publisher. */
> >>     PG_TRY();
> >>     {
> >>         check_publications(wrconn, publications, true);
> >>     }
> >>     PG_FINALLY();
> >>     {
> >>         /* We are done with the remote side, close connection. */
> >>         walrcv_disconnect(wrconn);
> >>     }
> >>
> >
> > Modified.
> >
> >> 4) And also the comment below that's there before check_publications
> >> is unnecessary, as the function name and description would say it all.
> >> /* Verify specified publication(s) exist in the publisher. */
> >>
> >
> > Modified.
> >
> >> 5) A typo -  it is "do not exist"
> >> # Multiple publications does not exist.
> >>
> >
> > Modified.
> >
> >> 6) Should we use "m" specified in all the test cases something like we
> >> do for $stderr =~ m/threads are not supported on this platform/ or
> >> m/replication slot "test_slot" was not created in this database/?
> >> $stderr =~
> >>          /ERROR:  publication "non_existent_pub" does not exist in the
> >> publisher/,
> >
> > Modified.
> >
> > Thanks for the comments, Attached patch has the fixes for the same.
>
> Thanks for updating the patch.  Some comments on v8 patch.
>
> 1) How about use appendStringInfoChar() to replace the first and last one,
> since it more faster.
> +                       appendStringInfoString(dest, "\"");
> +                       appendStringInfoString(dest, pubname);
> +                       appendStringInfoString(dest, "\"");

Modified.

> 2) How about use if (!validate_publication) to keep the code style consistent?
> +       if (validate_publication == false)
> +               return;

Modified.

> 3) Should we free the memory when finish the check_publications()?
> +       publicationsCopy = list_copy(publications);

I felt this list entries will be deleted in the success case, in error
case I felt no need to delete it as we will be exiting. Thoughts?

> 4) It is better wrap the word "streaming" with quote.  Also, should we add
> 'no "validate_publication"' comment for validate_publication parameters?
>                                                                                    NULL, NULL,  /* no "binary" */
> -                                                                                  NULL, NULL); /* no streaming */
> +                                                                                  NULL, NULL,  /* no streaming */
> +                                                                                  NULL, NULL);

Modified.

Thanks for the comments, attached v9 patch has the fixes for the same.

Regards,
Vignesh

On Thu, 06 May 2021 at 21:52, vignesh C <vignesh21@gmail.com> wrote:
> On Thu, May 6, 2021 at 9:08 AM Japin Li <japinli@hotmail.com> wrote:
>> 3) Should we free the memory when finish the check_publications()?
>> +       publicationsCopy = list_copy(publications);
>
> I felt this list entries will be deleted in the success case, in error
> case I felt no need to delete it as we will be exiting. Thoughts?
>

Sorry for the noise! You are right. The v9 patch set looks good to me.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
>

Some comments:
1.
I don't see any change in pg_dump.c, don't we need to dump this option?

2.
+ /* Try to connect to the publisher. */
+ wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
+ if (!wrconn)
+ ereport(ERROR,
+ (errmsg("could not connect to the publisher: %s", err)));

Instead of using global wrconn, I think you should use a local variable?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 7, 2021 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> Some comments:
> 1.
> I don't see any change in pg_dump.c, don't we need to dump this option?

I don't think it is necessary there as the default value of the
validate_publication is false, so even if the pg_dump has no mention
of the option, then it is assumed to be false while restoring. Note
that the validate_publication option is transient (like with other
options such as create_slot, copy_data) which means it can't be stored
in pg_subscritpion catalogue. Therefore, user specified value can't be
fetched once the CREATE/ALTER subscription command is finished. If we
were to dump the option, we should be storing it in the catalogue,
which I don't think is necessary. Thoughts?

> 2.
> + /* Try to connect to the publisher. */
> + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> + if (!wrconn)
> + ereport(ERROR,
> + (errmsg("could not connect to the publisher: %s", err)));
>
> Instead of using global wrconn, I think you should use a local variable?

Yeah, we should be using local wrconn, otherwise there can be
consequences, see the patches at [1]. Thanks for pointing out this.

[1] - https://www.postgresql.org/message-id/CAHut%2BPuSwWmmeK%2Bfe6E2duep8588Jk82XXH73nE4dUxwDNkNUg%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 7, 2021 at 5:38 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> >
> > Some comments:
> > 1.
> > I don't see any change in pg_dump.c, don't we need to dump this option?
>
> I don't think it is necessary there as the default value of the
> validate_publication is false, so even if the pg_dump has no mention
> of the option, then it is assumed to be false while restoring. Note
> that the validate_publication option is transient (like with other
> options such as create_slot, copy_data) which means it can't be stored
> in pg_subscritpion catalogue. Therefore, user specified value can't be
> fetched once the CREATE/ALTER subscription command is finished. If we
> were to dump the option, we should be storing it in the catalogue,
> which I don't think is necessary. Thoughts?

If we are not storing it in the catalog then it does not need to be dumped.

> > 2.
> > + /* Try to connect to the publisher. */
> > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > + if (!wrconn)
> > + ereport(ERROR,
> > + (errmsg("could not connect to the publisher: %s", err)));
> >
> > Instead of using global wrconn, I think you should use a local variable?
>
> Yeah, we should be using local wrconn, otherwise there can be
> consequences, see the patches at [1]. Thanks for pointing out this.

Right.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 7, 2021 at 5:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 5:38 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, May 7, 2021 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > >
> > > Some comments:
> > > 1.
> > > I don't see any change in pg_dump.c, don't we need to dump this option?
> >
> > I don't think it is necessary there as the default value of the
> > validate_publication is false, so even if the pg_dump has no mention
> > of the option, then it is assumed to be false while restoring. Note
> > that the validate_publication option is transient (like with other
> > options such as create_slot, copy_data) which means it can't be stored
> > in pg_subscritpion catalogue. Therefore, user specified value can't be
> > fetched once the CREATE/ALTER subscription command is finished. If we
> > were to dump the option, we should be storing it in the catalogue,
> > which I don't think is necessary. Thoughts?
>
> If we are not storing it in the catalog then it does not need to be dumped.

I intentionally did not store this value, I felt we need not persist
this option's value. This value will be false while dumping similar to
other non stored parameters.

> > > 2.
> > > + /* Try to connect to the publisher. */
> > > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > > + if (!wrconn)
> > > + ereport(ERROR,
> > > + (errmsg("could not connect to the publisher: %s", err)));
> > >
> > > Instead of using global wrconn, I think you should use a local variable?
> >
> > Yeah, we should be using local wrconn, otherwise there can be
> > consequences, see the patches at [1]. Thanks for pointing out this.

Modified.

Thanks for the comments, the attached patch has the fix for the same.

Regards,
Vignesh

On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 5:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, May 7, 2021 at 5:38 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Fri, May 7, 2021 at 11:50 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Thu, May 6, 2021 at 7:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > >
> > > > Some comments:
> > > > 1.
> > > > I don't see any change in pg_dump.c, don't we need to dump this option?
> > >
> > > I don't think it is necessary there as the default value of the
> > > validate_publication is false, so even if the pg_dump has no mention
> > > of the option, then it is assumed to be false while restoring. Note
> > > that the validate_publication option is transient (like with other
> > > options such as create_slot, copy_data) which means it can't be stored
> > > in pg_subscritpion catalogue. Therefore, user specified value can't be
> > > fetched once the CREATE/ALTER subscription command is finished. If we
> > > were to dump the option, we should be storing it in the catalogue,
> > > which I don't think is necessary. Thoughts?
> >
> > If we are not storing it in the catalog then it does not need to be dumped.
>
> I intentionally did not store this value, I felt we need not persist
> this option's value. This value will be false while dumping similar to
> other non stored parameters.
>
> > > > 2.
> > > > + /* Try to connect to the publisher. */
> > > > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > > > + if (!wrconn)
> > > > + ereport(ERROR,
> > > > + (errmsg("could not connect to the publisher: %s", err)));
> > > >
> > > > Instead of using global wrconn, I think you should use a local variable?
> > >
> > > Yeah, we should be using local wrconn, otherwise there can be
> > > consequences, see the patches at [1]. Thanks for pointing out this.
>
> Modified.
>
> Thanks for the comments, the attached patch has the fix for the same.

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh

On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached patch has the fix for the same.
>
> The patch was not applying on the head, attached patch which is rebased on HEAD.

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh

On Wed, Jun 30, 2021 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments, the attached patch has the fix for the same.
> >
> > The patch was not applying on the head, attached patch which is rebased on HEAD.
>
> The patch was not applying on the head, attached patch which is rebased on HEAD.

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh

On Tue, Jul 6, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Jun 30, 2021 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > Thanks for the comments, the attached patch has the fix for the same.
> > >
> > > The patch was not applying on the head, attached patch which is rebased on HEAD.
> >
> > The patch was not applying on the head, attached patch which is rebased on HEAD.
>
> The patch was not applying on the head, attached patch which is rebased on HEAD.

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh

On Thu, Jul 15, 2021 at 5:57 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Jul 6, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, Jun 30, 2021 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > Thanks for the comments, the attached patch has the fix for the same.
> > > >

The patch was not applying on the head, attached patch which is rebased on HEAD.

Regards,
Vignesh

On Thu, Aug 26, 2021 at 07:49:49PM +0530, vignesh C wrote:
> On Thu, Jul 15, 2021 at 5:57 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, Jul 6, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Wed, Jun 30, 2021 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > >
> > > > > > Thanks for the comments, the attached patch has the fix for the same.
> > > > >
> 
> The patch was not applying on the head, attached patch which is rebased on HEAD.
> 

Hi,

I'm testing this patch now. It doesn't apply cleanly but is the
documentation part, so while a rebase would be good it doesn't avoid me
to test.

A couple of questions:

+check_publications(WalReceiverConn *wrconn, List *publications,
+                                  bool validate_publication)
[...]
+connect_and_check_pubs(Subscription *sub, List *publications,
+                                          bool validate_publication)

I wonder why validate_publication is passed as an argument just to
return if it's false, why not just test it before calling those
functions? Maybe is just a matter of style.

+get_publications_str(List *publications, StringInfo dest, bool quote_literal)

what's the purpose of the quote_literal argument?

-- 
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

On Tue, Sep 28, 2021 at 7:49 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:
>
> On Thu, Aug 26, 2021 at 07:49:49PM +0530, vignesh C wrote:
> > On Thu, Jul 15, 2021 at 5:57 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Tue, Jul 6, 2021 at 8:09 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Wed, Jun 30, 2021 at 8:23 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jun 6, 2021 at 11:55 AM vignesh C <vignesh21@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, May 7, 2021 at 6:44 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > > >
> > > > > > > Thanks for the comments, the attached patch has the fix for the same.
> > > > > >
> >
> > The patch was not applying on the head, attached patch which is rebased on HEAD.
> >
>
> Hi,
>
> I'm testing this patch now. It doesn't apply cleanly but is the
> documentation part, so while a rebase would be good it doesn't avoid me
> to test.

I have rebased the patch on top of Head.

> A couple of questions:
>
> +check_publications(WalReceiverConn *wrconn, List *publications,
> +                                  bool validate_publication)
> [...]
> +connect_and_check_pubs(Subscription *sub, List *publications,
> +                                          bool validate_publication)
>
> I wonder why validate_publication is passed as an argument just to
> return if it's false, why not just test it before calling those
> functions? Maybe is just a matter of style.

I felt it will be better to have the check inside function so that it
need not be checked at the multiple caller function.

> +get_publications_str(List *publications, StringInfo dest, bool quote_literal)
>
> what's the purpose of the quote_literal argument?

In case of error the publication that is not present will be displayed
within double quotes like below:
ERROR:  publications "pub3", "pub4" do not exist in the publisher
Whereas in case of the query we use single quotes, so quote_literal is
used to differentiate and handle accordingly.

Attached v12 version is rebased on top of Head.

Regards,
Vignesh

On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignesh21@gmail.com> wrote:
> Attached v12 version is rebased on top of Head.

Thanks for the patch. Here are some comments on v12:

1) I think ERRCODE_TOO_MANY_ARGUMENTS isn't the right error code, the
ERRCODE_UNDEFINED_OBJECT is more meaningful. Please change.
+ ereport(ERROR,
+ (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
+ errmsg_plural("publication %s does not exist in the publisher",
+    "publications %s do not exist in the publisher",

The existing code using
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
                 errmsg("subscription \"%s\" does not exist", subname)));

2) Typo: It is "One of the specified publications exists."
+# One of the specified publication exist.

3) I think we can remove the test case "+# Specified publication does
not exist." because the "+# One of the specified publication exist."
covers the code.

4) Do we need the below test case? Even with refresh = false, it does
call connect_and_check_pubs() right? Please remove it.
+# Specified publication does not exist with refresh = false.
+($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
+       "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub
WITH (REFRESH = FALSE, VALIDATE_PUBLICATION = TRUE)"
+);
+ok( $stderr =~
+         m/ERROR:  publication "non_existent_pub" does not exist in
the publisher/,
+       "Alter subscription for non existent publication fails");
+

5) Change the test case names to different ones instead of the same.
Have something like:
"Create subscription fails with single non-existent publication");
"Create subscription fails with multiple non-existent publications");
"Create subscription fails with mutually exclusive options");
"Alter subscription add publication fails with non-existent publication");
"Alter subscription set publication fails with non-existent publication");
"Alter subscription set publication fails with connection to a
non-existent database");

Unnecessary test cases would add up to the "make check-world" times,
please remove them.

Regards,
Bharath Rupireddy.

On Wed, Nov 10, 2021 at 11:16 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignesh21@gmail.com> wrote:
> > Attached v12 version is rebased on top of Head.
>
> Thanks for the patch. Here are some comments on v12:
>
> 1) I think ERRCODE_TOO_MANY_ARGUMENTS isn't the right error code, the
> ERRCODE_UNDEFINED_OBJECT is more meaningful. Please change.
> + ereport(ERROR,
> + (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
> + errmsg_plural("publication %s does not exist in the publisher",
> +    "publications %s do not exist in the publisher",
>
> The existing code using
>         ereport(ERROR,
>                 (errcode(ERRCODE_UNDEFINED_OBJECT),
>                  errmsg("subscription \"%s\" does not exist", subname)));

Modified

> 2) Typo: It is "One of the specified publications exists."
> +# One of the specified publication exist.

Modified

> 3) I think we can remove the test case "+# Specified publication does
> not exist." because the "+# One of the specified publication exist."
> covers the code.

Modified

> 4) Do we need the below test case? Even with refresh = false, it does
> call connect_and_check_pubs() right? Please remove it.
> +# Specified publication does not exist with refresh = false.
> +($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
> +       "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub
> WITH (REFRESH = FALSE, VALIDATE_PUBLICATION = TRUE)"
> +);
> +ok( $stderr =~
> +         m/ERROR:  publication "non_existent_pub" does not exist in
> the publisher/,
> +       "Alter subscription for non existent publication fails");
> +

Modified

> 5) Change the test case names to different ones instead of the same.
> Have something like:
> "Create subscription fails with single non-existent publication");
> "Create subscription fails with multiple non-existent publications");
> "Create subscription fails with mutually exclusive options");
> "Alter subscription add publication fails with non-existent publication");
> "Alter subscription set publication fails with non-existent publication");
> "Alter subscription set publication fails with connection to a
> non-existent database");

Modified

Thanks for the comments, the attached v13 patch has the fixes for the same.

Regards,
Vignesh

On Sat, Nov 13, 2021 at 12:50 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v13 patch has the fixes for the same.

Thanks for the updated v13 patch. I have no further comments, it looks
good to me.

Regards,
Bharath Rupireddy.

Just wondering if we should also be detecting the incorrect conninfo
set with ALTER SUBSCRIPTION command as well. See below:

-- try creating a subscription with incorrect conninfo. the command fails.
postgres=# create subscription sub1 connection 'host=localhost
port=5490 dbname=postgres' publication pub1;
ERROR:  could not connect to the publisher: connection to server at
"localhost" (::1), port 5490 failed: Connection refused
    Is the server running on that host and accepting TCP/IP connections?
connection to server at "localhost" (127.0.0.1), port 5490 failed:
Connection refused
    Is the server running on that host and accepting TCP/IP connections?
postgres=#
postgres=#

-- this time the conninfo is correct and the command succeeded.
postgres=# create subscription sub1 connection 'host=localhost
port=5432 dbname=postgres' publication pub1;
NOTICE:  created replication slot "sub1" on publisher
CREATE SUBSCRIPTION
postgres=#
postgres=#

-- reset the connninfo in the subscription to some wrong value. the
command succeeds.
postgres=# alter subscription sub1 connection 'host=localhost
port=5490 dbname=postgres';
ALTER SUBSCRIPTION
postgres=#

postgres=# drop subscription sub1;
ERROR:  could not connect to publisher when attempting to drop
replication slot "sub1": connection to server at "localhost" (::1),
port 5490 failed: Connection refused
    Is the server running on that host and accepting TCP/IP connections?
connection to server at "localhost" (127.0.0.1), port 5490 failed:
Connection refused
    Is the server running on that host and accepting TCP/IP connections?
HINT:  Use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to
disassociate the subscription from the slot.

==

When creating a subscription we do connect to the publisher node hence
the incorrect connection info gets detected. But that's not the case
with alter subscription.

--
With Regards,
Ashutosh Sharma.

On Sat, Nov 13, 2021 at 6:27 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, Nov 13, 2021 at 12:50 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v13 patch has the fixes for the same.
>
> Thanks for the updated v13 patch. I have no further comments, it looks
> good to me.
>
> Regards,
> Bharath Rupireddy.
>
>

Just wondering if we should also be detecting the incorrect conninfo

set with ALTER SUBSCRIPTION command as well. See below:

-- try creating a subscription with incorrect conninfo. the command fails.

postgres=# create subscription sub1 connection 'host=localhost

port=5490 dbname=postgres' publication pub1;

ERROR:  could not connect to the publisher: connection to server at

"localhost" (::1), port 5490 failed: Connection refused

    Is the server running on that host and accepting TCP/IP connections?

connection to server at "localhost" (127.0.0.1), port 5490 failed:

Connection refused

    Is the server running on that host and accepting TCP/IP connections?

-- reset the connninfo in the subscription to some wrong value. the

command succeeds.

postgres=# alter subscription sub1 connection 'host=localhost

port=5490 dbname=postgres';

ALTER SUBSCRIPTION

postgres=#

postgres=# drop subscription sub1;

ERROR:  could not connect to publisher when attempting to drop

replication slot "sub1": connection to server at "localhost" (::1),

port 5490 failed: Connection refused

    Is the server running on that host and accepting TCP/IP connections?

connection to server at "localhost" (127.0.0.1), port 5490 failed:

Connection refused

    Is the server running on that host and accepting TCP/IP connections?

HINT:  Use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to

disassociate the subscription from the slot.

On Wed, Feb 9, 2022 at 11:53 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Feb 9, 2022, at 12:06 PM, Ashutosh Sharma wrote:
>
> Just wondering if we should also be detecting the incorrect conninfo
> set with ALTER SUBSCRIPTION command as well. See below:
>
> -- try creating a subscription with incorrect conninfo. the command fails.
> postgres=# create subscription sub1 connection 'host=localhost
> port=5490 dbname=postgres' publication pub1;
> ERROR:  could not connect to the publisher: connection to server at
> "localhost" (::1), port 5490 failed: Connection refused
>     Is the server running on that host and accepting TCP/IP connections?
> connection to server at "localhost" (127.0.0.1), port 5490 failed:
> Connection refused
>     Is the server running on that host and accepting TCP/IP connections?
>
> That's because by default 'connect' parameter is true.
>

So can we use this option with the ALTER SUBSCRIPTION command. I think
we can't, which means if the user sets wrong conninfo using ALTER
SUBSCRIPTION command then we don't have the option to detect it like
we have in case of CREATE SUBSCRIPTION command. Since this thread is
trying to add the ability to identify the wrong/missing publication
name specified with the ALTER SUBSCRIPTION command, can't we do the
same for the wrong conninfo?

--
With Regards,
Ashutosh Sharma.

I have spent little time looking at the latest patch. The patch looks
to be in good shape as it has already been reviewed by many people
here, although I did get some comments. Please take a look and let me
know your thoughts.


+   /* Try to connect to the publisher. */
+   wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
+   if (!wrconn)
+       ereport(ERROR,
+               (errmsg("could not connect to the publisher: %s", err)));

I think it would be good to also include the errcode
(ERRCODE_CONNECTION_FAILURE) here?

--

@@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,

        PG_TRY();
        {
+           check_publications(wrconn, publications, opts.validate_publication);
+


Instead of passing the opts.validate_publication argument to
check_publication function, why can't we first check if this option is
set or not and accordingly call check_publication function? For other
options I see that it has been done in the similar way for e.g. check
for opts.connect or opts.refresh or opts.enabled etc.

--

Above comment also applies for:

@@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
                replaces[Anum_pg_subscription_subpublications - 1] = true;

                update_tuple = true;
+               connect_and_check_pubs(sub, stmt->publication,
+                                      opts.validate_publication);


--

+         <para>
+          When true, the command verifies if all the specified publications
+          that are being subscribed to are present in the publisher and throws
+          an error if any of the publication doesn't exist. The default is
+          <literal>false</literal>.

publication -> publications (in the 4th line : throw an error if any
of the publication doesn't exist)

This applies for both CREATE and ALTER subscription commands.

--
With Regards,
Ashutosh Sharma.

On Sat, Nov 13, 2021 at 12:50 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Nov 10, 2021 at 11:16 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Tue, Nov 9, 2021 at 9:27 PM vignesh C <vignesh21@gmail.com> wrote:
> > > Attached v12 version is rebased on top of Head.
> >
> > Thanks for the patch. Here are some comments on v12:
> >
> > 1) I think ERRCODE_TOO_MANY_ARGUMENTS isn't the right error code, the
> > ERRCODE_UNDEFINED_OBJECT is more meaningful. Please change.
> > + ereport(ERROR,
> > + (errcode(ERRCODE_TOO_MANY_ARGUMENTS),
> > + errmsg_plural("publication %s does not exist in the publisher",
> > +    "publications %s do not exist in the publisher",
> >
> > The existing code using
> >         ereport(ERROR,
> >                 (errcode(ERRCODE_UNDEFINED_OBJECT),
> >                  errmsg("subscription \"%s\" does not exist", subname)));
>
> Modified
>
> > 2) Typo: It is "One of the specified publications exists."
> > +# One of the specified publication exist.
>
> Modified
>
> > 3) I think we can remove the test case "+# Specified publication does
> > not exist." because the "+# One of the specified publication exist."
> > covers the code.
>
> Modified
>
> > 4) Do we need the below test case? Even with refresh = false, it does
> > call connect_and_check_pubs() right? Please remove it.
> > +# Specified publication does not exist with refresh = false.
> > +($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
> > +       "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub
> > WITH (REFRESH = FALSE, VALIDATE_PUBLICATION = TRUE)"
> > +);
> > +ok( $stderr =~
> > +         m/ERROR:  publication "non_existent_pub" does not exist in
> > the publisher/,
> > +       "Alter subscription for non existent publication fails");
> > +
>
> Modified
>
> > 5) Change the test case names to different ones instead of the same.
> > Have something like:
> > "Create subscription fails with single non-existent publication");
> > "Create subscription fails with multiple non-existent publications");
> > "Create subscription fails with mutually exclusive options");
> > "Alter subscription add publication fails with non-existent publication");
> > "Alter subscription set publication fails with non-existent publication");
> > "Alter subscription set publication fails with connection to a
> > non-existent database");
>
> Modified
>
> Thanks for the comments, the attached v13 patch has the fixes for the same.
>
> Regards,
> Vignesh

On Thu, Feb 10, 2022 at 3:15 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> On Wed, Feb 9, 2022 at 11:53 PM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Wed, Feb 9, 2022, at 12:06 PM, Ashutosh Sharma wrote:
> >
> > Just wondering if we should also be detecting the incorrect conninfo
> > set with ALTER SUBSCRIPTION command as well. See below:
> >
> > -- try creating a subscription with incorrect conninfo. the command fails.
> > postgres=# create subscription sub1 connection 'host=localhost
> > port=5490 dbname=postgres' publication pub1;
> > ERROR:  could not connect to the publisher: connection to server at
> > "localhost" (::1), port 5490 failed: Connection refused
> >     Is the server running on that host and accepting TCP/IP connections?
> > connection to server at "localhost" (127.0.0.1), port 5490 failed:
> > Connection refused
> >     Is the server running on that host and accepting TCP/IP connections?
> >
> > That's because by default 'connect' parameter is true.
> >
>
> So can we use this option with the ALTER SUBSCRIPTION command. I think
> we can't, which means if the user sets wrong conninfo using ALTER
> SUBSCRIPTION command then we don't have the option to detect it like
> we have in case of CREATE SUBSCRIPTION command. Since this thread is
> trying to add the ability to identify the wrong/missing publication
> name specified with the ALTER SUBSCRIPTION command, can't we do the
> same for the wrong conninfo?

I felt this can be extended once this feature is committed. Thoughts?

Regards,
Vignesh

On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> I have spent little time looking at the latest patch. The patch looks
> to be in good shape as it has already been reviewed by many people
> here, although I did get some comments. Please take a look and let me
> know your thoughts.
>
>
> +   /* Try to connect to the publisher. */
> +   wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> +   if (!wrconn)
> +       ereport(ERROR,
> +               (errmsg("could not connect to the publisher: %s", err)));
>
> I think it would be good to also include the errcode
> (ERRCODE_CONNECTION_FAILURE) here?

Modified

> --
>
> @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> CreateSubscriptionStmt *stmt,
>
>         PG_TRY();
>         {
> +           check_publications(wrconn, publications, opts.validate_publication);
> +
>
>
> Instead of passing the opts.validate_publication argument to
> check_publication function, why can't we first check if this option is
> set or not and accordingly call check_publication function? For other
> options I see that it has been done in the similar way for e.g. check
> for opts.connect or opts.refresh or opts.enabled etc.

Modified

> --
>
> Above comment also applies for:
>
> @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> AlterSubscriptionStmt *stmt,
>                 replaces[Anum_pg_subscription_subpublications - 1] = true;
>
>                 update_tuple = true;
> +               connect_and_check_pubs(sub, stmt->publication,
> +                                      opts.validate_publication);
>

Modified

> --
>
> +         <para>
> +          When true, the command verifies if all the specified publications
> +          that are being subscribed to are present in the publisher and throws
> +          an error if any of the publication doesn't exist. The default is
> +          <literal>false</literal>.
>
> publication -> publications (in the 4th line : throw an error if any
> of the publication doesn't exist)
>
> This applies for both CREATE and ALTER subscription commands.

Modified

Thanks for the comments, the attached v14 patch has the changes for the same.

Regard,s
Vignesh

On Sun, Feb 13, 2022 at 7:32 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Feb 10, 2022 at 3:15 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > On Wed, Feb 9, 2022 at 11:53 PM Euler Taveira <euler@eulerto.com> wrote:
> > >
> > > On Wed, Feb 9, 2022, at 12:06 PM, Ashutosh Sharma wrote:
> > >
> > > Just wondering if we should also be detecting the incorrect conninfo
> > > set with ALTER SUBSCRIPTION command as well. See below:
> > >
> > > -- try creating a subscription with incorrect conninfo. the command fails.
> > > postgres=# create subscription sub1 connection 'host=localhost
> > > port=5490 dbname=postgres' publication pub1;
> > > ERROR:  could not connect to the publisher: connection to server at
> > > "localhost" (::1), port 5490 failed: Connection refused
> > >     Is the server running on that host and accepting TCP/IP connections?
> > > connection to server at "localhost" (127.0.0.1), port 5490 failed:
> > > Connection refused
> > >     Is the server running on that host and accepting TCP/IP connections?
> > >
> > > That's because by default 'connect' parameter is true.
> > >
> >
> > So can we use this option with the ALTER SUBSCRIPTION command. I think
> > we can't, which means if the user sets wrong conninfo using ALTER
> > SUBSCRIPTION command then we don't have the option to detect it like
> > we have in case of CREATE SUBSCRIPTION command. Since this thread is
> > trying to add the ability to identify the wrong/missing publication
> > name specified with the ALTER SUBSCRIPTION command, can't we do the
> > same for the wrong conninfo?
>
> I felt this can be extended once this feature is committed. Thoughts?
>

I think that should be okay. I just wanted to share with you people to
know if it can be taken care of in this patch itself but it's ok if we
see it later.

--
With Regards,
Ashutosh Sharma.

Thanks for working on my review comments. I'll take a look at the new
changes and let you know my comments, if any. I didn't get a chance to
check it out today as I was busy reviewing some other patches, but
I'll definitely take a look at the new patch in a day or so and let
you know my feedback.

--
With Regards,
Ashutosh Sharma.

On Sun, Feb 13, 2022 at 7:34 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > I have spent little time looking at the latest patch. The patch looks
> > to be in good shape as it has already been reviewed by many people
> > here, although I did get some comments. Please take a look and let me
> > know your thoughts.
> >
> >
> > +   /* Try to connect to the publisher. */
> > +   wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > +   if (!wrconn)
> > +       ereport(ERROR,
> > +               (errmsg("could not connect to the publisher: %s", err)));
> >
> > I think it would be good to also include the errcode
> > (ERRCODE_CONNECTION_FAILURE) here?
>
> Modified
>
> > --
> >
> > @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> > CreateSubscriptionStmt *stmt,
> >
> >         PG_TRY();
> >         {
> > +           check_publications(wrconn, publications, opts.validate_publication);
> > +
> >
> >
> > Instead of passing the opts.validate_publication argument to
> > check_publication function, why can't we first check if this option is
> > set or not and accordingly call check_publication function? For other
> > options I see that it has been done in the similar way for e.g. check
> > for opts.connect or opts.refresh or opts.enabled etc.
>
> Modified
>
> > --
> >
> > Above comment also applies for:
> >
> > @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> > AlterSubscriptionStmt *stmt,
> >                 replaces[Anum_pg_subscription_subpublications - 1] = true;
> >
> >                 update_tuple = true;
> > +               connect_and_check_pubs(sub, stmt->publication,
> > +                                      opts.validate_publication);
> >
>
> Modified
>
> > --
> >
> > +         <para>
> > +          When true, the command verifies if all the specified publications
> > +          that are being subscribed to are present in the publisher and throws
> > +          an error if any of the publication doesn't exist. The default is
> > +          <literal>false</literal>.
> >
> > publication -> publications (in the 4th line : throw an error if any
> > of the publication doesn't exist)
> >
> > This applies for both CREATE and ALTER subscription commands.
>
> Modified
>
> Thanks for the comments, the attached v14 patch has the changes for the same.
>
> Regard,s
> Vignesh

Thanks for working on the review comments. The changes in the new
patch look good to me.  I am marking it as ready to commit.

--
With Regards,
Ashutosh Sharma.

On Sun, Feb 13, 2022 at 7:34 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > I have spent little time looking at the latest patch. The patch looks
> > to be in good shape as it has already been reviewed by many people
> > here, although I did get some comments. Please take a look and let me
> > know your thoughts.
> >
> >
> > +   /* Try to connect to the publisher. */
> > +   wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > +   if (!wrconn)
> > +       ereport(ERROR,
> > +               (errmsg("could not connect to the publisher: %s", err)));
> >
> > I think it would be good to also include the errcode
> > (ERRCODE_CONNECTION_FAILURE) here?
>
> Modified
>
> > --
> >
> > @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> > CreateSubscriptionStmt *stmt,
> >
> >         PG_TRY();
> >         {
> > +           check_publications(wrconn, publications, opts.validate_publication);
> > +
> >
> >
> > Instead of passing the opts.validate_publication argument to
> > check_publication function, why can't we first check if this option is
> > set or not and accordingly call check_publication function? For other
> > options I see that it has been done in the similar way for e.g. check
> > for opts.connect or opts.refresh or opts.enabled etc.
>
> Modified
>
> > --
> >
> > Above comment also applies for:
> >
> > @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> > AlterSubscriptionStmt *stmt,
> >                 replaces[Anum_pg_subscription_subpublications - 1] = true;
> >
> >                 update_tuple = true;
> > +               connect_and_check_pubs(sub, stmt->publication,
> > +                                      opts.validate_publication);
> >
>
> Modified
>
> > --
> >
> > +         <para>
> > +          When true, the command verifies if all the specified publications
> > +          that are being subscribed to are present in the publisher and throws
> > +          an error if any of the publication doesn't exist. The default is
> > +          <literal>false</literal>.
> >
> > publication -> publications (in the 4th line : throw an error if any
> > of the publication doesn't exist)
> >
> > This applies for both CREATE and ALTER subscription commands.
>
> Modified
>
> Thanks for the comments, the attached v14 patch has the changes for the same.
>
> Regard,s
> Vignesh

On 2022-02-13 19:34:05 +0530, vignesh C wrote:
> Thanks for the comments, the attached v14 patch has the changes for the same.

The patch needs a rebase, it currently fails to apply:
http://cfbot.cputube.org/patch_37_2957.log

On Tue, Mar 22, 2022 at 5:29 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2022-02-13 19:34:05 +0530, vignesh C wrote:
> > Thanks for the comments, the attached v14 patch has the changes for the same.
>
> The patch needs a rebase, it currently fails to apply:
> http://cfbot.cputube.org/patch_37_2957.log

The attached v15 patch is rebased on top of HEAD.

Regards,
Vignesh

On Tue, Mar 22, 2022 at 3:23 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 5:29 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2022-02-13 19:34:05 +0530, vignesh C wrote:
> > > Thanks for the comments, the attached v14 patch has the changes for the same.
> >
> > The patch needs a rebase, it currently fails to apply:
> > http://cfbot.cputube.org/patch_37_2957.log

The patch was not applying on HEAD, attached patch which is rebased on
top of HEAD.

Regards,
Vignesh

On Sat, Mar 26, 2022 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
>
> The patch was not applying on HEAD, attached patch which is rebased on
> top of HEAD.
>

IIUC, this patch provides an option that allows us to give an error if
while creating/altering subcsiction, user gives non-existant
publications. I am not sure how useful it is to add such behavior via
an option especially when we know that it can occur in some other ways
like after creating the subscription, users can independently drop
publication from publisher. I think it could be useful to provide
additional information here but it would be better if we can follow
Euler's suggestion [1] in the thread where he suggested issuing a
WARNING if the publications don't exist and document that the
subscription catalog can have non-existent publications.

I think we should avoid adding new options unless they are really
required and useful.

[1] - https://www.postgresql.org/message-id/a2f2fba6-40dd-44cc-b40e-58196bb77f1c%40www.fastmail.com

-- 
With Regards,
Amit Kapila.

On Tue, Mar 29, 2022 at 11:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Mar 26, 2022 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > The patch was not applying on HEAD, attached patch which is rebased on
> > top of HEAD.
> >
>
> IIUC, this patch provides an option that allows us to give an error if
> while creating/altering subcsiction, user gives non-existant
> publications. I am not sure how useful it is to add such behavior via
> an option especially when we know that it can occur in some other ways
> like after creating the subscription, users can independently drop
> publication from publisher. I think it could be useful to provide
> additional information here but it would be better if we can follow
> Euler's suggestion [1] in the thread where he suggested issuing a
> WARNING if the publications don't exist and document that the
> subscription catalog can have non-existent publications.
>
> I think we should avoid adding new options unless they are really
> required and useful.
>

*
+connect_and_check_pubs(Subscription *sub, List *publications)
+{
+ char    *err;
+ WalReceiverConn *wrconn;
+
+ /* Load the library providing us libpq calls. */
+ load_file("libpqwalreceiver", false);
+
+ /* Try to connect to the publisher. */
+ wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
+ if (!wrconn)
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not connect to the publisher: %s", err));

I think it won't be a good idea to add new failure modes in existing
commands especially if we decide to make it non-optional. I think we
can do this check only in case we are already connecting to the
publisher.

-- 
With Regards,
Amit Kapila.

On Tue, Mar 29, 2022 at 11:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Mar 26, 2022 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > The patch was not applying on HEAD, attached patch which is rebased on
> > top of HEAD.
> >
>
> IIUC, this patch provides an option that allows us to give an error if
> while creating/altering subcsiction, user gives non-existant
> publications. I am not sure how useful it is to add such behavior via
> an option especially when we know that it can occur in some other ways
> like after creating the subscription, users can independently drop
> publication from publisher. I think it could be useful to provide
> additional information here but it would be better if we can follow
> Euler's suggestion [1] in the thread where he suggested issuing a
> WARNING if the publications don't exist and document that the
> subscription catalog can have non-existent publications.
>
> I think we should avoid adding new options unless they are really
> required and useful.
>
> [1] - https://www.postgresql.org/message-id/a2f2fba6-40dd-44cc-b40e-58196bb77f1c%40www.fastmail.com

Thanks for the suggestion, I have changed the patch as suggested.
Attached v16 patch has the changes for the same.

Regards,
Vignesh

On Tue, Mar 29, 2022 at 4:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 29, 2022 at 11:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Mar 26, 2022 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > The patch was not applying on HEAD, attached patch which is rebased on
> > > top of HEAD.
> > >
> >
> > IIUC, this patch provides an option that allows us to give an error if
> > while creating/altering subcsiction, user gives non-existant
> > publications. I am not sure how useful it is to add such behavior via
> > an option especially when we know that it can occur in some other ways
> > like after creating the subscription, users can independently drop
> > publication from publisher. I think it could be useful to provide
> > additional information here but it would be better if we can follow
> > Euler's suggestion [1] in the thread where he suggested issuing a
> > WARNING if the publications don't exist and document that the
> > subscription catalog can have non-existent publications.
> >
> > I think we should avoid adding new options unless they are really
> > required and useful.
> >
>
> *
> +connect_and_check_pubs(Subscription *sub, List *publications)
> +{
> + char    *err;
> + WalReceiverConn *wrconn;
> +
> + /* Load the library providing us libpq calls. */
> + load_file("libpqwalreceiver", false);
> +
> + /* Try to connect to the publisher. */
> + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> + if (!wrconn)
> + ereport(ERROR,
> + errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("could not connect to the publisher: %s", err));
>
> I think it won't be a good idea to add new failure modes in existing
> commands especially if we decide to make it non-optional. I think we
> can do this check only in case we are already connecting to the
> publisher.

I have modified it to check only in create subscription/alter
subscription .. add publication and alter subscription.. set
publication cases where we are connecting to the publisher.
The changes for the same are present at v16 patch attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm2zHd9FAn%2BMAQ3x-2%2BRnu8%3DRu%2BBeQXokfNBKo6sNAVb3A%40mail.gmail.com

Regards,
Vignesh

On Tue, Mar 29, 2022 at 8:11 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Mar 29, 2022 at 11:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> Thanks for the suggestion, I have changed the patch as suggested.
> Attached v16 patch has the changes for the same.
>

Thanks, I have one more comment.

postgres=# Alter subscription sub1 add publication pub4;
WARNING: publications "pub2", "pub4" do not exist in the publisher
ALTER SUBSCRIPTION

This gives additional publication in WARNING message which was not
part of current command but is present from the earlier time.

postgres=# Alter Subscription sub1 set publication pub5;
WARNING: publication "pub5" does not exist in the publisher
ALTER SUBSCRIPTION

SET variant doesn't give such a problem.

I feel we should be consistent here.

-- 
With Regards,
Amit Kapila.

On Wed, Mar 30, 2022 at 11:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 29, 2022 at 8:11 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, Mar 29, 2022 at 11:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > Thanks for the suggestion, I have changed the patch as suggested.
> > Attached v16 patch has the changes for the same.
> >
>
> Thanks, I have one more comment.
>
> postgres=# Alter subscription sub1 add publication pub4;
> WARNING: publications "pub2", "pub4" do not exist in the publisher
> ALTER SUBSCRIPTION
>
> This gives additional publication in WARNING message which was not
> part of current command but is present from the earlier time.
>
> postgres=# Alter Subscription sub1 set publication pub5;
> WARNING: publication "pub5" does not exist in the publisher
> ALTER SUBSCRIPTION
>
> SET variant doesn't give such a problem.
>
> I feel we should be consistent here.

I have made the changes for this, attached v17 patch has the changes
for the same.

Regards,
Vignesh

On Wed, Mar 30, 2022 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
>
> I have made the changes for this, attached v17 patch has the changes
> for the same.
>

The patch looks good to me. I have made minor edits in the comments
and docs. See the attached and let me know what you think? I intend to
commit this tomorrow unless there are more comments or suggestions.

-- 
With Regards,
Amit Kapila.

On Wed, Mar 30, 2022 at 4:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 30, 2022 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > I have made the changes for this, attached v17 patch has the changes
> > for the same.
> >
>
> The patch looks good to me. I have made minor edits in the comments
> and docs. See the attached and let me know what you think? I intend to
> commit this tomorrow unless there are more comments or suggestions.

I have one minor comment:

+ "Create subscription throws warning for multiple non-existent publications");
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION mysub1;");
+ "CREATE SUBSCRIPTION mysub1 CONNECTION '$publisher_connstr'
PUBLICATION mypub;"
+ "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub"
+ "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub"

Why should we drop the subscription mysub1 and create it for ALTER ..
ADD and ALTER .. SET tests? Can't we just do below which saves
unnecessary subscription creation, drop, wait_for_catchup and
poll_query_until?

+ "Create subscription throws warning for multiple non-existent publications");
+ "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub2"
+ "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub3"

Regards,
Bharath Rupireddy.

On Wed, Mar 30, 2022 at 5:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Mar 30, 2022 at 4:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 30, 2022 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > I have made the changes for this, attached v17 patch has the changes
> > > for the same.
> > >
> >
> > The patch looks good to me. I have made minor edits in the comments
> > and docs. See the attached and let me know what you think? I intend to
> > commit this tomorrow unless there are more comments or suggestions.
>
> I have one minor comment:
>
> + "Create subscription throws warning for multiple non-existent publications");
> +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION mysub1;");
> + "CREATE SUBSCRIPTION mysub1 CONNECTION '$publisher_connstr'
> PUBLICATION mypub;"
> + "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub"
> + "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub"
>
> Why should we drop the subscription mysub1 and create it for ALTER ..
> ADD and ALTER .. SET tests? Can't we just do below which saves
> unnecessary subscription creation, drop, wait_for_catchup and
> poll_query_until?
>
> + "Create subscription throws warning for multiple non-existent publications");
> + "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub2"
> + "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub3"

Or I would even simplify the entire tests as follows:

+ "CREATE SUBSCRIPTION mysub1 CONNECTION '$publisher_connstr'
PUBLICATION mypub, non_existent_pub1"
+ "Create subscription throws warning for non-existent publication");
>> no drop of mysub1 >>
+ "CREATE SUBSCRIPTION mysub2 CONNECTION '$publisher_connstr'
PUBLICATION non_existent_pub1, non_existent_pub2"
+ "Create subscription throws warning for multiple non-existent publications");
>> no drop of mysub2 >>
+ "ALTER SUBSCRIPTION mysub2 ADD PUBLICATION non_existent_pub3"
+ "Alter subscription add publication throws warning for non-existent
publication");
+ "ALTER SUBSCRIPTION mysub2 SET PUBLICATION non_existent_pub4"
+ "Alter subscription set publication throws warning for non-existent
publication");
 $node_subscriber->stop;
 $node_publisher->stop;

Regards,
Bharath Rupireddy.

On Wed, Mar 30, 2022 at 5:42 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Mar 30, 2022 at 5:37 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Wed, Mar 30, 2022 at 4:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Mar 30, 2022 at 12:22 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > I have made the changes for this, attached v17 patch has the changes
> > > > for the same.
> > > >
> > >
> > > The patch looks good to me. I have made minor edits in the comments
> > > and docs. See the attached and let me know what you think? I intend to
> > > commit this tomorrow unless there are more comments or suggestions.
> >
> > I have one minor comment:
> >
> > + "Create subscription throws warning for multiple non-existent publications");
> > +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION mysub1;");
> > + "CREATE SUBSCRIPTION mysub1 CONNECTION '$publisher_connstr'
> > PUBLICATION mypub;"
> > + "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub"
> > + "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub"
> >
> > Why should we drop the subscription mysub1 and create it for ALTER ..
> > ADD and ALTER .. SET tests? Can't we just do below which saves
> > unnecessary subscription creation, drop, wait_for_catchup and
> > poll_query_until?
> >
> > + "Create subscription throws warning for multiple non-existent publications");
> > + "ALTER SUBSCRIPTION mysub1 ADD PUBLICATION non_existent_pub2"
> > + "ALTER SUBSCRIPTION mysub1 SET PUBLICATION non_existent_pub3"
>
> Or I would even simplify the entire tests as follows:
>
> + "CREATE SUBSCRIPTION mysub1 CONNECTION '$publisher_connstr'
> PUBLICATION mypub, non_existent_pub1"
> + "Create subscription throws warning for non-existent publication");
> >> no drop of mysub1 >>
> + "CREATE SUBSCRIPTION mysub2 CONNECTION '$publisher_connstr'
> PUBLICATION non_existent_pub1, non_existent_pub2"
> + "Create subscription throws warning for multiple non-existent publications");
> >> no drop of mysub2 >>
> + "ALTER SUBSCRIPTION mysub2 ADD PUBLICATION non_existent_pub3"
> + "Alter subscription add publication throws warning for non-existent
> publication");
> + "ALTER SUBSCRIPTION mysub2 SET PUBLICATION non_existent_pub4"
> + "Alter subscription set publication throws warning for non-existent
> publication");
>  $node_subscriber->stop;
>  $node_publisher->stop;

Your suggestion looks valid, I have modified it as suggested.
Additionally I have removed Create subscription with multiple
non-existent publications and changed add publication with sing
non-existent publication to add publication with multiple non-existent
publications to cover the multiple non-existent publications path.
Attached v19 patch has the changes for the same.

Regards,
Vignesh

On Wed, Mar 30, 2022 at 9:54 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Mar 30, 2022 at 5:42 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Your suggestion looks valid, I have modified it as suggested.
> Additionally I have removed Create subscription with multiple
> non-existent publications and changed add publication with sing
> non-existent publication to add publication with multiple non-existent
> publications to cover the multiple non-existent publications path.
> Attached v19 patch has the changes for the same.
>

Pushed.

-- 
With Regards,
Amit Kapila.