Re: AtEOXact_ApplyLauncher() and subtransactions

On Thu, 5 Jul 2018 at 3:37 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>
> On 4 July 2018 at 00:27, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Tue, Jun 26, 2018 at 6:25 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> >> Added this into the July 2018 commitfest :
> >>
> >> https://commitfest.postgresql.org/18/1696/
> >
> > It seems to me that it would probably be better to separate this into
> > two patches, because I think there are really two separate issues.
>
> Ok, will do that.
>
> > With regard to the lack of proper subtransaction handling, I think it
> > would be best if we could avoid introducing a new AtSubStart function.
> > I wrote a patch for this issue that works that uses a slightly
> > different kind of stack than yours, which I have attached to this
> > email, and it creates stack levels lazily so that it doesn't need an
> > AtSubStart function.
>
> Yes, I agree that if we can avoid this function, that would be good.
> Couldn't find a proper way to do this. Will have a look at your patch.

I have split it into two patches.

0001 patch contains the main fix. In this patch I have used some
naming conventions and some comments that you used in your patch,
plus, I used your method of lazily allocating new stack level. The
stack is initially Null.

The fix for the other issue is in 0002 patch. Having separate rel oids
list for each subids is essential only for this issue. So the changes
for using this structure are in this patch, not the 0001 one. As you
suggested, I have kept the subids in hash table as against linked
list.

> > As for the other part of your fix, which I think basically boils down
> > to comparing the final states instead of just looking at what got
> > changed, the logic looks complicated and I don't think I fully
> > understand it.
>
> Once I split the patch, let me try to add up some comments to make it clearer.

When a subscription is altered for the *first* time in a transaction,
an entry is created for that sub, in committed_subrels_table hash
table. That entry represents a cached list of tables belonging to that
subscription since the last committed change. For each ALTER
SUBSCRIPTION command, if we create the stop workers by comparing with
this cached list, we have the final list of stop-workers if committed
in this state. So if there are two ALTER commands for the same
subscription, the second one replaces the earlier stop-worker list by
its own list.

I have added some more comments in the below snippet as shown. Hope
that this helps :

@@ -594,7 +619,16 @@ AlterSubscription_refresh(Subscription *sub, bool
copy_data)
{
RemoveSubscriptionRel(sub->oid, relid);

- logicalrep_worker_stop_at_commit(sub->oid, relid);
+ /*
+ * If we found an entry in committed_subrels for this subid, that
+ * means subrelids represents a modified version of the
+ * committed_subrels_entry->relids. If we didn't find an entry, it
+ * means this is the first time we are altering the sub, so they
+ * both have the same committed list; so in that case, we avoid
+ * another iteration below, and create the stop workers here itself.
+ */
+ if (!sub_found)
+ stop_relids = lappend_oid(stop_relids, relid);

ereport(DEBUG1,
(errmsg("table \"%s.%s\" removed from subscription \"%s\"",