Re: logical replication apply to run with sync commit off by default

On Mon, Mar 20, 2017 at 7:56 PM, Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)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