On Sun, Apr 16, 2017 at 06:14:49AM +0000, Noah Misch wrote: > On Fri, Apr 14, 2017 at 04:47:12AM +0900, Fujii Masao wrote: > > 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. > > > > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer > > value from the argument, instead of DatumGetObjectId(). > > > > No one resets on_commit_launcher_wakeup flag to false. > > > > ApplyLauncherWakeup() should be static function. > > > > Seems not only CREATE SUBSCRIPTION but also ALTER SUBSCRIPTION's > > subcommands (e.g., ENABLED one) should wake the launcher up on commit. > > > > Why is IsBackendPid() check necessary in ApplyLauncherWakeup()? > > > > 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. > > > > 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(). > > > > Typo: "an subscriber" should be "a subscriber" in some places. > > > > /* 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. > > > > 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. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
