Re: [HACKERS] logical replication apply to run with sync commit off by default
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 -- 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] logical replication apply to run with sync commit off by default
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
From 40f16e060194280a3ce345a1cde57e22a9892007 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription
This also changes default behaviour of subscription workers to
synchronous_commit = off
---
doc/src/sgml/catalogs.sgml | 10 ++
doc/src/sgml/ref/alter_subscription.sgml | 2 ++
doc/src/sgml/ref/create_subscription.sgml | 30 +++-
src/backend/catalog/pg_subscription.c | 8 +
src/backend/commands/subscriptioncmds.c| 56 --
src/backend/replication/logical/launcher.c | 2 +-
src/backend/replication/logical/worker.c | 8 +
src/bin/pg_dump/pg_dump.c | 11 +-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
src/bin/psql/describe.c| 5 ++-
src/include/catalog/pg_subscription.h | 7 ++--
src/test/regress/expected/subscription.out | 27 +++---
src/test/regress/sql/subscription.sql | 3 +-
14 files changed, 141 insertions(+), 31 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 5883673..2d878ad 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6531,6 +6531,16 @@
+ subsynccommit
+ text
+
+
+ Contains value for synchronous_commit setting of the
+ subscription workers.
+
+
+
+
subconninfo
text
diff --git a/doc/src/sgml/ref/alter_subscription.sgml
b/doc/src/sgml/ref/alter_subscription.sgml
index 640fac0..f71ee38 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can
be:
SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = synchronous_commit
ALTER SUBSCRIPTION name SET
PUBLICATION publication_name [,
...] { REFRESH WITH ( puboption [,
... ] ) | NOREFRESH }
ALTER SUBSCRIPTION name REFRESH
PUBLICATION WITH ( puboption [,
... ] )
@@ -91,6 +92,7 @@ ALTER SUBSCRIPTION name DISABLE
CONNECTION 'conninfo'
SLOT NAME = slot_name
+SYNCHRONOUS_COMMIT = synchronous_commit
These clauses alter properties originally set by
diff --git a/doc/src/sgml/ref/create_subscription.sgml
b/doc/src/sgml/ref/create_subscription.sgml
index 3410d6f..3fc010e 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -32,6 +32,7 @@ CREATE SUBSCRIPTION subscription_nameslot_name
| COPY DATA | NOCOPY DATA
+| SYNCHRONOUS_COMMIT = synchronous_commit
| NOCONNECT
@@ -148,7 +149,34 @@ CREATE SUBSCRIPTION subscription_name
-NOCONNECT
+SYNCHRONOUS_COMMIT = synchronous_commit
+
+
+ The value of this parameter overrides the
+ setting. The default value is
+ off.
+
+
+ It's safe to use off for logical replication because
+ in case of transaction loss on subscriber due to missing sync, the data
+ will be resent from publisher.
+
+
+ The logical replication worker will also report position of the writes
+ and flushes to the publisher so the publisher will wait on the actual
+ flush when doing synchronous logical replication. This however means
+ that setting the SYNCHRONOUS_COMMIT for subscriber
+ to off when the subscription is used for synchronous
+ replication may increase the latency for COMMIT on
+ the publisher. In this scenario it may be advantageous to set the
+ SYNCHRONOUS_COMMIT to local or
+ higher.
+
+
+
+
+
+NOCONNECT
Instructs CREATE SUBSCRIPTION to skip the initial
diff --git a/src/backend/catalog/pg_subscription.c
b/src/backend/catalog/pg_subscription.c
index f5ba9f6..4f294d3 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -85,6 +85,14 @@ GetSubscription(Oid subid, bool missing_ok)
Assert(!isnull
Re: [HACKERS] logical replication apply to run with sync commit off by default
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 -- 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] logical replication apply to run with sync commit off by default
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 -- 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] logical replication apply to run with sync commit off by default
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 -- 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] logical replication apply to run with sync commit off by default
On 7 April 2017 at 01:38, Peter Eisentraut 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 -- 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] logical replication apply to run with sync commit off by default
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 -- 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] logical replication apply to run with sync commit off by default
On Fri, Mar 24, 2017 at 11:49 PM, Petr Jelinek 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 >>> 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=xoor8j71mad1oteojawmuje3...@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). - + + The value of this parameter overrides the + synchronous_commit setting. + The default value is false. + + + If true, the apply for the subscription will run with + synchronous_commit set to local. + Otherwise it will have it set to false. + 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 -- 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] logical replication apply to run with sync commit off by default
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
>> 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=xoor8j71mad1oteojawmuje3...@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
>From 4e56f693a0b82d116930a03d63c7d034ef004f25 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription
This also changes default behaviour of subscription workers to
synchronous_commit = off
---
doc/src/sgml/catalogs.sgml | 11 +++
doc/src/sgml/ref/alter_subscription.sgml | 2 ++
doc/src/sgml/ref/create_subscription.sgml | 12
src/backend/catalog/pg_subscription.c | 1 +
src/backend/commands/subscriptioncmds.c| 49 --
src/backend/replication/logical/launcher.c | 2 +-
src/backend/replication/logical/worker.c | 10 ++
src/bin/pg_dump/pg_dump.c | 12 +++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
src/bin/psql/describe.c| 5 ++-
src/include/catalog/pg_subscription.h | 11 ---
src/test/regress/expected/subscription.out | 27
src/test/regress/sql/subscription.sql | 3 +-
14 files changed, 116 insertions(+), 32 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index c531c73..9e072ea 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6414,6 +6414,17 @@
+ subsynccommit
+ bool
+
+
+ If true, the apply for the subscription will run with
+ synchronous_commit set to local.
+ Otherwise it will have it set to false.
+
+
+
+
subconninfo
text
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6f94247..572e370 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,8 @@ ALTER SUBSCRIPTION name WITH ( where suboption can be:
SLOT NAME = slot_name
+SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = boolean
ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] { REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
ALTER SUBSCRIPTION name REFRESH PUBLICATION WITH ( puboption [, ... ] )
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 8f3c30b..479f4e4 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -32,6 +32,7 @@ CREATE SUBSCRIPTION subscription_nameslot_name
| COPY DATA | NOCOPY DATA
+| SYNCHRONOUS_COMMIT = boolean
| NOCONNECT
@@ -148,6 +149,17 @@ CREATE SUBSCRIPTION subscription_name
+SYNCHRONOUS_COMMIT = boolean
+
+
+ The value of this parameter overrides the
+ synchronous_commit setting.
+ The default value is false.
+
+
+
+
+
NOCONNECT
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index e420ec1..e7a1634 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -68,6 +68,7 @@ GetSubscription(Oid subid, bool missing_ok)
sub->name = pstrdup(NameStr(subform->subname));
sub->owner = subform->subowner;
sub->enabled = subform->subenabled;
+ sub->synccommit = subform->subsynccommit;
/* Get conninfo */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 0784ca7..8f201b2 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -60,12 +60,13 @@ static List *fetch_table_list(
Re: [HACKERS] logical replication apply to run with sync commit off by default
On 21/03/17 18:54, Robert Haas wrote:
> On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek
> 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=xoor8j71mad1oteojawmuje3...@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.
>
> + local otherwise to false. The
> + default value is false independently of the default
> + synchronous_commit 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
From 1234142533027e411a2ea5feff60f782402cefe2 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription
This also changes default behaviour of subscription workers to
synchronous_commit = off
---
doc/src/sgml/catalogs.sgml | 11 +++
doc/src/sgml/ref/alter_subscription.sgml | 1 +
doc/src/sgml/ref/create_subscription.sgml | 12
src/backend/catalog/pg_subscription.c | 1 +
src/backend/commands/subscriptioncmds.c| 49 --
src/backend/replication/logical/launcher.c | 2 +-
src/backend/replication/logical/worker.c | 10 ++
src/bin/pg_dump/pg_dump.c | 12 +++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
src/bin/psql/describe.c| 5 ++-
src/include/catalog/pg_subscription.h | 11 ---
src/test/regress/expected/subscription.out | 27
src/test/regress/sql/subscription.sql | 3 +-
14 files changed, 115 insertions(+), 32 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 228ec78..f71d9c9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6395,6 +6395,17 @@
+ subsynccommit
+ bool
+
+
+ If true, the apply for the subscription will run with
+ synchronous_commit set to local.
+ Otherwise it will have it set to false.
+
+
+
+
subconninfo
text
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6335e17..712de98 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can be:
SLOT NAME = slot_name
+| SYNCHRONOUS_
Re: [HACKERS] logical replication apply to run with sync commit off by default
On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek
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=xoor8j71mad1oteojawmuje3...@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?
+ local otherwise to false. The
+ default value is false independently of the default
+ synchronous_commit 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
--
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] logical replication apply to run with sync commit off by default
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=xoor8j71mad1oteojawmuje3...@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
>From 800b56aab4714333c2e240c087518a8d1b5e7dc0 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription
This also changes default behaviour of subscription workers to
synchronous_commit = off
---
doc/src/sgml/catalogs.sgml | 11 ++
doc/src/sgml/ref/alter_subscription.sgml | 1 +
doc/src/sgml/ref/create_subscription.sgml | 15
src/backend/catalog/pg_subscription.c | 1 +
src/backend/commands/subscriptioncmds.c| 59 +-
src/backend/replication/logical/launcher.c | 2 +-
src/backend/replication/logical/worker.c | 28 +-
src/bin/pg_dump/pg_dump.c | 12 +-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
src/bin/psql/describe.c| 5 ++-
src/include/catalog/pg_subscription.h | 11 --
src/test/regress/expected/subscription.out | 27 +++---
src/test/regress/sql/subscription.sql | 3 +-
14 files changed, 144 insertions(+), 34 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 228ec78..f71d9c9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6395,6 +6395,17 @@
+ subsynccommit
+ bool
+
+
+ If true, the apply for the subscription will run with
+ synchronous_commit set to local.
+ Otherwise it will have it set to false.
+
+
+
+
subconninfo
text
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6335e17..712de98 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can be:
SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = boolean
ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] { REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
ALTER SUBSCRIPTION name REFRESH PUBLICATION WITH ( puboption [, ... ] )
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 6468470..6baff2f 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -29,6 +29,7 @@ CREATE SUBSCRIPTION subscription_name
@@ -145,6 +146,20 @@ CREATE SUBSCRIPTION subscription_name
+SYNCHRONOUS_COMMIT = boolean
+
+
+ Modifies the synchronous_commit setting of the
+ subscription workers. When set to true, the
+ synchronous_commit for the worker will be set to
+ local otherwise to false. The
+ default value is false independently of the default
+ synchronous_commit setting for the instance.
+
+
+
+
+
NOCONNECT
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9b74892..26921aa 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -68,6 +68,7 @@ GetSubscription(Oid subid, bool missing_ok)
sub->name = pstrdup(NameStr(subform->subname));
sub->owner = subform->subowner;
sub->enabled = subform->subenabled;
+ sub->synccommit = subform->subsynccommit;
/* Get conninfo */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index cba2d5c..402f682 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -60,12 +60,13 @@ static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
static void
parse_subscription_options(List *options, bool *connect, bool
Re: [HACKERS] logical replication apply to run with sync commit off by default
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=xoor8j71mad1oteojawmuje3...@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
>From 0876a23ddc385a6c4dc8dc26abcd1e82c9ab2482 Mon Sep 17 00:00:00 2001
From: Petr Jelinek
Date: Mon, 6 Mar 2017 13:07:45 +0100
Subject: [PATCH] Add option to modify sync commit per subscription
This also changes default behaviour of subscription workers to
synchronous_commit = off
---
doc/src/sgml/catalogs.sgml | 11 ++
doc/src/sgml/ref/alter_subscription.sgml | 1 +
doc/src/sgml/ref/create_subscription.sgml | 15
src/backend/catalog/pg_subscription.c | 1 +
src/backend/commands/subscriptioncmds.c| 59 +-
src/backend/replication/logical/launcher.c | 2 +-
src/backend/replication/logical/worker.c | 28 +-
src/bin/pg_dump/pg_dump.c | 12 +-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c| 5 ++-
src/include/catalog/pg_subscription.h | 11 --
src/test/regress/expected/subscription.out | 27 +++---
src/test/regress/sql/subscription.sql | 3 +-
13 files changed, 143 insertions(+), 33 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 228ec78..f71d9c9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6395,6 +6395,17 @@
+ subsynccommit
+ bool
+
+
+ If true, the apply for the subscription will run with
+ synchronous_commit set to local.
+ Otherwise it will have it set to false.
+
+
+
+
subconninfo
text
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6335e17..712de98 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -26,6 +26,7 @@ ALTER SUBSCRIPTION name WITH ( where suboption can be:
SLOT NAME = slot_name
+| SYNCHRONOUS_COMMIT = boolean
ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] { REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
ALTER SUBSCRIPTION name REFRESH PUBLICATION WITH ( puboption [, ... ] )
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 6468470..6baff2f 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -29,6 +29,7 @@ CREATE SUBSCRIPTION subscription_name
@@ -145,6 +146,20 @@ CREATE SUBSCRIPTION subscription_name
+SYNCHRONOUS_COMMIT = boolean
+
+
+ Modifies the synchronous_commit setting of the
+ subscription workers. When set to true, the
+ synchronous_commit for the worker will be set to
+ local otherwise to false. The
+ default value is false independently of the default
+ synchronous_commit setting for the instance.
+
+
+
+
+
NOCONNECT
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9b74892..26921aa 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -68,6 +68,7 @@ GetSubscription(Oid subid, bool missing_ok)
sub->name = pstrdup(NameStr(subform->subname));
sub->owner = subform->subowner;
sub->enabled = subform->subenabled;
+ sub->synccommit = subform->subsynccommit;
/* Get conninfo */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index cba2d5c..402f682 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -60,12 +60,13 @@ static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
static void
parse_subscription_options(List *options, bool *connect, bool *enabled_given,
bool *enabled, bool *create_slot, char **slot_name,
- bool *copy_data)
+ bool *copy_data, bool *synchronous_commit)
{
