On Sat, Feb 6, 2021 at 6:30 PM osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > > Hi > > > On Friday, February 5, 2021 5:51 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > On Fri, Feb 5, 2021 at 12:36 PM osumi.takami...@fujitsu.com > > <osumi.takami...@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. >
I checked this patch. It applied cleanly on top of V28, and all tests passed OK. Here are two feedback comments. 1. For the regression test there is 2 x SQL and 1 x function test. I thought to cover all the combinations there should be another function test. e.g. Tests ALTER … REFRESH Tests ALTER …. (refresh = true) Tests ALTER … (refresh = true) in a function Tests ALTER … REFRESH in a function <== this combination is not being testing ?? 2. For the 004 test case I know the test is needing some PK constraint violation # Check if DROP SUBSCRIPTION cleans up slots on the publisher side # when the subscriber is stuck on data copy for constraint But it is not clear to me what was the exact cause of that PK violation. I think you must be relying on data that is leftover from some previous test case but I am not sure which one. Can you make the comment more detailed to say *how* the PK violation is happening - e.g something to say which rows, in which table, and inserted by who? ------ Kind Regards, Peter Smith. Fujitsu Australia
