On 13/12/16 21:42, Peter Eisentraut wrote:
> On 12/10/16 2:48 AM, Petr Jelinek wrote:
>> Attached new version with your updates and rebased on top of the current
>> HEAD (the partitioning patch produced quite a few conflicts).
>
> I have attached a few more "fixup" patches, mostly with some editing of
> documentation and comments and some compiler warnings.
>
> In 0006 in the protocol documentation I have left a "XXX ???" where I
> didn't understand what it was trying to say.
>
Okay I'll address that separately, thanks.
> All issues from (my) previous reviews appear to have been addressed.
>
> Comments besides that:
>
>
> 0003-Add-SUBSCRIPTION-catalog-and-DDL-v12.patch
>
> Still wondering about the best workflow with pg_dump, but it seems all
> the pieces are there right now, and the interfaces can be tweaked later.
Right, either way there needs to be some special handling for
subscriptions, having to request them specifically seems safest option
to me, but I am open to suggestions there.
>
> DROP SUBSCRIPTION requires superuser, but should perhaps be owner check
> only?
>
Hmm not sure that it requires superuser, I actually think it mistakenly
didn't require anything. In any case will make sure it just does owner
check.
> DROP SUBSCRIPTION IF EXISTS crashes if the subscription does not in fact
> exist.
>
Right, missing return.
> Maybe write the grammar so that SLOT does not need to be a new key word.
> The changes you made for CREATE PUBLICATION should allow that.
>
Hmm how would that look like? The opt_drop_slot would become IDENT
IDENT? Or maybe you want me to add the WITH (definition) kind of thing?
> The tests are not added to serial_schedule. Intentional? If so, document?
>
Not intentional, will fix. Never use it, easy to forget about it.
>
> 0004-Define-logical-replication-protocol-and-output-plugi-v12.patch
>
> Not sure why pg_catalog is encoded as a zero-length string. I guess it
> saves some space. Maybe that could be explained in a brief code comment?
>
Yes it's to save space, mainly for built-in types.
>
> 0005-Add-logical-replication-workers-v12.patch
>
> The way the executor stuff is organized now looks better to me.
>
> The subscriber crashes if max_replication_slots is 0:
>
> TRAP: FailedAssertion("!(max_replication_slots > 0)", File: "origin.c",
> Line: 999)
>
> The documentation says that replication slots are required on the
> subscriber, but from a user's perspective, it's not clear why that is.
Yeah honestly I think origins should not depend on
max_replication_slots. They are not really connected (you can have many
of one and none of the other and vice versa). Also max_replication_slots
should IMHO default to max_wal_senders at this point. (In ideal world
all of those 3 would be in DSM instead of SHM and only governed by some
implementation maximum which is probably 2^16 and the GUCs would be removed)
But yes as it is, we should check for that, probably both during CREATE
SUBSCRIPTION and during apply start.
>
> Dropping a table that is part of a live subscription results in log
> messages like
>
> WARNING: leaked hash_seq_search scan for hash table 0x7f9d2a807238
>
> I was testing replicating into a temporary table, which failed like this:
>
> FATAL: the logical replication target public.test1 not found
> LOG: worker process: (PID 2879) exited with exit code 1
> LOG: starting logical replication worker for subscription 16392
> LOG: logical replication apply for subscription mysub started
>
> That's okay, but those messages were repeated every few seconds or so
> and would create quite some log volume. I wonder if that needs to be
> reigned in somewhat.
It retries every 5s or so I think, I am not sure how that could be
improved besides using the wal_retrieve_retry_interval instead of
hardcoded 5s (or maybe better add GUC for apply). Maybe some kind of
backoff algorithm could be added as well.
--
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
