Re: [HACKERS] some review comments on logical rep code - Mailing list pgsql-hackers

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