Re: Logical Replication WIP

On Sat, Jan 21, 2017 at 1:39 AM, Petr Jelinek
<petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
> On 20/01/17 17:33, Jaime Casanova wrote:
>> On 20 January 2017 at 11:25, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>>> On 20/01/17 17:05, Fujii Masao wrote:
>>>> On Fri, Jan 20, 2017 at 11:08 PM, Peter Eisentraut
>>>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>>>> On 1/19/17 5:01 PM, Petr Jelinek wrote:
>>>>>> There were some conflicting changes committed today so I rebased the
>>>>>> patch on top of them.
>>>>>>
>>>>>> Other than that nothing much has changed, I removed the separate sync
>>>>>> commit patch, included the rename patch in the patchset and fixed the
>>>>>> bug around pg_subscription catalog reported by Erik Rijkers.
>>>>>
>>>>> Committed.
>>>>
>>>> Sorry I've not followed the discussion about logical replication at all, but
>>>> why does logical replication launcher need to start up by default?
>>>>
>>>
>>> Because running subscriptions is allowed by default. You'd need to set
>>> max_logical_replication_workers to 0 to disable that.
>>>
>>
>> surely wal_level < logical shouldn't start a logical replication
>> launcher, and after an initdb wal_level is only replica
>>
>
> Launcher is needed for subscriptions, subscriptions don't depend on
> wal_level.

But why did you enable only subscription by default while publication is
disabled by default (i.e., wal_level != logical)? I think that it's better to
enable both by default OR disable both by default.

While I was reading the logical rep code, I found that
logicalrep_worker_launch returns *without* releasing LogicalRepWorkerLock
when there is no unused worker slot. This seems a bug.

/* Report this after the initial starting message for consistency. */
if (max_replication_slots == 0)
ereport(ERROR,
(errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
errmsg("cannot start logical replication workers when
max_replication_slots = 0")));

logicalrep_worker_launch checks max_replication_slots as above.
Why does it need to check that setting value in the *subscriber* side?
Maybe I'm missing something here, but ISTM that the subscription uses
one replication slot in *publisher* side but doesn't use in *subscriber* side.

* The apply worker may spawn additional workers (sync) for initial data
* synchronization of tables.

The above header comment in logical/worker.c is true?

The copyright in each file that the commit of logical rep added needs to
be updated.

Regards,

--
Fujii Masao