Identify missing publications from publisher while create/alter subscription.

>
> 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.