On 01/31/2018 07:53 AM, Masahiko Sawada wrote: > On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra > <tomas.von...@2ndquadrant.com> wrote: >> On 01/19/2018 03:34 PM, Tomas Vondra wrote: >>> Attached is v5, fixing a silly bug in part 0006, causing segfault when >>> creating a subscription. >>> >> >> Meh, there was a bug in the sgml docs (<variable> vs. <varname>), >> causing another failure. Hopefully v6 will pass the CI build, it does >> pass a build with the same parameters on my system. > > Thank you for working on this. This patch would be helpful for > synchronous replication. > > I haven't looked at the code deeply yet, but I've reviewed the v6 > patch set especially on subscriber side. All of the patches can be > applied to current HEAD cleanly. Here is review comment. > > ---- > CREATE SUBSCRIPTION commands accept work_mem < 64 but it leads ERROR > on publisher side when starting replication. Probably we should check > the value on the subscriber side as well. > > ---- > When streaming = on, if we drop subscription in the middle of > receiving stream changes, DROP SUBSCRIPTION could leak tmp files > (.chages file and .subxacts file). Also it also happens when a > transaction on upstream aborted without abort record. > > ---- > Since we can change both streaming option and work_mem option by ALTER > SUBSCRIPTION, documentation of ALTER SUBSCRIPTION needs to be updated. > > ---- > If we create a subscription without any options, both > pg_subscription.substream and pg_subscription.subworkmem are set to > null. However, since GetSubscription are't aware of NULL we start the > replication with invalid options like follows. > LOG: received replication command: START_REPLICATION SLOT "hoge_sub" > LOGICAL 0/0 (proto_version '2', work_mem '893219954', streaming 'on', > publication_names '"hoge_pub"') > > I think we can set substream to false and subworkmem to -1 instead of > null, and then makes libpqrcv_startstreaming not set streaming option > if stream is -1. > > ---- > Some WARNING messages appeared. Maybe these are for debug purpose? > > WARNING: updating stream stats 0x1c12ef8 4 3 65604 > WARNING: UpdateSpillStats: updating stats 0x1c12ef8 0 0 0 39 41 2632080 > > Regards, >
Thanks for the review! I'll address the issues in the next version of the patch. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
