On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <masao.fu...@gmail.com> wrote: > Hi, > > Though I've read only a part of the logical rep code yet, I'd like to > share some (relatively minor) review comments that I got so far.
It seems nobody is working on dealing with these review comments, so I've attached patches. Since there are lots of review comment I numbered each review comment. The prefix of patches I attached is corresponding to review comment number. 1. > > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer > value from the argument, instead of DatumGetObjectId(). Attached 001 patch fixes it. 2. > > No one resets on_commit_launcher_wakeup flag to false. Attached 002 patch makes on_commit_launcher_wakeup turn off after woke up the launcher process. 3. > > ApplyLauncherWakeup() should be static function. Attached 003 patch fixes it (and also fixes #5 review comment). 4. > > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's > subcommands (e.g., ENABLED one) should wake the launcher up on commit. This is also reported by Horiguchi-san on another thread[1], and patch exists. 5. > > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()? I also guess it's necessary. This change is included in #3 patch. 6. > > Normal statements that the walsender for logical rep runs are logged > by log_replication_commands. So if log_statement is also set, > those commands are logged twice. Attached 006 patch fixes it by adding "-c log_statement = none" to connection parameter of libpqrcv if logical = true. 7. > > LogicalRepCtx->launcher_pid isn't reset to 0 when the launcher emits > an error. The callback function to reset it needs to be registered > via on_shmem_exit(). Attached 007 patch adds callback function to reset LogicalRepCtx->launcher_pid. 8. > > Typo: "an subscriber" should be "a subscriber" in some places. It seems that there is no longer these typo. 9. > /* Get conninfo */ > datum = SysCacheGetAttr(SUBSCRIPTIONOID, > tup, > Anum_pg_subscription_subconninfo, > &isnull); > Assert(!isnull); > > This assertion makes me think that pg_subscription.subconnifo should > have NOT NULL constraint. Also subslotname and subpublications > should have NOT NULL constraint because of the same reason. Agreed. Attached 009 patch add NOT NULL constraint to all other columns of pg_subscription. pg_subscription.subsynccommit should have it as well. 10. > > SpinLockAcquire(&MyLogicalRepWorker->relmutex); > MyLogicalRepWorker->relstate = > GetSubscriptionRelState(MyLogicalRepWorker->subid, > MyLogicalRepWorker->relid, > &MyLogicalRepWorker->relstate_lsn, > false); > SpinLockRelease(&MyLogicalRepWorker->relmutex); > > Non-"short-term" function like GetSubscriptionRelState() should not > be called while spinlock is being held. > One option is adding new LWLock for the relation state change but this lock is used at many locations where the operation actually doesn't take time. I think that the discussion would be needed to get consensus, so patch for it is not attached. [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyot...@lab.ntt.co.jp Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
001_change_DatumGetObjectId_to_DatumGetInt32.patch
Description: Binary data
002_Reset_on_commit_launcher_wakeup.patch
Description: Binary data
003_Fix_ApplyLancherWakeUp_function.patch
Description: Binary data
006_Prevent_to_emit_statement_log_double.patch
Description: Binary data
007_Regiter_on_shmem_exit_for_launcher.patch
Description: Binary data
009_Add_not_null_constraint.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers