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."
Also, I don't see any test after you enable this special case. Is it
covered by existing tests, if not then let's try to add a test for
this?
--
With Regards,
Amit Kapila.
