Re: Logical Replication WIP

On 25/01/17 18:16, Peter Eisentraut wrote:
> On 1/22/17 8:11 PM, Petr Jelinek wrote:
>> 0001 - Changes the libpqrcv_connect to use async libpq api so that it
>> won't get stuck forever in case of connect is stuck. This is preexisting
>> bug that also affects walreceiver but it's less visible there as there
>> is no SQL interface to initiate connection there.
>
> Probably a mistake here:
>
> + case PGRES_POLLING_READING:
> + extra_flag = WL_SOCKET_READABLE;
> + /* pass through */
> + case PGRES_POLLING_WRITING:
> + extra_flag = WL_SOCKET_WRITEABLE;
>
> extra_flag gets overwritten in the reading case.
>

Eh, reworked that to just if statement as switch does not really buy us
anything there.

> Please elaborate in the commit message what this change is for.
>

Okay.

>> 0002 - Close replication connection when CREATE SUBSCRIPTION gets
>> canceled (otherwise walsender on the other side may stay in idle in
>> transaction state).
>
> committed

Thanks!

>
>> 0003 - Fixes buffer initialization in walsender that I found when
>> testing the above two. This one should be back-patched to 9.4 since it's
>> broken since then.
>
> Can you explain more in which code path this problem occurs?

With existing code base, anything that calls WalSndWaitForWal (it calls
ProcessRepliesIfAny()) which is called from logical_read_xlog_page which
is given as callback to logical decoding in CreateReplicationSlot and
StartLogicalReplication.

The reason why I decided to put it into init is that following up all
the paths to where buffers are used is rather complicated due to various
callbacks so if anybody else starts poking around in the future it might
get easily broken again if we don't initialize those unconditionally
(plus the memory footprint is few kB and in usual use of WalSender they
will eventually be initialized anyway as they are needed for streaming).

> I think we should get rid of the global variables and give each function
> its own buffer that it initializes the first time through. Otherwise
> we'll keep having to worry about this.
>

Because of above, it would mean some refactoring in logical decoding
APIs not just in WalSender so that would not be backpatchable (and in
general it's much bigger patch then).

>> 0004 - Fixes the foreign key issue reported by Thom Brown and also adds
>> tests for FK and trigger handling.
>
> I think the trigger handing should go into execReplication.c.
>

Not in the current state, eventually (and I am afraid that PG11 material
at this point as we still have partitioned tables support and initial
data copy to finish in this release) we'll want to move all the executor
state code to execReplication.c and do less of reinitialization but in
the current code the trigger stuff belongs to worker IMHO.

>> 0005 - Adds support for renaming publications and subscriptions.
>
> Could those not be handled in the generic rename support in
> ExecRenameStmt()?

Yes seems they can.

Attached updated version of the uncommitted patches.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services