Re: Single transaction in the tablesync worker?

On Sat, Feb 6, 2021 at 6:30 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> Hi
>
>
> On Friday, February 5, 2021 5:51 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Fri, Feb 5, 2021 at 12:36 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)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