Thread: [HACKERS] logical replication apply to run with sync commit off by default
Hi, there has been discussion at the logical replication initial copy thread [1] about making apply work with sync commit off by default for performance reasons and adding option to change that per subscription. Here I propose patch to implement this - it adds boolean column subssynccommit to pg_subscription catalog which decides if synchronous_commit should be off or local for apply. And it adds SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and ALTER SUBSCRIPTION. When nothing is specified it will set it to false. The patch is built on top of copy patch currently as there are conflicts between the two and this helps a bit with testing of copy patch. [1] https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com -- 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
Attachment
Re: [HACKERS] logical replication apply to run with sync commit off by default
From
Petr Jelinek
Date:
On 07/03/17 06:23, Petr Jelinek wrote: > Hi, > > there has been discussion at the logical replication initial copy thread > [1] about making apply work with sync commit off by default for > performance reasons and adding option to change that per subscription. > > Here I propose patch to implement this - it adds boolean column > subssynccommit to pg_subscription catalog which decides if > synchronous_commit should be off or local for apply. And it adds > SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and > ALTER SUBSCRIPTION. When nothing is specified it will set it to false. > > The patch is built on top of copy patch currently as there are conflicts > between the two and this helps a bit with testing of copy patch. > > [1] > https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com > I rebased this patch against recent changes and the latest version of copy patch. -- 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
Attachment
Re: [HACKERS] logical replication apply to run with sync commit off by default
From
Petr Jelinek
Date:
On 18/03/17 13:31, Petr Jelinek wrote: > On 07/03/17 06:23, Petr Jelinek wrote: >> Hi, >> >> there has been discussion at the logical replication initial copy thread >> [1] about making apply work with sync commit off by default for >> performance reasons and adding option to change that per subscription. >> >> Here I propose patch to implement this - it adds boolean column >> subssynccommit to pg_subscription catalog which decides if >> synchronous_commit should be off or local for apply. And it adds >> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. >> >> The patch is built on top of copy patch currently as there are conflicts >> between the two and this helps a bit with testing of copy patch. >> >> [1] >> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com >> > > I rebased this patch against recent changes and the latest version of > copy patch. > And another rebase after pg_dump tests commit. -- 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
Attachment
Re: [HACKERS] logical replication apply to run with sync commit off by default
From
Robert Haas
Date:
On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > On 18/03/17 13:31, Petr Jelinek wrote: >> On 07/03/17 06:23, Petr Jelinek wrote: >>> there has been discussion at the logical replication initial copy thread >>> [1] about making apply work with sync commit off by default for >>> performance reasons and adding option to change that per subscription. >>> >>> Here I propose patch to implement this - it adds boolean column >>> subssynccommit to pg_subscription catalog which decides if >>> synchronous_commit should be off or local for apply. And it adds >>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. >>> >>> The patch is built on top of copy patch currently as there are conflicts >>> between the two and this helps a bit with testing of copy patch. >>> >>> [1] >>> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com >>> >> >> I rebased this patch against recent changes and the latest version of >> copy patch. > > And another rebase after pg_dump tests commit. + else if (strcmp(defel->defname, "nosynchronous_commit") == 0 && synchronous_commit) + { + if (synchronous_commit_given) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("conflicting or redundant options"))); + + synchronous_commit_given = true; + *synchronous_commit = !defGetBoolean(defel); + } Uh, what's this nosynchronous_commit thing? + <literal>local</literal> otherwise to <literal>false</literal>. The + default value is <literal>false</literal> independently of the default + <literal>synchronous_commit</literal> setting for the instance. This phrasing isn't very clear or accurate, IMHO. I'd say something like "The value of this parameter overrides the synchronous_commit setting. The default value is false." And I'd make the word synchronous_commit in that sentence a link to the GUC, so that it's absolutely unmistakable what we mean by "the synchronous_commit setting". /* + * We need to make new connection to new slot if slot name has changed + * so exit here as well if that's the case. + */ + if (strcmp(newsub->slotname, MySubscription->slotname) != 0) + { + ereport(LOG, + (errmsg("logical replication worker for subscription \"%s\" will " + "restart because the replication slot name was changed", + MySubscription->name))); + + walrcv_disconnect(wrconn); + proc_exit(0); + } Looks unrelated. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] logical replication apply to run with sync commit off by default
From
Petr Jelinek
Date:
On 21/03/17 18:54, Robert Haas wrote: > On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek > <petr.jelinek@2ndquadrant.com> wrote: >> On 18/03/17 13:31, Petr Jelinek wrote: >>> On 07/03/17 06:23, Petr Jelinek wrote: >>>> there has been discussion at the logical replication initial copy thread >>>> [1] about making apply work with sync commit off by default for >>>> performance reasons and adding option to change that per subscription. >>>> >>>> Here I propose patch to implement this - it adds boolean column >>>> subssynccommit to pg_subscription catalog which decides if >>>> synchronous_commit should be off or local for apply. And it adds >>>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >>>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. >>>> >>>> The patch is built on top of copy patch currently as there are conflicts >>>> between the two and this helps a bit with testing of copy patch. >>>> >>>> [1] >>>> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com >>>> >>> >>> I rebased this patch against recent changes and the latest version of >>> copy patch. >> >> And another rebase after pg_dump tests commit. > > + else if (strcmp(defel->defname, "nosynchronous_commit") == 0 > && synchronous_commit) > + { > + if (synchronous_commit_given) > + ereport(ERROR, > + (errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("conflicting or redundant options"))); > + > + synchronous_commit_given = true; > + *synchronous_commit = !defGetBoolean(defel); > + } > > Uh, what's this nosynchronous_commit thing? Ah originally I didn't have it as bool just as (no)synchronous_commit, forgot to rip this out. > > + <literal>local</literal> otherwise to <literal>false</literal>. The > + default value is <literal>false</literal> independently of the default > + <literal>synchronous_commit</literal> setting for the instance. > > This phrasing isn't very clear or accurate, IMHO. I'd say something > like "The value of this parameter overrides the synchronous_commit > setting. The default value is false." And I'd make the word > synchronous_commit in that sentence a link to the GUC, so that it's > absolutely unmistakable what we mean by "the synchronous_commit > setting". Okay. > > /* > + * We need to make new connection to new slot if slot name has changed > + * so exit here as well if that's the case. > + */ > + if (strcmp(newsub->slotname, MySubscription->slotname) != 0) > + { > + ereport(LOG, > + (errmsg("logical replication worker for subscription > \"%s\" will " > + "restart because the replication slot name > was changed", > + MySubscription->name))); > + > + walrcv_disconnect(wrconn); > + proc_exit(0); > + } > > Looks unrelated. > Oops, need to fix this separately. -- 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
Attachment
On 21/03/17 22:37, Petr Jelinek wrote: > On 21/03/17 18:54, Robert Haas wrote: >> On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek >> <petr.jelinek@2ndquadrant.com> wrote: >>> On 18/03/17 13:31, Petr Jelinek wrote: >>>> On 07/03/17 06:23, Petr Jelinek wrote: >>>>> there has been discussion at the logical replication initial copy thread >>>>> [1] about making apply work with sync commit off by default for >>>>> performance reasons and adding option to change that per subscription. >>>>> >>>>> Here I propose patch to implement this - it adds boolean column >>>>> subssynccommit to pg_subscription catalog which decides if >>>>> synchronous_commit should be off or local for apply. And it adds >>>>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >>>>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. >>>>> >>>>> The patch is built on top of copy patch currently as there are conflicts >>>>> between the two and this helps a bit with testing of copy patch. >>>>> >>>>> [1] >>>>> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com >>>>> >>>> >>>> I rebased this patch against recent changes and the latest version of >>>> copy patch. >>> Rebase after table copy patch got committed. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Mar 24, 2017 at 11:49 PM, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > On 21/03/17 22:37, Petr Jelinek wrote: >> On 21/03/17 18:54, Robert Haas wrote: >>> On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek >>> <petr.jelinek@2ndquadrant.com> wrote: >>>> On 18/03/17 13:31, Petr Jelinek wrote: >>>>> On 07/03/17 06:23, Petr Jelinek wrote: >>>>>> there has been discussion at the logical replication initial copy thread >>>>>> [1] about making apply work with sync commit off by default for >>>>>> performance reasons and adding option to change that per subscription. >>>>>> >>>>>> Here I propose patch to implement this - it adds boolean column >>>>>> subssynccommit to pg_subscription catalog which decides if >>>>>> synchronous_commit should be off or local for apply. And it adds >>>>>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >>>>>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. >>>>>> >>>>>> The patch is built on top of copy patch currently as there are conflicts >>>>>> between the two and this helps a bit with testing of copy patch. >>>>>> >>>>>> [1] >>>>>> https://www.postgresql.org/message-id/CA+TgmoY7Lk2YKArcp4O=Qu=xOoR8j71MAd1OTEojAWmujE3k0w@mail.gmail.com >>>>>> >>>>> >>>>> I rebased this patch against recent changes and the latest version of >>>>> copy patch. >>>> > > Rebase after table copy patch got committed. > This patch seems to conflict current HEAD. Please update the patch. $ patch -p1 < 0001-Add-option-to-modify-sync-commit-per-subscription.patch (Stripping trailing CRs from patch.) patching file doc/src/sgml/catalogs.sgml Hunk #1 succeeded at 6511 (offset 97 lines). (Stripping trailing CRs from patch.) patching file doc/src/sgml/ref/alter_subscription.sgml (Stripping trailing CRs from patch.) patching file doc/src/sgml/ref/create_subscription.sgml (Stripping trailing CRs from patch.) patching file src/backend/catalog/pg_subscription.c (Stripping trailing CRs from patch.) patching file src/backend/commands/subscriptioncmds.c (Stripping trailing CRs from patch.) patching file src/backend/replication/logical/launcher.c (Stripping trailing CRs from patch.) patching file src/backend/replication/logical/worker.c Hunk #1 succeeded at 1395 (offset -15 lines). Hunk #2 succeeded at 1468 (offset -15 lines). (Stripping trailing CRs from patch.) patching file src/bin/pg_dump/pg_dump.c Hunk #1 succeeded at 3654 (offset 1 line). Hunk #2 succeeded at 3672 (offset 1 line). Hunk #3 succeeded at 3687 (offset 1 line). Hunk #4 succeeded at 3705 (offset 1 line). Hunk #5 succeeded at 3776 (offset 1 line). (Stripping trailing CRs from patch.) patching file src/bin/pg_dump/pg_dump.h Hunk #1 succeeded at 612 (offset 8 lines). (Stripping trailing CRs from patch.) patching file src/bin/pg_dump/t/002_pg_dump.pl (Stripping trailing CRs from patch.) patching file src/bin/psql/describe.c Hunk #1 succeeded at 5171 (offset 51 lines). Hunk #2 succeeded at 5198 (offset 51 lines). (Stripping trailing CRs from patch.) patching file src/include/catalog/pg_subscription.h (Stripping trailing CRs from patch.) patching file src/test/regress/expected/subscription.out Hunk #1 FAILED at 25. Hunk #2 succeeded at 89 (offset 25 lines). 1 out of 2 hunks FAILED -- saving rejects to file src/test/regress/expected/subscription.out.rej (Stripping trailing CRs from patch.) patching file src/test/regress/sql/subscription.sql Hunk #1 succeeded at 66 (offset 21 lines). ----- + <para> + The value of this parameter overrides the + <xref linkend="guc-synchronous-commit">synchronous_commit setting. + The default value is <literal>false<literal>. + </para> + <entry> + If true, the apply for the subscription will run with + <literal>synchronous_commit</literal> set to <literal>local</literal>. + Otherwise it will have it set to <literal>false</literal>. + </entry> synchronous_commit can work with false actually but I think that it's better to use "off" instead of "false" according to the document. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Peter Eisentraut
Date:
On 3/24/17 10:49, Petr Jelinek wrote: >>>>> On 07/03/17 06:23, Petr Jelinek wrote: >>>>>> there has been discussion at the logical replication initial copy thread >>>>>> [1] about making apply work with sync commit off by default for >>>>>> performance reasons and adding option to change that per subscription. >>>>>> >>>>>> Here I propose patch to implement this - it adds boolean column >>>>>> subssynccommit to pg_subscription catalog which decides if >>>>>> synchronous_commit should be off or local for apply. And it adds >>>>>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and >>>>>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. > Rebase after table copy patch got committed. I think this change could use some more documentation. Under what circumstances would a user want to turn this back on? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Craig Ringer
Date:
On 7 April 2017 at 01:38, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > I think this change could use some more documentation. Under what > circumstances would a user want to turn this back on? The main reason is if you want to use synchronous_standby_names and synchronous commit on the upstream. Turning sync appy back on for logical replicas will cause them to send more timely standby status updates, so commits on the upstream can return faster. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] logical replication apply to run with sync commit off by default
From
Noah Misch
Date:
On Thu, Apr 06, 2017 at 01:38:41PM -0400, Peter Eisentraut wrote: > On 3/24/17 10:49, Petr Jelinek wrote: > >>>>> On 07/03/17 06:23, Petr Jelinek wrote: > >>>>>> there has been discussion at the logical replication initial copy thread > >>>>>> [1] about making apply work with sync commit off by default for > >>>>>> performance reasons and adding option to change that per subscription. > >>>>>> > >>>>>> Here I propose patch to implement this - it adds boolean column > >>>>>> subssynccommit to pg_subscription catalog which decides if > >>>>>> synchronous_commit should be off or local for apply. And it adds > >>>>>> SYNCHRONOUS_COMMIT = boolean to the list of WITH options for CREATE and > >>>>>> ALTER SUBSCRIPTION. When nothing is specified it will set it to false. > > > Rebase after table copy patch got committed. > > I think this change could use some more documentation. Under what > circumstances would a user want to turn this back on? [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Peter Eisentraut
Date:
On 3/24/17 10:49, Petr Jelinek wrote: > Rebase after table copy patch got committed. You could please sent another rebase of this, perhaps with some more documentation, as discussed downthread. Also, I wonder why we don't offer the other values of synchronous_commit. In any case, we should keep the values consistent. So on should be on and local should be local, but not mixing it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Peter Eisentraut
Date:
On 4/9/17 22:17, Noah Misch wrote: > [Action required within three days. This is a generic notification.] I'm expecting Petr to post an updated patch by the end of this week. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Petr Jelinek
Date:
On 12/04/17 06:10, Peter Eisentraut wrote: > On 3/24/17 10:49, Petr Jelinek wrote: >> Rebase after table copy patch got committed. > > You could please sent another rebase of this, perhaps with some more > documentation, as discussed downthread. > > Also, I wonder why we don't offer the other values of > synchronous_commit. In any case, we should keep the values consistent. > So on should be on and local should be local, but not mixing it. > Now that I am back from vacation I took look at this again. The reason why I did only boolean initially is that he other values just didn't seem all that useful. But you are right it's better to be consistent and there is no real reason why not to allow all of the possible values. So how about the attached? -- 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
Attachment
Re: [HACKERS] logical replication apply to run with sync commit offby default
From
Peter Eisentraut
Date:
On 4/13/17 17:52, Petr Jelinek wrote: > On 12/04/17 06:10, Peter Eisentraut wrote: >> On 3/24/17 10:49, Petr Jelinek wrote: >>> Rebase after table copy patch got committed. >> >> You could please sent another rebase of this, perhaps with some more >> documentation, as discussed downthread. >> >> Also, I wonder why we don't offer the other values of >> synchronous_commit. In any case, we should keep the values consistent. >> So on should be on and local should be local, but not mixing it. >> > > Now that I am back from vacation I took look at this again. The reason > why I did only boolean initially is that he other values just didn't > seem all that useful. But you are right it's better to be consistent and > there is no real reason why not to allow all of the possible values. > > So how about the attached? committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services