Identify missing publications from publisher while create/alter subscription.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Identify missing publications from publisher while create/alter subscription.

vignesh C
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

v1-0001-Identify-missing-publications-from-publisher-whil.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Japin Li

On Fri, 22 Jan 2021 at 00:51, Bharath Rupireddy <[hidden email]> wrote:

> On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Fri, Jan 22, 2021 at 10:14 AM japin <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

vignesh C
On Fri, Jan 22, 2021 at 12:14 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Fri, Jan 22, 2021 at 10:14 AM japin <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

vignesh C
In reply to this post by Japin Li
On Fri, Jan 22, 2021 at 10:14 AM japin <[hidden email]> wrote:

>
>
> On Fri, 22 Jan 2021 at 00:51, Bharath Rupireddy <[hidden email]> wrote:
> > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

vignesh C
In reply to this post by Bharath Rupireddy
On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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

v2-0001-Identify-missing-publications-from-publisher-whil.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Dilip Kumar-2
On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:

>
> On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> <[hidden email]> wrote:
> >
> > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <[hidden email]> wrote:

>
> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:
> >
> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > <[hidden email]> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Dilip Kumar-2
On Mon, Jan 25, 2021 at 2:48 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <[hidden email]> wrote:
> >
> > On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > > <[hidden email]> wrote:
> > > >
> > > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Dilip Kumar-2
On Mon, Jan 25, 2021 at 3:38 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Japin Li
In reply to this post by Bharath Rupireddy

On Mon, 25 Jan 2021 at 17:18, Bharath Rupireddy <[hidden email]> wrote:

> On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <[hidden email]> wrote:
>>
>> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:
>> >
>> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
>> > <[hidden email]> wrote:
>> > >
>> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Mon, Jan 25, 2021 at 5:18 PM japin <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

vignesh C
In reply to this post by Japin Li
On Mon, Jan 25, 2021 at 5:18 PM japin <[hidden email]> wrote:

>
>
> On Mon, 25 Jan 2021 at 17:18, Bharath Rupireddy <[hidden email]> wrote:
> > On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <[hidden email]> wrote:
> >>
> >> On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:
> >> >
> >> > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> >> > <[hidden email]> wrote:
> >> > >
> >> > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

vignesh C
In reply to this post by Dilip Kumar-2


On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <[hidden email]> wrote:

>
> On Mon, Jan 25, 2021 at 2:48 PM Bharath Rupireddy
> <[hidden email]> wrote:
> >
> > On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <[hidden email]> wrote:
> > >
> > > On Mon, Jan 25, 2021 at 1:10 PM vignesh C <[hidden email]> wrote:
> > > >
> > > > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > > > <[hidden email]> wrote:
> > > > >
> > > > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <[hidden email]> 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.
>
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 easier for 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=vignesh port=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 to step 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then we should throw an error similar to step 4 behavior. If publication is dropped after subscription is created, this should be removed when an alter subscription subname refresh publication is performed similar to step 4.
Thoughts?

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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Japin Li
In reply to this post by Bharath Rupireddy

On Mon, 25 Jan 2021 at 21:55, Bharath Rupireddy <[hidden email]> wrote:

> On Mon, Jan 25, 2021 at 5:18 PM japin <[hidden email]> 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@...

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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
In reply to this post by vignesh C
On Mon, Jan 25, 2021 at 10:32 PM vignesh C <[hidden email]> 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 easier for 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=vignesh port=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 to step 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then we should throw an error similar to step 4 behavior. If publication is dropped after subscription is created, this should be removed 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


Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Euler Taveira-3
On Wed, Feb 3, 2021, at 2:13 AM, Bharath Rupireddy wrote:
On Mon, Jan 25, 2021 at 10:32 PM vignesh C <[hidden email]> wrote:

> If a publication which does not exist is specified during create subscription, then we should throw an error similar to step 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then we should throw an error similar to step 4 behavior. If publication is dropped after subscription is created, this should be 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.

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.


--
Euler Taveira

Reply | Threaded
Open this post in threaded view
|

Re: Identify missing publications from publisher while create/alter subscription.

Bharath Rupireddy
On Wed, Mar 3, 2021 at 8:59 AM Euler Taveira <[hidden email]> wrote:

>
> On Wed, Feb 3, 2021, at 2:13 AM, Bharath Rupireddy wrote:
>
> On Mon, Jan 25, 2021 at 10:32 PM vignesh C <[hidden email]> wrote:
>
> > If a publication which does not exist is specified during create subscription, then we should throw an error similar to step 2 behavior. Similarly if a publication which does not exist is specified during alter subscription, then we should throw an error similar to step 4 behavior. If publication is dropped after subscription is created, this should be 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?

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


12