On Thu, Apr 20, 2017 at 12:05 PM, Noah Misch <n...@leadboat.com> wrote: > 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
I'm not the owner of this item, but the current status is; I've pushed several patches, and there is now only one remaining patch. I posted the review comment on that patch, and I'm expecting that Masahiko-san will update the patch. So what about waiting for the updated version of the patch by next Monday (April 24th)? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
