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
> <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