From: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> |
---|---|
To: | Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> |
Cc: | Erik Rijkers <er(at)xs4all(dot)nl>, Andres Freund <andres(at)anarazel(dot)de>, Steve Singer <steve(at)ssinger(dot)info>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, pgsql-hackers-owner(at)postgresql(dot)org |
Subject: | Re: Logical Replication WIP |
Date: | 2016-12-13 20:42:17 |
Message-ID: | c3d7de89-67ff-f21e-27ae-ac99988230c5@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
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.
DROP SUBSCRIPTION requires superuser, but should perhaps be owner check
only?
DROP SUBSCRIPTION IF EXISTS crashes if the subscription does not in fact
exist.
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.
The tests are not added to serial_schedule. Intentional? If so, document?
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?
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.
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.
I think this is getting very close to the point where it's committable.
So if anyone else has major concerns about the whole approach and
perhaps the way the new code in 0005 is organized, now would be the time ...
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment | Content-Type | Size |
---|---|---|
0002-fixup-Add-PUBLICATION-catalogs-and-DDL.patch | text/x-patch | 1.2 KB |
0004-fixup-Add-SUBSCRIPTION-catalog-and-DDL.patch | text/x-patch | 19.7 KB |
0006-fixup-Define-logical-replication-protocol-and-output.patch | text/x-patch | 13.4 KB |
0008-fixup-Add-logical-replication-workers.patch | text/x-patch | 28.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-12-13 21:05:11 | Re: Logical Replication WIP |
Previous Message | Tom Lane | 2016-12-13 19:44:07 | Re: New design for FK-based join selectivity estimation |