Re: 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
> <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?

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