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

On Mon, Jan 25, 2021 at 3:07 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Jan 25, 2021 at 2:48 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Mon, Jan 25, 2021 at 2:42 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
wrote:
> > >
> > > On Mon, Jan 25, 2021 at 1:10 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > >
> > > > On Thu, Jan 21, 2021 at 10:21 PM Bharath Rupireddy
> > > > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Thu, Jan 21, 2021 at 6:56 PM vignesh C <vignesh21(at)gmail(dot)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.
>

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 existpostgres=# 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