Hi
On Friday, February 5, 2021 5:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Feb 5, 2021 at 12:36 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > We need to add some tests to prove the new checks of AlterSubscription()
> work.
> > I chose TAP tests as we need to set connect = true for the subscription.
> > When it can contribute to the development, please utilize this.
> > I used v28 to check my patch and works as we expect.
> >
>
> Thanks for writing the tests but I don't understand why you need to set
> connect = true for this test? I have tried below '... with connect = false' and it
> seems to be working:
> postgres=# CREATE SUBSCRIPTION mysub
> postgres-# CONNECTION 'host=localhost port=5432
> dbname=postgres'
> postgres-# PUBLICATION mypublication WITH (connect = false);
> WARNING: tables were not subscribed, you will have to run ALTER
> SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables CREATE
> SUBSCRIPTION postgres=# Begin; BEGIN postgres=*# Alter Subscription
> mysub Refresh Publication;
> ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
> subscriptions
>
> So, if possible lets write this test in src/test/regress/sql/subscription.sql.
OK. I changed the place to write the tests for those.
> I have another idea for a test case: What if we write a test such that it fails PK
> violation on copy and then drop the subscription. Then check there shouldn't
> be any dangling slot on the publisher? This is similar to a test in
> subscription/t/004_sync.pl, we can use some of that framework but have a
> separate test for this.
I've added this PK violation test to the attached tests.
The patch works with v28 and made no failure during regression tests.
Best Regards,
Takamichi Osumi
