On Fri, May 28, 2021 at 4:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 8:05 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > Thanks for confirmation. The problem seemed to be as you reported a
> > table not closed when a transaction was committed.
> > This seems to be because the function UpdateTwoPhaseState was
> > committing a transaction inside the function when the caller of
> > UpdateTwoPhaseState had
> > a table open in CreateSubscription. This function was newly included
> > in the CreateSubscription code, to handle the new use case of
> > two_phase being enabled on
> > create subscription if "copy_data = false". I don't think
> > CreateSubscription required this to be inside a transaction and the
> > committing of transaction
> > was only meant for where this function was originally created to be
> > used in the apply worker code (ApplyWorkerMain()).
> > So, I removed the committing of the transaction from inside the
> > function UpdateTwoPhaseState() and instead started and committed the
> > transaction
> > prior to and after this function is invoked in the apply worker code.
> >
>
> You have made these changes in 0002 whereas they should be part of 0001.
>
> One minor comment for 0001.
> * Special case: if when tables were specified but copy_data is
> + * false then it is safe to enable two_phase up-front because
> + * those tables are already initially READY state. Note, if
> + * the subscription has no tables then enablement cannot be
> + * done here - we must leave the twophase state as PENDING, to
> + * allow ALTER SUBSCRIPTION ... REFRESH PUBLICATION to work.
>
> Can we slightly modify this comment as: "Note that if tables were
> specified but copy_data is false then it is safe to enable two_phase
> up-front because those tables are already initially READY state. When
> the subscription has no tables, we leave the twophase state as
> PENDING, to allow ALTER SUBSCRIPTION ... REFRESH PUBLICATION to work."
>
Created v81 - rebased to head and I have corrected the patch-set such
that the fix as well as Tang's test cases are now part of
patch-1. Also added this above minor comment update.
regards,
Ajin Cherian
Fujitsu Australia