Thread
Commits
-
Add subtransaction handling for table synchronization workers.
- 4beb25c63221 10.5 landed
- 9ec9f8f6833d 11.0 landed
- 32df1c9afa5a 12.0 landed
-
AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-06-05T11:02:59Z
Hi, When a SUBSCRIPTION is altered, then the currently running table-synchronization workers that are no longer needed for the altered subscription, are terminated. This is done by the function AtEOXact_ApplyLauncher() inside CommitTransaction(). So during each ALTER-SUBSCRIPTION command, the on_commit_stop_workers list is appended with new set of workers to be stopped. And then at commit, AtEOXact_ApplyLauncher() stops the workers in the list. But there is no handling for sub-transaction abort. Consider this : -- On secondary, create a subscription that will initiate the table sync CREATE SUBSCRIPTION mysub CONNECTION 'dbname=postgres host=localhost user=rep password=Password port=5432' PUBLICATION mypub; -- While the sync is going on, change the publication of the -- subscription in a subtransaction, so that it adds up the synchronization -- workers for the earlier publication into the 'on_commit_stop_workers' list select pg_sleep(1); begin; savepoint a; ALTER SUBSCRIPTION mysub SET PUBLICATION mypub_2; rollback to a; -- Commit will stop the above sync. commit; So the ALTER-SUBSCRIPTION has been rolled back. But the on_commit_stop_workers is not reverted back to its earlier state. And then at commit, the workers will be killed unnecessarily. I have observed that when the workers are killed, they are restarted. But consider the case where the original subscription was synchronizing hundreds of tables. And just when it is about to finish, someone changes the subscription inside a subtransaction and rolls it back, and then commits the transaction. The whole synchronization gets re-started from scratch. Below log snippets show this behaviour : < CREATE-SUBSCRIPTION command spawns workers > 2018-06-05 15:04:07.841 IST [39951] LOG: logical replication apply worker for subscription "mysub" has started 2018-06-05 15:04:07.848 IST [39953] LOG: logical replication table synchronization worker for subscription "mysub", table "article" has started < After some time, ALTER-SUBSCRIPTION rollbacks inside subtransaction, and commits transaction. Workers are killed > 2018-06-05 15:04:32.903 IST [39953] FATAL: terminating logical replication worker due to administrator command 2018-06-05 15:04:32.903 IST [39953] CONTEXT: COPY article, line 37116293 2018-06-05 15:04:32.904 IST [39666] LOG: background worker "logical replication worker" (PID 39953) exited with exit code 1 < Workers are restarted > 2018-06-05 15:04:32.909 IST [40003] LOG: logical replication table synchronization worker for subscription "mysub", table "article" has started < Synchronization done after some time > 2018-06-05 15:05:10.042 IST [40003] LOG: logical replication table synchronization worker for subscription "mysub", table "article" has finished ----------- To reproduce the issue : 1. On master server, create the tables and publications using attached setup_master.sql. 2. On secondary server, run the attached run_on_secondary.sql. This will reproduce the issue as can be seen from the log output. ----------- Fix : I haven't written a patch for it, but I think we should have a separate on_commit_stop_workers for each subtransaction. At subtransaction commit, we replace the on_commit_stop_workers list of the parent subtransaction with the one from the committed subtransaction; and on abort, discard the list of the current subtransaction. So have a stack of the lists. ----------- Furthermore, before fixing this, we may have to fix another issue which, I suspect, would surface even without subtransactions, as explained below : Suppose publication pubx is set for tables tx1 and and tx2. And publication puby is for tables ty1 and ty2. Subscription mysub is set to synchronise tables tx1 and tx2 : CREATE SUBSCRIPTION mysub ... PUBLICATION pubx; Now suppose the subscription is altered to synchronise ty1 and ty2, and then again altered back to synchronise tx1 and tx2 in the same transaction. begin; ALTER SUBSCRIPTION mysub set publication puby; ALTER SUBSCRIPTION mysub set publication pubx; commit; Here, workers for tx1 and tx2 are added to on_commit_stop_workers after the publication is set to puby. And then workers for ty1 and ty2 are further added to that list after the 2nd ALTER command where publication is set to pubx, because the earlier ALTER command has already changed the catalogs to denote that ty1 and ty2 are being synchronised. Effectively, at commit, all the workers are targetted to be stopped, when actually at commit time there is no final change in the tables to be synchronised. What actually we should do is : for each ALTER command we should compare the very first set of tables being synchronised since the last committed ALTER command, rather than checking the catalogs. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-14T16:28:54Z
On 2018-Jun-05, Amit Khandekar wrote: > When a SUBSCRIPTION is altered, then the currently running > table-synchronization workers that are no longer needed for the > altered subscription, are terminated. This is done by the function > AtEOXact_ApplyLauncher() inside CommitTransaction(). So during each > ALTER-SUBSCRIPTION command, the on_commit_stop_workers list is > appended with new set of workers to be stopped. And then at commit, > AtEOXact_ApplyLauncher() stops the workers in the list. > > But there is no handling for sub-transaction abort. Peter, any comments here? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-06-15T04:16:09Z
On 6/5/18 07:02, Amit Khandekar wrote: > I haven't written a patch for it, but I think we should have a > separate on_commit_stop_workers for eachyou get subtransaction. At > subtransaction commit, we replace the on_commit_stop_workers list of > the parent subtransaction with the one from the committed > subtransaction; and on abort, discard the list of the current > subtransaction. So have a stack of the lists. Is there maybe a more general mechanism we could attach this to? Maybe resource owners? > Subscription mysub is set to synchronise tables tx1 and tx2 : > CREATE SUBSCRIPTION mysub ... PUBLICATION pubx; > > Now suppose the subscription is altered to synchronise ty1 and ty2, > and then again altered back to synchronise tx1 and tx2 in the same > transaction. > begin; > ALTER SUBSCRIPTION mysub set publication puby; > ALTER SUBSCRIPTION mysub set publication pubx; > commit; > > Here, workers for tx1 and tx2 are added to on_commit_stop_workers > after the publication is set to puby. And then workers for ty1 and ty2 > are further added to that list after the 2nd ALTER command where > publication is set to pubx, because the earlier ALTER command has > already changed the catalogs to denote that ty1 and ty2 are being > synchronised. Effectively, at commit, all the workers are targetted to > be stopped, when actually at commit time there is no final change in > the tables to be synchronised. I'm not so bothered about this scenario. When you drop and then recreate a table in the same transaction, that doesn't mean you keep the data that was previously in the table. If you want to *undo* a change, you need to do rollback, not commit further changes that try to recreate the previous state. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-06-15T18:33:31Z
On 15 June 2018 at 09:46, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 6/5/18 07:02, Amit Khandekar wrote: >> I haven't written a patch for it, but I think we should have a >> separate on_commit_stop_workers for eachyou get subtransaction. At >> subtransaction commit, we replace the on_commit_stop_workers list of >> the parent subtransaction with the one from the committed >> subtransaction; and on abort, discard the list of the current >> subtransaction. So have a stack of the lists. > > Is there maybe a more general mechanism we could attach this to? Maybe > resource owners? You mean using something like ResourceOwnerRelease() ? We need to merge the on_commit_stop_workers list into the parent transaction's list. So we can't release the whole list on commit. The way I am implementing this can be seen in attached apply_launcher_subtrans_WIP.patch. (check launcher.c changes). I haven't started testing it yet though. on_commit_stop_workers denotes a list of subscriptions, and each element also contains a list of relations for that subscription. This list is built by ALTER-SUBSCRIPTION commands that have run in the current (sub)transaction. At sub-transaction start, the on_commit_stop_workers is pushed into a stack. Then on_commit_stop_workers starts with empty list. This list is then populated by ALTER-SUBCSRIPTION commands of the current sub-transaction. At sub-transaction commit, this list is merged into the list of parent subtransaction, the parent transaction stack element is popped out, and the merged list becomes the new on_commit_stop_workers. So say, parent has sub1(tab1, tab2), sub2(tab2, tab3), where sub means subscription. and current on_commit_workers has sub2(tab4) and sub3(tab1) At commit, for subscription sub2, we should replace the outer sub2(tab2, tab3) with the newer sub2(tab4). So, the merged list will have : sub1(tab1, tab2), sub2(tab4), sub3(tab1) At sub-transaction abort, the on_commit_stop_workers is discarded. The parent transaction worker list is assigned back to on_commit_stop_workers. > >> Subscription mysub is set to synchronise tables tx1 and tx2 : >> CREATE SUBSCRIPTION mysub ... PUBLICATION pubx; >> >> Now suppose the subscription is altered to synchronise ty1 and ty2, >> and then again altered back to synchronise tx1 and tx2 in the same >> transaction. >> begin; >> ALTER SUBSCRIPTION mysub set publication puby; >> ALTER SUBSCRIPTION mysub set publication pubx; >> commit; >> >> Here, workers for tx1 and tx2 are added to on_commit_stop_workers >> after the publication is set to puby. And then workers for ty1 and ty2 >> are further added to that list after the 2nd ALTER command where >> publication is set to pubx, because the earlier ALTER command has >> already changed the catalogs to denote that ty1 and ty2 are being >> synchronised. Effectively, at commit, all the workers are targetted to >> be stopped, when actually at commit time there is no final change in >> the tables to be synchronised. > > I'm not so bothered about this scenario. When you drop and then > recreate a table in the same transaction, that doesn't mean you keep the > data that was previously in the table. If you want to *undo* a change, > you need to do rollback, not commit further changes that try to recreate > the previous state. May be the example I gave is not practical. But a user can as well do something like : CREATE SUBSCRIPTION mysub ... PUBLICATION pub1; ... begin; ALTER SUBSCRIPTION mysub set publication pub2; .... ALTER SUBSCRIPTION mysub set publication pub3; commit; So pub3 is yet another publication. So here the old one is not set back again. Or may there can be ALTER SUBSCRIPTION REFRESH. I believe on_commit_stop_workers was designed keeping in mind that all actions are to be done only at commit; we should not immediately stop workers. So it implies that the workers it determines in the end of commit, also should be accurate. I have anyways included the changes for this in the same attached patch (changes are in subscriptioncmds.c) -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-06-18T09:32:56Z
On 16 June 2018 at 00:03, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > The way I am implementing this can be seen in attached > apply_launcher_subtrans_WIP.patch. (check launcher.c changes). I > haven't started testing it yet though. Attached patch passes some basic testing I did. Will do some more testing, and some self-review and code organising, if required. I will also split the patch into two : one containing the main issue regarding subtransaction, and the other containing the other issue I mentioned earlier that shows up without subtransaction as well. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-06-20T16:52:29Z
On 18 June 2018 at 15:02, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > On 16 June 2018 at 00:03, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> The way I am implementing this can be seen in attached >> apply_launcher_subtrans_WIP.patch. (check launcher.c changes). I >> haven't started testing it yet though. > > Attached patch passes some basic testing I did. Will do some more > testing, and some self-review and code organising, if required. Done. Attached is v2 version of the patch. Comments welcome. Changed GetSubscriptionRelations() to GetSubscriptionRelids(), which now returns only the oids, not the subrel states. This was convenient for storing the exact returned list into the committed subscription rels. And anyways the subrel states were not used anywhere. > I will also split the patch into two : one containing the main issue > regarding subtransaction, and the other containing the other issue I > mentioned earlier that shows up without subtransaction as well. Did not split the patch. The changes for the other issue that shows up without subtransaction are all in subscriptioncmds.c , and that file contains mostly the changes for this issue. So kept it as a single patch. But if it gets inconvenient for someone while reviewing, I will be happy to split it. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-06-26T10:25:33Z
Added this into the July 2018 commitfest : https://commitfest.postgresql.org/18/1696/ On 20 June 2018 at 22:22, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > On 18 June 2018 at 15:02, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> On 16 June 2018 at 00:03, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >>> The way I am implementing this can be seen in attached >>> apply_launcher_subtrans_WIP.patch. (check launcher.c changes). I >>> haven't started testing it yet though. >> >> Attached patch passes some basic testing I did. Will do some more >> testing, and some self-review and code organising, if required. > > Done. Attached is v2 version of the patch. Comments welcome. > > Changed GetSubscriptionRelations() to GetSubscriptionRelids(), which > now returns only the oids, not the subrel states. This was convenient > for storing the exact returned list into the committed subscription > rels. And anyways the subrel states were not used anywhere. > >> I will also split the patch into two : one containing the main issue >> regarding subtransaction, and the other containing the other issue I >> mentioned earlier that shows up without subtransaction as well. > > Did not split the patch. The changes for the other issue that shows up > without subtransaction are all in subscriptioncmds.c , and that file > contains mostly the changes for this issue. So kept it as a single > patch. But if it gets inconvenient for someone while reviewing, I will > be happy to split it. > > -- > Thanks, > -Amit Khandekar > EnterpriseDB Corporation > The Postgres Database Company -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Robert Haas <robertmhaas@gmail.com> — 2018-07-03T18:57:57Z
On Tue, Jun 26, 2018 at 6:25 AM, Amit Khandekar <amitdkhan.pg@gmail.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. 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. It would probably also be possible to adapt your patch to create new stack levels on demand instead of at subtransaction start. I'm not sure which approach is better, but I do think it would be best not to use your patch as you have it now, because that does unnecessary work at the beginning and end of every subtransaction if there is an ALTER SUBSCRIPTION command pending at an outer level, even though the subtransaction may never touch the subscription state. 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, but here are a few comments. + subrelids = GetSubscriptionRelids(sub->oid, + committed_subrels ? + CurrentMemoryContext : TopTransactionContext); This looks ugly and dangerous. In the first place, if GetSubscriptionRelids() needs to work in one of several memory contexts, the best thing would probably be for the caller to be responsible for saving and restoring the memory context as needed, rather than passing it as an argument. Secondly, it's not very clear why we need to do this. The comment says we have to do it, but it doesn't give a reason. + * Merge the current list into the immediate parent. + * So say, parent has sub1(tab1, tab2), sub2(tab2, tab3), + * and current on_commit_workers has sub2(tab4) and sub3(tab1), + * then the merged list will have : + * sub1(tab1, tab2), sub2(tab4), sub3(tab1) I don't think this is very clear. Also, why is that the correct answer? Why not sub2(tab2, tab3, tab4)? + foreach(lc, on_commit_stop_workers) + { + SubscriptionRels *subrels = lfirst(lc); + ListCell *lc1; + + /* Search this subrel in the subrels of the top of stack. */ + foreach(lc1, subtrans_stop_workers->stop_workers) This might be very expensive if both lists are long. I guess that's probably not very likely. You would need to have modify a lot of subscriptions and then, within a subtransaction, modify a lot of subscriptions again. + foreach(lc, committed_subrels_list) + { + SubscriptionRels *subrels = (SubscriptionRels *) lfirst(lc); + + if (sub->oid == subrels->subid) + { + committed_subrels = subrels; + break; + } + } This looks to be O(n^2) in the number of subscriptions modified in a single transaction. Like Peter, I'm not entirely sure how much we should worry about this problem. However, if we're going to worry about it, I wonder if we should worry harder and try to come up with a better data structure than a bunch of linked lists. Maybe a hash table indexed by subid? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-07-05T10:07:31Z
On 4 July 2018 at 00:27, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jun 26, 2018 at 6:25 AM, Amit Khandekar <amitdkhan.pg@gmail.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. > 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. > but here are a few comments. > > + subrelids = GetSubscriptionRelids(sub->oid, > + committed_subrels ? > + CurrentMemoryContext : > TopTransactionContext); > > This looks ugly and dangerous. In the first place, if > GetSubscriptionRelids() needs to work in one of several memory > contexts, the best thing would probably be for the caller to be > responsible for saving and restoring the memory context as needed, > rather than passing it as an argument. I wanted to make the context switch only around the code which allocates the list. I understand that all the other code in GetSubscriptionRelids() such as heap_open() , systable_beginscan() etc would not be affected if we run that in TopTransactionContext, because that all gets freed in the cleanup functions at the bottom of the function, but still I thought this way would be more robust for future, in case there is some new code that allocates some temporary memory which is not freed. > Secondly, it's not very clear > why we need to do this. The comment says we have to do it, but it > doesn't give a reason. When committed_subrels is NULL, it means this is the first time we are running the ALTER command since the main transaction started, and we want to store the committed subrels list in the transaction context, so that the subsequent ALTER commands compare with this list. May be when I split the patch, I will come up with an example. > > + * Merge the current list into the immediate parent. > + * So say, parent has sub1(tab1, tab2), > sub2(tab2, tab3), > + * and current on_commit_workers has > sub2(tab4) and sub3(tab1), > + * then the merged list will have : > + * sub1(tab1, tab2), sub2(tab4), sub3(tab1) > > I don't think this is very clear. Also, why is that the correct > answer? Why not sub2(tab2, tab3, tab4)? Parent sub-transaction has sub2(tab2, tab3), meaning that when the subtransaction sub2 commits (and when the COMMIT happens), we should stop the worker for (subscription sub2, table tab2) , and the worker for (subscription sub2, table tab3). And the current sub-transaction has modified the subscription sub2 such that the workers to be stopped are sub2(tab4). Note that this the final list of workers to be stopped, regardless of the earlier ALTER SUBSCRIPTION commands having run in the upper subtransaction. So this list should completely replace the parent's sub2(tab2, tab3). We always compare the subscription tables with the ones that are committed, not with the ones that were last altered in the same transaction. Hence the other patch. > > + foreach(lc, on_commit_stop_workers) > + { > + SubscriptionRels *subrels = lfirst(lc); > + ListCell *lc1; > + > + /* Search this subrel in the subrels > of the top of stack. */ > + foreach(lc1, > subtrans_stop_workers->stop_workers) > > This might be very expensive if both lists are long. I guess that's > probably not very likely. You would need to have modify a lot of > subscriptions and then, within a subtransaction, modify a lot of > subscriptions again. Yes that's what I also think. > > + foreach(lc, committed_subrels_list) > + { > + SubscriptionRels *subrels = (SubscriptionRels *) lfirst(lc); > + > + if (sub->oid == subrels->subid) > + { > + committed_subrels = subrels; > + break; > + } > + } > > This looks to be O(n^2) in the number of subscriptions modified in a > single transaction. > > Like Peter, I'm not entirely sure how much we should worry about this > problem. At the upper sections, I have explained it a bit. Basically, without this second patch, the main patch won't work if we have the same subscription altered more than once in the sub-transaction stack. > However, if we're going to worry about it, I wonder if we > should worry harder and try to come up with a better data structure > than a bunch of linked lists. Maybe a hash table indexed by subid? I had thought about using hash to search a subscription id, but then thought in practice, there would not be too many subscriptions in the same transaction. Hence continued to use the existing linked list data structure; just that instead of a single linked list having everything, I thought let's keep a list of tables belonging to the same subscription in one linked list. The reason for this was that it is easy to abandon or merge a subscription table list when a sub-transaction rolls back or commits. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-07-16T06:36:24Z
On Thu, 5 Jul 2018 at 3:37 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > > On 4 July 2018 at 00:27, Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Jun 26, 2018 at 6:25 AM, Amit Khandekar <amitdkhan.pg@gmail.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\"", -
Re: AtEOXact_ApplyLauncher() and subtransactions
Robert Haas <robertmhaas@gmail.com> — 2018-07-16T21:59:43Z
On Mon, Jul 16, 2018 at 2:36 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > 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. Committed and back-patched to v11 and v10. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: AtEOXact_ApplyLauncher() and subtransactions
Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-07-18T04:07:27Z
On 17 July 2018 at 03:29, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jul 16, 2018 at 2:36 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> 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. > > Committed and back-patched to v11 and v10. Thanks Robert.