Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Sat, Mar 4, 2017 at 1:46 PM, Peter Eisentrautwrote: > On 3/3/17 13:58, Petr Jelinek wrote: >> On 23/02/17 08:24, Masahiko Sawada wrote: >>> Attached updated version patches. Please review these. >>> >> >> This version looks good to me, I'd only change the >> >>> +PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION >>> CREATE SLOT"); >> >> to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it >> for other commands (and same with DROP). > > I have committed fixes for these issues. Thanks! > > I didn't like the syntax change in DROP SUBSCRIPTION, so I have just > fixed the parsing of the existing syntax. We can discuss syntax changes > separately. Understood. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 3/3/17 13:58, Petr Jelinek wrote: > On 23/02/17 08:24, Masahiko Sawada wrote: >> Attached updated version patches. Please review these. >> > > This version looks good to me, I'd only change the > >> +PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE >> SLOT"); > > to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it > for other commands (and same with DROP). I have committed fixes for these issues. I didn't like the syntax change in DROP SUBSCRIPTION, so I have just fixed the parsing of the existing syntax. We can discuss syntax changes separately. The second patch I have committed after some editing. I think it was generated on top of the existing data copy patch, so it was a bit of a mess. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 23/02/17 08:24, Masahiko Sawada wrote: > Attached updated version patches. Please review these. > This version looks good to me, I'd only change the > + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE > SLOT"); to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it for other commands (and same with DROP). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Wed, Feb 22, 2017 at 2:17 AM, Masahiko Sawadawrote: > On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao wrote: >> On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada >> wrote: >>> On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao wrote: On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada wrote: > On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek > wrote: >> On 15/02/17 06:43, Masahiko Sawada wrote: >>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao >>> wrote: On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada wrote: > On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek > wrote: >> On 10/02/17 19:55, Masahiko Sawada wrote: >>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >>> wrote: On 08/02/17 07:40, Masahiko Sawada wrote: > On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier > wrote: >> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao >> wrote: >>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >>> wrote: For example what happens if apply crashes during the DROP SUBSCRIPTION/COMMIT and is not started because the delete from catalog is now visible so the subscription is no longer there? >>> >>> Another idea is to treat DROP SUBSCRIPTION in the same way as >>> VACUUM, i.e., >>> make it emit an error if it's executed within user's >>> transaction block. >> >> It seems to me that this is exactly Petr's point: using >> PreventTransactionChain() to prevent things to happen. > > Agreed. It's better to prevent to be executed inside user > transaction > block. And I understood there is too many failure scenarios we > need to > handle. > >>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() >>> just >>> after removing the entry from pg_subscription, then connect to >>> the publisher >>> and remove the replication slot. >> >> For consistency that may be important. > > Agreed. > > Attached patch, please give me feedback. > This looks good (and similar to what initial patch had btw). Works fine for me as well. Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are similar failure scenarios there, should we prevent it from running inside transaction as well? >>> >>> Hmm, after thought I suspect current discussing approach. For >>> example, please image the case where CRAETE SUBSCRIPTION creates >>> subscription successfully but fails to create replication slot for >>> whatever reason, and then DROP SUBSCRIPTION drops the subscription >>> but >>> dropping replication slot is failed. In such case, CREAET >>> SUBSCRIPTION >>> and DROP SUBSCRIPTION return ERROR but the subscription is created >>> and >>> dropped successfully. I think that this behaviour confuse the user. >>> >>> I think we should just prevent calling DROP SUBSCRIPTION in user's >>> transaction block. Or I guess that it could be better to separate >>> the >>> starting/stopping logical replication from subscription management. >>> >> >> We need to stop the replication worker(s) in order to be able to drop >> the slot. There is no such issue with startup of the worker as that >> one >> is launched by launcher after the transaction has committed. >> >> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION >> inside a >> transaction block and don't do any commits inside of those (so that >> there are no rollbacks, which solves your initial issue I believe). >> That >> way failure to create/drop slot will result in subscription not being >> created/dropped which is what we want. On second thought, +1. > I basically agree this option, but why do we need to change CREATE > SUBSCRIPTION as well? Because the window between the creation of replication slot and the transaction commit of CREATE SUBSCRIPTION
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masaowrote: > On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada > wrote: >> On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao wrote: >>> On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada >>> wrote: On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek wrote: > On 15/02/17 06:43, Masahiko Sawada wrote: >> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao >> wrote: >>> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada >>> wrote: On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek wrote: > On 10/02/17 19:55, Masahiko Sawada wrote: >> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >> wrote: >>> On 08/02/17 07:40, Masahiko Sawada wrote: On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier wrote: > On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao > wrote: >> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >> wrote: >>> For example what happens if apply crashes during the DROP >>> SUBSCRIPTION/COMMIT and is not started because the delete from >>> catalog >>> is now visible so the subscription is no longer there? >> >> Another idea is to treat DROP SUBSCRIPTION in the same way as >> VACUUM, i.e., >> make it emit an error if it's executed within user's transaction >> block. > > It seems to me that this is exactly Petr's point: using > PreventTransactionChain() to prevent things to happen. Agreed. It's better to prevent to be executed inside user transaction block. And I understood there is too many failure scenarios we need to handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() >> just >> after removing the entry from pg_subscription, then connect to >> the publisher >> and remove the replication slot. > > For consistency that may be important. Agreed. Attached patch, please give me feedback. >>> >>> This looks good (and similar to what initial patch had btw). Works >>> fine >>> for me as well. >>> >>> Remaining issue is, what to do about CREATE SUBSCRIPTION then, >>> there are >>> similar failure scenarios there, should we prevent it from running >>> inside transaction as well? >>> >> >> Hmm, after thought I suspect current discussing approach. For >> example, please image the case where CRAETE SUBSCRIPTION creates >> subscription successfully but fails to create replication slot for >> whatever reason, and then DROP SUBSCRIPTION drops the subscription >> but >> dropping replication slot is failed. In such case, CREAET >> SUBSCRIPTION >> and DROP SUBSCRIPTION return ERROR but the subscription is created >> and >> dropped successfully. I think that this behaviour confuse the user. >> >> I think we should just prevent calling DROP SUBSCRIPTION in user's >> transaction block. Or I guess that it could be better to separate the >> starting/stopping logical replication from subscription management. >> > > We need to stop the replication worker(s) in order to be able to drop > the slot. There is no such issue with startup of the worker as that > one > is launched by launcher after the transaction has committed. > > IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION > inside a > transaction block and don't do any commits inside of those (so that > there are no rollbacks, which solves your initial issue I believe). > That > way failure to create/drop slot will result in subscription not being > created/dropped which is what we want. >>> >>> On second thought, +1. >>> I basically agree this option, but why do we need to change CREATE SUBSCRIPTION as well? >>> >>> Because the window between the creation of replication slot and the >>> transaction >>> commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error >>> happens >>> during that window, the replication slot unexpectedly remains while >>> there is no >>> corresponding subscription. Of
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawadawrote: > On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao wrote: >> On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada >> wrote: >>> On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek >>> wrote: On 15/02/17 06:43, Masahiko Sawada wrote: > On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao > wrote: >> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada >> wrote: >>> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek >>> wrote: On 10/02/17 19:55, Masahiko Sawada wrote: > On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek > wrote: >> On 08/02/17 07:40, Masahiko Sawada wrote: >>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >>> wrote: On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: > On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek > wrote: >> For example what happens if apply crashes during the DROP >> SUBSCRIPTION/COMMIT and is not started because the delete from >> catalog >> is now visible so the subscription is no longer there? > > Another idea is to treat DROP SUBSCRIPTION in the same way as > VACUUM, i.e., > make it emit an error if it's executed within user's transaction > block. It seems to me that this is exactly Petr's point: using PreventTransactionChain() to prevent things to happen. >>> >>> Agreed. It's better to prevent to be executed inside user >>> transaction >>> block. And I understood there is too many failure scenarios we need >>> to >>> handle. >>> > Also DROP SUBSCRIPTION should call CommitTransactionCommand() just > after removing the entry from pg_subscription, then connect to > the publisher > and remove the replication slot. For consistency that may be important. >>> >>> Agreed. >>> >>> Attached patch, please give me feedback. >>> >> >> This looks good (and similar to what initial patch had btw). Works >> fine >> for me as well. >> >> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there >> are >> similar failure scenarios there, should we prevent it from running >> inside transaction as well? >> > > Hmm, after thought I suspect current discussing approach. For > example, please image the case where CRAETE SUBSCRIPTION creates > subscription successfully but fails to create replication slot for > whatever reason, and then DROP SUBSCRIPTION drops the subscription but > dropping replication slot is failed. In such case, CREAET SUBSCRIPTION > and DROP SUBSCRIPTION return ERROR but the subscription is created and > dropped successfully. I think that this behaviour confuse the user. > > I think we should just prevent calling DROP SUBSCRIPTION in user's > transaction block. Or I guess that it could be better to separate the > starting/stopping logical replication from subscription management. > We need to stop the replication worker(s) in order to be able to drop the slot. There is no such issue with startup of the worker as that one is launched by launcher after the transaction has committed. IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a transaction block and don't do any commits inside of those (so that there are no rollbacks, which solves your initial issue I believe). That way failure to create/drop slot will result in subscription not being created/dropped which is what we want. >> >> On second thought, +1. >> >>> I basically agree this option, but why do we need to change CREATE >>> SUBSCRIPTION as well? >> >> Because the window between the creation of replication slot and the >> transaction >> commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error >> happens >> during that window, the replication slot unexpectedly remains while >> there is no >> corresponding subscription. Of course, even If we prevent CREATE >> SUBSCRIPTION >> from being executed within user's transaction block, there is still such >> window. But we can reduce the possibility of that problem. > > Thank you for the explanation. I
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masaowrote: > On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada > wrote: >> On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek >> wrote: >>> On 15/02/17 06:43, Masahiko Sawada wrote: On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao wrote: > On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada > wrote: >> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek >> wrote: >>> On 10/02/17 19:55, Masahiko Sawada wrote: On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek wrote: > On 08/02/17 07:40, Masahiko Sawada wrote: >> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >> wrote: >>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao >>> wrote: On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek wrote: > For example what happens if apply crashes during the DROP > SUBSCRIPTION/COMMIT and is not started because the delete from > catalog > is now visible so the subscription is no longer there? Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., make it emit an error if it's executed within user's transaction block. >>> >>> It seems to me that this is exactly Petr's point: using >>> PreventTransactionChain() to prevent things to happen. >> >> Agreed. It's better to prevent to be executed inside user transaction >> block. And I understood there is too many failure scenarios we need >> to >> handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just after removing the entry from pg_subscription, then connect to the publisher and remove the replication slot. >>> >>> For consistency that may be important. >> >> Agreed. >> >> Attached patch, please give me feedback. >> > > This looks good (and similar to what initial patch had btw). Works > fine > for me as well. > > Remaining issue is, what to do about CREATE SUBSCRIPTION then, there > are > similar failure scenarios there, should we prevent it from running > inside transaction as well? > Hmm, after thought I suspect current discussing approach. For example, please image the case where CRAETE SUBSCRIPTION creates subscription successfully but fails to create replication slot for whatever reason, and then DROP SUBSCRIPTION drops the subscription but dropping replication slot is failed. In such case, CREAET SUBSCRIPTION and DROP SUBSCRIPTION return ERROR but the subscription is created and dropped successfully. I think that this behaviour confuse the user. I think we should just prevent calling DROP SUBSCRIPTION in user's transaction block. Or I guess that it could be better to separate the starting/stopping logical replication from subscription management. >>> >>> We need to stop the replication worker(s) in order to be able to drop >>> the slot. There is no such issue with startup of the worker as that one >>> is launched by launcher after the transaction has committed. >>> >>> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a >>> transaction block and don't do any commits inside of those (so that >>> there are no rollbacks, which solves your initial issue I believe). That >>> way failure to create/drop slot will result in subscription not being >>> created/dropped which is what we want. > > On second thought, +1. > >> I basically agree this option, but why do we need to change CREATE >> SUBSCRIPTION as well? > > Because the window between the creation of replication slot and the > transaction > commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error > happens > during that window, the replication slot unexpectedly remains while there > is no > corresponding subscription. Of course, even If we prevent CREATE > SUBSCRIPTION > from being executed within user's transaction block, there is still such > window. But we can reduce the possibility of that problem. Thank you for the explanation. I understood and agree. I think we should disallow to call ALTER SUBSCRIPTION inside a user's transaction block as well. >>> >>> Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever. >>> >>
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawadawrote: > On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek > wrote: >> On 15/02/17 06:43, Masahiko Sawada wrote: >>> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao wrote: On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada wrote: > On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek > wrote: >> On 10/02/17 19:55, Masahiko Sawada wrote: >>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >>> wrote: On 08/02/17 07:40, Masahiko Sawada wrote: > On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier > wrote: >> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao >> wrote: >>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >>> wrote: For example what happens if apply crashes during the DROP SUBSCRIPTION/COMMIT and is not started because the delete from catalog is now visible so the subscription is no longer there? >>> >>> Another idea is to treat DROP SUBSCRIPTION in the same way as >>> VACUUM, i.e., >>> make it emit an error if it's executed within user's transaction >>> block. >> >> It seems to me that this is exactly Petr's point: using >> PreventTransactionChain() to prevent things to happen. > > Agreed. It's better to prevent to be executed inside user transaction > block. And I understood there is too many failure scenarios we need to > handle. > >>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >>> after removing the entry from pg_subscription, then connect to the >>> publisher >>> and remove the replication slot. >> >> For consistency that may be important. > > Agreed. > > Attached patch, please give me feedback. > This looks good (and similar to what initial patch had btw). Works fine for me as well. Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are similar failure scenarios there, should we prevent it from running inside transaction as well? >>> >>> Hmm, after thought I suspect current discussing approach. For >>> example, please image the case where CRAETE SUBSCRIPTION creates >>> subscription successfully but fails to create replication slot for >>> whatever reason, and then DROP SUBSCRIPTION drops the subscription but >>> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION >>> and DROP SUBSCRIPTION return ERROR but the subscription is created and >>> dropped successfully. I think that this behaviour confuse the user. >>> >>> I think we should just prevent calling DROP SUBSCRIPTION in user's >>> transaction block. Or I guess that it could be better to separate the >>> starting/stopping logical replication from subscription management. >>> >> >> We need to stop the replication worker(s) in order to be able to drop >> the slot. There is no such issue with startup of the worker as that one >> is launched by launcher after the transaction has committed. >> >> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a >> transaction block and don't do any commits inside of those (so that >> there are no rollbacks, which solves your initial issue I believe). That >> way failure to create/drop slot will result in subscription not being >> created/dropped which is what we want. On second thought, +1. > I basically agree this option, but why do we need to change CREATE > SUBSCRIPTION as well? Because the window between the creation of replication slot and the transaction commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens during that window, the replication slot unexpectedly remains while there is no corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION from being executed within user's transaction block, there is still such window. But we can reduce the possibility of that problem. >>> >>> Thank you for the explanation. I understood and agree. >>> >>> I think we should disallow to call ALTER SUBSCRIPTION inside a user's >>> transaction block as well. >> >> Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever. >> > > Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached > fixed version patch. We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction block only when CREATE/DROP SLOT option
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinekwrote: > On 15/02/17 06:43, Masahiko Sawada wrote: >> On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao wrote: >>> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada >>> wrote: On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek wrote: > On 10/02/17 19:55, Masahiko Sawada wrote: >> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >> wrote: >>> On 08/02/17 07:40, Masahiko Sawada wrote: On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier wrote: > On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao > wrote: >> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >> wrote: >>> For example what happens if apply crashes during the DROP >>> SUBSCRIPTION/COMMIT and is not started because the delete from >>> catalog >>> is now visible so the subscription is no longer there? >> >> Another idea is to treat DROP SUBSCRIPTION in the same way as >> VACUUM, i.e., >> make it emit an error if it's executed within user's transaction >> block. > > It seems to me that this is exactly Petr's point: using > PreventTransactionChain() to prevent things to happen. Agreed. It's better to prevent to be executed inside user transaction block. And I understood there is too many failure scenarios we need to handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >> after removing the entry from pg_subscription, then connect to the >> publisher >> and remove the replication slot. > > For consistency that may be important. Agreed. Attached patch, please give me feedback. >>> >>> This looks good (and similar to what initial patch had btw). Works fine >>> for me as well. >>> >>> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are >>> similar failure scenarios there, should we prevent it from running >>> inside transaction as well? >>> >> >> Hmm, after thought I suspect current discussing approach. For >> example, please image the case where CRAETE SUBSCRIPTION creates >> subscription successfully but fails to create replication slot for >> whatever reason, and then DROP SUBSCRIPTION drops the subscription but >> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION >> and DROP SUBSCRIPTION return ERROR but the subscription is created and >> dropped successfully. I think that this behaviour confuse the user. >> >> I think we should just prevent calling DROP SUBSCRIPTION in user's >> transaction block. Or I guess that it could be better to separate the >> starting/stopping logical replication from subscription management. >> > > We need to stop the replication worker(s) in order to be able to drop > the slot. There is no such issue with startup of the worker as that one > is launched by launcher after the transaction has committed. > > IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a > transaction block and don't do any commits inside of those (so that > there are no rollbacks, which solves your initial issue I believe). That > way failure to create/drop slot will result in subscription not being > created/dropped which is what we want. >>> >>> On second thought, +1. >>> I basically agree this option, but why do we need to change CREATE SUBSCRIPTION as well? >>> >>> Because the window between the creation of replication slot and the >>> transaction >>> commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error >>> happens >>> during that window, the replication slot unexpectedly remains while there >>> is no >>> corresponding subscription. Of course, even If we prevent CREATE >>> SUBSCRIPTION >>> from being executed within user's transaction block, there is still such >>> window. But we can reduce the possibility of that problem. >> >> Thank you for the explanation. I understood and agree. >> >> I think we should disallow to call ALTER SUBSCRIPTION inside a user's >> transaction block as well. > > Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever. > Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached fixed version patch. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center disallow_sub_ddls_in_transaction_block_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 15/02/17 06:43, Masahiko Sawada wrote: > On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masaowrote: >> On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada >> wrote: >>> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek >>> wrote: On 10/02/17 19:55, Masahiko Sawada wrote: > On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek > wrote: >> On 08/02/17 07:40, Masahiko Sawada wrote: >>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >>> wrote: On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: > On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek > wrote: >> For example what happens if apply crashes during the DROP >> SUBSCRIPTION/COMMIT and is not started because the delete from >> catalog >> is now visible so the subscription is no longer there? > > Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, > i.e., > make it emit an error if it's executed within user's transaction > block. It seems to me that this is exactly Petr's point: using PreventTransactionChain() to prevent things to happen. >>> >>> Agreed. It's better to prevent to be executed inside user transaction >>> block. And I understood there is too many failure scenarios we need to >>> handle. >>> > Also DROP SUBSCRIPTION should call CommitTransactionCommand() just > after removing the entry from pg_subscription, then connect to the > publisher > and remove the replication slot. For consistency that may be important. >>> >>> Agreed. >>> >>> Attached patch, please give me feedback. >>> >> >> This looks good (and similar to what initial patch had btw). Works fine >> for me as well. >> >> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are >> similar failure scenarios there, should we prevent it from running >> inside transaction as well? >> > > Hmm, after thought I suspect current discussing approach. For > example, please image the case where CRAETE SUBSCRIPTION creates > subscription successfully but fails to create replication slot for > whatever reason, and then DROP SUBSCRIPTION drops the subscription but > dropping replication slot is failed. In such case, CREAET SUBSCRIPTION > and DROP SUBSCRIPTION return ERROR but the subscription is created and > dropped successfully. I think that this behaviour confuse the user. > > I think we should just prevent calling DROP SUBSCRIPTION in user's > transaction block. Or I guess that it could be better to separate the > starting/stopping logical replication from subscription management. > We need to stop the replication worker(s) in order to be able to drop the slot. There is no such issue with startup of the worker as that one is launched by launcher after the transaction has committed. IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a transaction block and don't do any commits inside of those (so that there are no rollbacks, which solves your initial issue I believe). That way failure to create/drop slot will result in subscription not being created/dropped which is what we want. >> >> On second thought, +1. >> >>> I basically agree this option, but why do we need to change CREATE >>> SUBSCRIPTION as well? >> >> Because the window between the creation of replication slot and the >> transaction >> commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error >> happens >> during that window, the replication slot unexpectedly remains while there is >> no >> corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION >> from being executed within user's transaction block, there is still such >> window. But we can reduce the possibility of that problem. > > Thank you for the explanation. I understood and agree. > > I think we should disallow to call ALTER SUBSCRIPTION inside a user's > transaction block as well. Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masaowrote: > On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada > wrote: >> On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek >> wrote: >>> On 10/02/17 19:55, Masahiko Sawada wrote: On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek wrote: > On 08/02/17 07:40, Masahiko Sawada wrote: >> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >> wrote: >>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao >>> wrote: On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek wrote: > For example what happens if apply crashes during the DROP > SUBSCRIPTION/COMMIT and is not started because the delete from catalog > is now visible so the subscription is no longer there? Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., make it emit an error if it's executed within user's transaction block. >>> >>> It seems to me that this is exactly Petr's point: using >>> PreventTransactionChain() to prevent things to happen. >> >> Agreed. It's better to prevent to be executed inside user transaction >> block. And I understood there is too many failure scenarios we need to >> handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just after removing the entry from pg_subscription, then connect to the publisher and remove the replication slot. >>> >>> For consistency that may be important. >> >> Agreed. >> >> Attached patch, please give me feedback. >> > > This looks good (and similar to what initial patch had btw). Works fine > for me as well. > > Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are > similar failure scenarios there, should we prevent it from running > inside transaction as well? > Hmm, after thought I suspect current discussing approach. For example, please image the case where CRAETE SUBSCRIPTION creates subscription successfully but fails to create replication slot for whatever reason, and then DROP SUBSCRIPTION drops the subscription but dropping replication slot is failed. In such case, CREAET SUBSCRIPTION and DROP SUBSCRIPTION return ERROR but the subscription is created and dropped successfully. I think that this behaviour confuse the user. I think we should just prevent calling DROP SUBSCRIPTION in user's transaction block. Or I guess that it could be better to separate the starting/stopping logical replication from subscription management. >>> >>> We need to stop the replication worker(s) in order to be able to drop >>> the slot. There is no such issue with startup of the worker as that one >>> is launched by launcher after the transaction has committed. >>> >>> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a >>> transaction block and don't do any commits inside of those (so that >>> there are no rollbacks, which solves your initial issue I believe). That >>> way failure to create/drop slot will result in subscription not being >>> created/dropped which is what we want. > > On second thought, +1. > >> I basically agree this option, but why do we need to change CREATE >> SUBSCRIPTION as well? > > Because the window between the creation of replication slot and the > transaction > commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens > during that window, the replication slot unexpectedly remains while there is > no > corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION > from being executed within user's transaction block, there is still such > window. But we can reduce the possibility of that problem. Thank you for the explanation. I understood and agree. I think we should disallow to call ALTER SUBSCRIPTION inside a user's transaction block as well. Attached patch changes these three DDLs so that they cannot be called inside a user's transaction block. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center disallow_sub_ddls_in_transaction_block.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawadawrote: > On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek > wrote: >> On 10/02/17 19:55, Masahiko Sawada wrote: >>> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >>> wrote: On 08/02/17 07:40, Masahiko Sawada wrote: > On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier > wrote: >> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao >> wrote: >>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >>> wrote: For example what happens if apply crashes during the DROP SUBSCRIPTION/COMMIT and is not started because the delete from catalog is now visible so the subscription is no longer there? >>> >>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, >>> i.e., >>> make it emit an error if it's executed within user's transaction block. >> >> It seems to me that this is exactly Petr's point: using >> PreventTransactionChain() to prevent things to happen. > > Agreed. It's better to prevent to be executed inside user transaction > block. And I understood there is too many failure scenarios we need to > handle. > >>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >>> after removing the entry from pg_subscription, then connect to the >>> publisher >>> and remove the replication slot. >> >> For consistency that may be important. > > Agreed. > > Attached patch, please give me feedback. > This looks good (and similar to what initial patch had btw). Works fine for me as well. Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are similar failure scenarios there, should we prevent it from running inside transaction as well? >>> >>> Hmm, after thought I suspect current discussing approach. For >>> example, please image the case where CRAETE SUBSCRIPTION creates >>> subscription successfully but fails to create replication slot for >>> whatever reason, and then DROP SUBSCRIPTION drops the subscription but >>> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION >>> and DROP SUBSCRIPTION return ERROR but the subscription is created and >>> dropped successfully. I think that this behaviour confuse the user. >>> >>> I think we should just prevent calling DROP SUBSCRIPTION in user's >>> transaction block. Or I guess that it could be better to separate the >>> starting/stopping logical replication from subscription management. >>> >> >> We need to stop the replication worker(s) in order to be able to drop >> the slot. There is no such issue with startup of the worker as that one >> is launched by launcher after the transaction has committed. >> >> IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a >> transaction block and don't do any commits inside of those (so that >> there are no rollbacks, which solves your initial issue I believe). That >> way failure to create/drop slot will result in subscription not being >> created/dropped which is what we want. On second thought, +1. > I basically agree this option, but why do we need to change CREATE > SUBSCRIPTION as well? Because the window between the creation of replication slot and the transaction commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens during that window, the replication slot unexpectedly remains while there is no corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION from being executed within user's transaction block, there is still such window. But we can reduce the possibility of that problem. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinekwrote: > On 10/02/17 19:55, Masahiko Sawada wrote: >> On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >> wrote: >>> On 08/02/17 07:40, Masahiko Sawada wrote: On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier wrote: > On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: >> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >> wrote: >>> For example what happens if apply crashes during the DROP >>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog >>> is now visible so the subscription is no longer there? >> >> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, >> i.e., >> make it emit an error if it's executed within user's transaction block. > > It seems to me that this is exactly Petr's point: using > PreventTransactionChain() to prevent things to happen. Agreed. It's better to prevent to be executed inside user transaction block. And I understood there is too many failure scenarios we need to handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >> after removing the entry from pg_subscription, then connect to the >> publisher >> and remove the replication slot. > > For consistency that may be important. Agreed. Attached patch, please give me feedback. >>> >>> This looks good (and similar to what initial patch had btw). Works fine >>> for me as well. >>> >>> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are >>> similar failure scenarios there, should we prevent it from running >>> inside transaction as well? >>> >> >> Hmm, after thought I suspect current discussing approach. For >> example, please image the case where CRAETE SUBSCRIPTION creates >> subscription successfully but fails to create replication slot for >> whatever reason, and then DROP SUBSCRIPTION drops the subscription but >> dropping replication slot is failed. In such case, CREAET SUBSCRIPTION >> and DROP SUBSCRIPTION return ERROR but the subscription is created and >> dropped successfully. I think that this behaviour confuse the user. >> >> I think we should just prevent calling DROP SUBSCRIPTION in user's >> transaction block. Or I guess that it could be better to separate the >> starting/stopping logical replication from subscription management. >> > > We need to stop the replication worker(s) in order to be able to drop > the slot. There is no such issue with startup of the worker as that one > is launched by launcher after the transaction has committed. > > IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a > transaction block and don't do any commits inside of those (so that > there are no rollbacks, which solves your initial issue I believe). That > way failure to create/drop slot will result in subscription not being > created/dropped which is what we want. > I basically agree this option, but why do we need to change CREATE SUBSCRIPTION as well? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 10/02/17 19:55, Masahiko Sawada wrote: > On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek >wrote: >> On 08/02/17 07:40, Masahiko Sawada wrote: >>> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >>> wrote: On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: > On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek > wrote: >> For example what happens if apply crashes during the DROP >> SUBSCRIPTION/COMMIT and is not started because the delete from catalog >> is now visible so the subscription is no longer there? > > Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, > i.e., > make it emit an error if it's executed within user's transaction block. It seems to me that this is exactly Petr's point: using PreventTransactionChain() to prevent things to happen. >>> >>> Agreed. It's better to prevent to be executed inside user transaction >>> block. And I understood there is too many failure scenarios we need to >>> handle. >>> > Also DROP SUBSCRIPTION should call CommitTransactionCommand() just > after removing the entry from pg_subscription, then connect to the > publisher > and remove the replication slot. For consistency that may be important. >>> >>> Agreed. >>> >>> Attached patch, please give me feedback. >>> >> >> This looks good (and similar to what initial patch had btw). Works fine >> for me as well. >> >> Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are >> similar failure scenarios there, should we prevent it from running >> inside transaction as well? >> > > Hmm, after thought I suspect current discussing approach. For > example, please image the case where CRAETE SUBSCRIPTION creates > subscription successfully but fails to create replication slot for > whatever reason, and then DROP SUBSCRIPTION drops the subscription but > dropping replication slot is failed. In such case, CREAET SUBSCRIPTION > and DROP SUBSCRIPTION return ERROR but the subscription is created and > dropped successfully. I think that this behaviour confuse the user. > > I think we should just prevent calling DROP SUBSCRIPTION in user's > transaction block. Or I guess that it could be better to separate the > starting/stopping logical replication from subscription management. > We need to stop the replication worker(s) in order to be able to drop the slot. There is no such issue with startup of the worker as that one is launched by launcher after the transaction has committed. IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a transaction block and don't do any commits inside of those (so that there are no rollbacks, which solves your initial issue I believe). That way failure to create/drop slot will result in subscription not being created/dropped which is what we want. Note that we do give users options to not create and not drop slots if they wish so we should really treat slot related failures as command errors. I don't want subscription setup to be 20 step where you have to deal with various things on multiple servers when it can be just plain simple one command on each side in many cases. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinekwrote: > On 08/02/17 07:40, Masahiko Sawada wrote: >> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >> wrote: >>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek wrote: > For example what happens if apply crashes during the DROP > SUBSCRIPTION/COMMIT and is not started because the delete from catalog > is now visible so the subscription is no longer there? Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., make it emit an error if it's executed within user's transaction block. >>> >>> It seems to me that this is exactly Petr's point: using >>> PreventTransactionChain() to prevent things to happen. >> >> Agreed. It's better to prevent to be executed inside user transaction >> block. And I understood there is too many failure scenarios we need to >> handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just after removing the entry from pg_subscription, then connect to the publisher and remove the replication slot. >>> >>> For consistency that may be important. >> >> Agreed. >> >> Attached patch, please give me feedback. >> > > This looks good (and similar to what initial patch had btw). Works fine > for me as well. > > Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are > similar failure scenarios there, should we prevent it from running > inside transaction as well? > Hmm, after thought I suspect current discussing approach. For example, please image the case where CRAETE SUBSCRIPTION creates subscription successfully but fails to create replication slot for whatever reason, and then DROP SUBSCRIPTION drops the subscription but dropping replication slot is failed. In such case, CREAET SUBSCRIPTION and DROP SUBSCRIPTION return ERROR but the subscription is created and dropped successfully. I think that this behaviour confuse the user. I think we should just prevent calling DROP SUBSCRIPTION in user's transaction block. Or I guess that it could be better to separate the starting/stopping logical replication from subscription management. Thought? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 08/02/17 07:40, Masahiko Sawada wrote: > On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier >wrote: >> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: >>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >>> wrote: For example what happens if apply crashes during the DROP SUBSCRIPTION/COMMIT and is not started because the delete from catalog is now visible so the subscription is no longer there? >>> >>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., >>> make it emit an error if it's executed within user's transaction block. >> >> It seems to me that this is exactly Petr's point: using >> PreventTransactionChain() to prevent things to happen. > > Agreed. It's better to prevent to be executed inside user transaction > block. And I understood there is too many failure scenarios we need to > handle. > >>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >>> after removing the entry from pg_subscription, then connect to the publisher >>> and remove the replication slot. >> >> For consistency that may be important. > > Agreed. > > Attached patch, please give me feedback. > This looks good (and similar to what initial patch had btw). Works fine for me as well. Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are similar failure scenarios there, should we prevent it from running inside transaction as well? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquierwrote: > On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao wrote: >> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek >> wrote: >>> For example what happens if apply crashes during the DROP >>> SUBSCRIPTION/COMMIT and is not started because the delete from catalog >>> is now visible so the subscription is no longer there? >> >> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., >> make it emit an error if it's executed within user's transaction block. > > It seems to me that this is exactly Petr's point: using > PreventTransactionChain() to prevent things to happen. Agreed. It's better to prevent to be executed inside user transaction block. And I understood there is too many failure scenarios we need to handle. >> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just >> after removing the entry from pg_subscription, then connect to the publisher >> and remove the replication slot. > > For consistency that may be important. Agreed. Attached patch, please give me feedback. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center drop_subscription_and_rollback_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masaowrote: > On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek > wrote: >> For example what happens if apply crashes during the DROP >> SUBSCRIPTION/COMMIT and is not started because the delete from catalog >> is now visible so the subscription is no longer there? > > Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., > make it emit an error if it's executed within user's transaction block. It seems to me that this is exactly Petr's point: using PreventTransactionChain() to prevent things to happen. > Also DROP SUBSCRIPTION should call CommitTransactionCommand() just > after removing the entry from pg_subscription, then connect to the publisher > and remove the replication slot. For consistency that may be important. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinekwrote: > On 07/02/17 13:10, Masahiko Sawada wrote: >> Hi all, >> >> While testing logical replciation I found that if the transaction >> issued DROP SUBSCRIPTION rollbacks then the logical repliation stops >> and the subscription can never be removed later. The document says >> that the replication worker associated with the subscription will not >> stop until after the transaction that issued this command has >> committed but it doesn't work. > > Ah then the docs are wrong and should be fixed. Maybe we should not > allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX > CONCURRENTLY. > >> The cause of this is that DropSubscription stops the apply worker and >> drops corresponding replication slot on publisher side without waiting >> for commit or rollback. The launcher process launches the apply worker >> again but the launched worker will fail to start logical replication >> because corresponding replication slot is already removed. And the >> orphan subscription can not be removed later. >> >> I think the logical replication should not stop and the corresponding >> replication slot and replication origin should not be removed until >> the transaction commits. >> >> The solution for this I came up with is that the launcher process >> stops the apply worker after DROP SUBSCRIPTION is committed rather >> than DropSubscription does. And the apply worker drops replication >> slot and replication origin before exits. Attached draft patch fixes >> this issue. >> > > I don't think we can allow the slot drop to be postponed. There is too > many failure scenarios where we would leave the remote slot in the > database and that's not acceptable IMHO. > > For example what happens if apply crashes during the DROP > SUBSCRIPTION/COMMIT and is not started because the delete from catalog > is now visible so the subscription is no longer there? Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e., make it emit an error if it's executed within user's transaction block. Also DROP SUBSCRIPTION should call CommitTransactionCommand() just after removing the entry from pg_subscription, then connect to the publisher and remove the replication slot. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 07/02/17 16:26, Petr Jelinek wrote: > On 07/02/17 13:10, Masahiko Sawada wrote: >> I think the logical replication should not stop and the corresponding >> replication slot and replication origin should not be removed until >> the transaction commits. >> >> The solution for this I came up with is that the launcher process >> stops the apply worker after DROP SUBSCRIPTION is committed rather >> than DropSubscription does. And the apply worker drops replication >> slot and replication origin before exits. Attached draft patch fixes >> this issue. >> > > I don't think we can allow the slot drop to be postponed. There is too > many failure scenarios where we would leave the remote slot in the > database and that's not acceptable IMHO. > > For example what happens if apply crashes during the DROP > SUBSCRIPTION/COMMIT and is not started because the delete from catalog > is now visible so the subscription is no longer there? > Not to mention that slot creation/drop is not transactional by itself so even if there was some way to tie remote transaction to local transaction (like say 2pc), it would still not work with ROLLBACK. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On 07/02/17 13:10, Masahiko Sawada wrote: > Hi all, > > While testing logical replciation I found that if the transaction > issued DROP SUBSCRIPTION rollbacks then the logical repliation stops > and the subscription can never be removed later. The document says > that the replication worker associated with the subscription will not > stop until after the transaction that issued this command has > committed but it doesn't work. Ah then the docs are wrong and should be fixed. Maybe we should not allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX CONCURRENTLY. > The cause of this is that DropSubscription stops the apply worker and > drops corresponding replication slot on publisher side without waiting > for commit or rollback. The launcher process launches the apply worker > again but the launched worker will fail to start logical replication > because corresponding replication slot is already removed. And the > orphan subscription can not be removed later. > > I think the logical replication should not stop and the corresponding > replication slot and replication origin should not be removed until > the transaction commits. > > The solution for this I came up with is that the launcher process > stops the apply worker after DROP SUBSCRIPTION is committed rather > than DropSubscription does. And the apply worker drops replication > slot and replication origin before exits. Attached draft patch fixes > this issue. > I don't think we can allow the slot drop to be postponed. There is too many failure scenarios where we would leave the remote slot in the database and that's not acceptable IMHO. For example what happens if apply crashes during the DROP SUBSCRIPTION/COMMIT and is not started because the delete from catalog is now visible so the subscription is no longer there? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK
On Tue, Feb 7, 2017 at 9:10 PM, Masahiko Sawadawrote: > Hi all, > > While testing logical replciation I found that if the transaction > issued DROP SUBSCRIPTION rollbacks then the logical repliation stops > and the subscription can never be removed later. The document says > that the replication worker associated with the subscription will not > stop until after the transaction that issued this command has > committed but it doesn't work. Yeah, this is a bug. ISTM that CREATE SUBSCRIPTION also has the similar issue. It creates the replication slot on the publisher side before the transaction has been committed. Even if the transaction is rollbacked, that replication slot is not removed. That is, in a transaction block, we should not connect to the publisher. Instead, the launcher or worker should do. > The cause of this is that DropSubscription stops the apply worker and > drops corresponding replication slot on publisher side without waiting > for commit or rollback. The launcher process launches the apply worker > again but the launched worker will fail to start logical replication > because corresponding replication slot is already removed. And the > orphan subscription can not be removed later. > > I think the logical replication should not stop and the corresponding > replication slot and replication origin should not be removed until > the transaction commits. Yes. > The solution for this I came up with is that the launcher process > stops the apply worker after DROP SUBSCRIPTION is committed rather > than DropSubscription does. And the apply worker drops replication > slot and replication origin before exits. Attached draft patch fixes > this issue. > > Please give me feedback. The patch failed to apply to HEAD. + worker = logicalrep_worker_find(subid); + if (worker) { - heap_close(rel, NoLock); - return; + if (stmt->drop_slot) + worker->drop_slot = true; + worker->need_to_stop = true; "drop_slot" and "need_to_stop" seem to be set to true even if the transaction is rollbacked. This would cause the same problem that you're trying to fix. I think that we should make the launcher periodically checks pg_subscription and stop the worker if there is no its corresponding subscription. Then, if necessary, the worker should remove its replication slot from the publisher. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] DROP SUBSCRIPTION and ROLLBACK
Hi all, While testing logical replciation I found that if the transaction issued DROP SUBSCRIPTION rollbacks then the logical repliation stops and the subscription can never be removed later. The document says that the replication worker associated with the subscription will not stop until after the transaction that issued this command has committed but it doesn't work. The cause of this is that DropSubscription stops the apply worker and drops corresponding replication slot on publisher side without waiting for commit or rollback. The launcher process launches the apply worker again but the launched worker will fail to start logical replication because corresponding replication slot is already removed. And the orphan subscription can not be removed later. I think the logical replication should not stop and the corresponding replication slot and replication origin should not be removed until the transaction commits. The solution for this I came up with is that the launcher process stops the apply worker after DROP SUBSCRIPTION is committed rather than DropSubscription does. And the apply worker drops replication slot and replication origin before exits. Attached draft patch fixes this issue. Please give me feedback. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center drop_subscription_and_rollback.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers