some review comments on logical rep code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

some review comments on logical rep code

Fujii Masao-2
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.

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.

Regards,

--
Fujii Masao


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Noah Misch-2
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Masahiko Sawada
In reply to this post by Fujii Masao-2
On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> 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.kyotaro@...

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

001_change_DatumGetObjectId_to_DatumGetInt32.patch (998 bytes) Download Attachment
002_Reset_on_commit_launcher_wakeup.patch (816 bytes) Download Attachment
003_Fix_ApplyLancherWakeUp_function.patch (1K) Download Attachment
006_Prevent_to_emit_statement_log_double.patch (1K) Download Attachment
007_Regiter_on_shmem_exit_for_launcher.patch (1K) Download Attachment
009_Add_not_null_constraint.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Kyotaro HORIGUCHI-2
At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>

> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> 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.

Hmm. I looked at the launcher side and found that it assigns bare
integer to bgw_main_arg. It also should use Int32GetDatum.

> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid,
>              Oid relid)
> {
>     int          slot;
...
>     for (slot = 0; slot < max_logical_replication_workers; slot++)
...
>     bgw.bgw_main_arg = slot;



> 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.

It is omitting the abort case. Maybe it should be
AtEOXact_ApplyLauncher(bool commit)?

> 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.

IsBackendPid() is not free, I suppose that just ignoring failure
with ESRCH is enough.

> 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.

The control by log_replication_commands is performed on
walsender, so this also shuld be done on the same place. Anyway
log_statement is irrelevant to walsender.

> 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.

I'm not sure the policy here, but I suppose that the assertions
there are still required irrelevantly from the nun-nullness of
the attribute.

> 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.

From the point of view of time, I agree that it doesn't seem to
harm. Bt I thing it as more significant problem that
potentially-throwable function is called within the mutex
region. It potentially causes a kind of dead lock.  (That said,
theoretically dosn't occur and I'm not sure what happens by the
dead lock..)


> [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyotaro@...

--
Kyotaro Horiguchi
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Masahiko Sawada
On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
<[hidden email]> wrote:

> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>
>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> 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.
>>

Thank you for reviewing.

>> 1.
>> >
>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>> > value from the argument, instead of DatumGetObjectId().
>>
>> Attached 001 patch fixes it.
>
> Hmm. I looked at the launcher side and found that it assigns bare
> integer to bgw_main_arg. It also should use Int32GetDatum.

Yeah, I agreed. Will fix it.

>
>> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid,
>>              Oid relid)
>> {
>>     int          slot;
> ...
>>     for (slot = 0; slot < max_logical_replication_workers; slot++)
> ...
>>     bgw.bgw_main_arg = slot;
>
>
>
>> 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.
>
> It is omitting the abort case. Maybe it should be
> AtEOXact_ApplyLauncher(bool commit)?

Should we wake up the launcher process even when abort?

>
>> 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.
>
> IsBackendPid() is not free, I suppose that just ignoring failure
> with ESRCH is enough.

After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
check if launcher_pid != 0?

>
>> 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.
>
> The control by log_replication_commands is performed on
> walsender, so this also shuld be done on the same place. Anyway
> log_statement is irrelevant to walsender.

Patch 006 emits all logs executed by the apply worker as a log of
log_replication_command but perhaps you're right. It would be better
to leave the log of log_statement if the command executed by the apply
worker is a SQL command. Will fix.

>
>> 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.
>
> I'm not sure the policy here, but I suppose that the assertions
> there are still required irrelevantly from the nun-nullness of
> the attribute.

You're right. Will fix it.

>> 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.
>
> From the point of view of time, I agree that it doesn't seem to
> harm. Bt I thing it as more significant problem that
> potentially-throwable function is called within the mutex
> region. It potentially causes a kind of dead lock.  (That said,
> theoretically dosn't occur and I'm not sure what happens by the
> dead lock..)
>
>
>> [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyotaro@...
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>


Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Masahiko Sawada
On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <[hidden email]> wrote:

> On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
> <[hidden email]> wrote:
>> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>
>>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> 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.
>>>
>
> Thank you for reviewing.
>
>>> 1.
>>> >
>>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>>> > value from the argument, instead of DatumGetObjectId().
>>>
>>> Attached 001 patch fixes it.
>>
>> Hmm. I looked at the launcher side and found that it assigns bare
>> integer to bgw_main_arg. It also should use Int32GetDatum.
>
> Yeah, I agreed. Will fix it.
>
>>
>>> logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid,
>>>              Oid relid)
>>> {
>>>     int          slot;
>> ...
>>>     for (slot = 0; slot < max_logical_replication_workers; slot++)
>> ...
>>>     bgw.bgw_main_arg = slot;
>>
>>
>>
>>> 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.
>>
>> It is omitting the abort case. Maybe it should be
>> AtEOXact_ApplyLauncher(bool commit)?
>
> Should we wake up the launcher process even when abort?
>
>>
>>> 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.
>>
>> IsBackendPid() is not free, I suppose that just ignoring failure
>> with ESRCH is enough.
>
> After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
> check if launcher_pid != 0?
>
>>
>>> 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.
>>
>> The control by log_replication_commands is performed on
>> walsender, so this also shuld be done on the same place. Anyway
>> log_statement is irrelevant to walsender.
>
> Patch 006 emits all logs executed by the apply worker as a log of
> log_replication_command but perhaps you're right. It would be better
> to leave the log of log_statement if the command executed by the apply
> worker is a SQL command. Will fix.
>
>>
>>> 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.
>>
>> I'm not sure the policy here, but I suppose that the assertions
>> there are still required irrelevantly from the nun-nullness of
>> the attribute.
>
> You're right. Will fix it.
>
>>> 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.
>>
>> From the point of view of time, I agree that it doesn't seem to
>> harm. Bt I thing it as more significant problem that
>> potentially-throwable function is called within the mutex
>> region. It potentially causes a kind of dead lock.  (That said,
>> theoretically dosn't occur and I'm not sure what happens by the
>> dead lock..)
>>
>>
>>> [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyotaro@...
>>
>> --
>> Kyotaro Horiguchi
>> NTT Open Source Software Center
>>
>
>
I've attached latest v2 three patches; 001, 006 and 009. The review
comments I got so far are incorporated.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

001_change_DatumGetObjectId_to_DatumGetInt32_v2.patch (1K) Download Attachment
006_Prevent_to_emit_statement_log_double_v2.patch (1K) Download Attachment
009_Add_not_null_constraint_v2.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Kyotaro HORIGUCHI-2
Hi,

Thank you for the revised version.

At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=[hidden email]>

> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <[hidden email]> wrote:
> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
> > <[hidden email]> wrote:
> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>
> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> wrote:
> >>> 1.
> >>> >
> >>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
> >>> > value from the argument, instead of DatumGetObjectId().
> >>>
> >>> Attached 001 patch fixes it.
> >>
> >> Hmm. I looked at the launcher side and found that it assigns bare
> >> integer to bgw_main_arg. It also should use Int32GetDatum.
> >
> > Yeah, I agreed. Will fix it.

Thanks.

> >>> 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.
> >>
> >> It is omitting the abort case. Maybe it should be
> >> AtEOXact_ApplyLauncher(bool commit)?
> >
> > Should we wake up the launcher process even when abort?

No, I meant that on_commit_launcher_wakeup should be just reset
without waking up launcher on abort.

> >>> 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.
> >>
> >> IsBackendPid() is not free, I suppose that just ignoring failure
> >> with ESRCH is enough.
> >
> > After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
> > check if launcher_pid != 0?

Yes. For example, code to send a signal to autovacuum launcher
looks as the follows. I don't think there's a reason to do
different thing here.

|       avlauncher_needs_signal = false;
|       if (AutoVacPID != 0)
|         kill(AutoVacPID, SIGUSR2);


> >>> 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.
> >>
> >> The control by log_replication_commands is performed on
> >> walsender, so this also shuld be done on the same place. Anyway
> >> log_statement is irrelevant to walsender.
> >
> > Patch 006 emits all logs executed by the apply worker as a log of
> > log_replication_command but perhaps you're right. It would be better
> > to leave the log of log_statement if the command executed by the apply
> > worker is a SQL command. Will fix.

Here, we can choose whether a SQL command is a part of
replication commands or not. The previous fix thought it is and
this doesn't. (They are emitted in different forms) Is this ok?

I'm not sure why you have moved the location of the code, but if
it should show all received commands, it might be better to be
placed before CHECK_FOR_INTERRUPTS..

The comment for the code seems should be more clearly.

- * compatibility. To prevent the command is logged doubly, we doesn't
- * log it when the command is a SQL command.
+ * compatibility. SQL command are logged later according
+ * to log_statement setting.

> >>> 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.
> >>
> >> I'm not sure the policy here, but I suppose that the assertions
> >> there are still required irrelevantly from the nun-nullness of
> >> the attribute.
> >
> > You're right. Will fix it.
> >
> >>> 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.
> >>
> >> From the point of view of time, I agree that it doesn't seem to
> >> harm. Bt I thing it as more significant problem that
> >> potentially-throwable function is called within the mutex
> >> region. It potentially causes a kind of dead lock.  (That said,
> >> theoretically dosn't occur and I'm not sure what happens by the
> >> dead lock..)
> >>
> >>
> >>> [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyotaro@...
> >>
> >> --
> >> Kyotaro Horiguchi
> >> NTT Open Source Software Center
> >>
> >
> >
>
> I've attached latest v2 three patches; 001, 006 and 009. The review
> comments I got so far are incorporated.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Masahiko Sawada
On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
<[hidden email]> wrote:

> Hi,
>
> Thank you for the revised version.
>
> At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=[hidden email]>
>> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <[hidden email]> wrote:
>> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
>> > <[hidden email]> wrote:
>> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>
>> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> wrote:
>> >>> 1.
>> >>> >
>> >>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>> >>> > value from the argument, instead of DatumGetObjectId().
>> >>>
>> >>> Attached 001 patch fixes it.
>> >>
>> >> Hmm. I looked at the launcher side and found that it assigns bare
>> >> integer to bgw_main_arg. It also should use Int32GetDatum.
>> >
>> > Yeah, I agreed. Will fix it.
>
> Thanks.
>
>> >>> 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.
>> >>
>> >> It is omitting the abort case. Maybe it should be
>> >> AtEOXact_ApplyLauncher(bool commit)?
>> >
>> > Should we wake up the launcher process even when abort?
>
> No, I meant that on_commit_launcher_wakeup should be just reset
> without waking up launcher on abort.
I understood. Sounds reasonable.

>
>> >>> 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.
>> >>
>> >> IsBackendPid() is not free, I suppose that just ignoring failure
>> >> with ESRCH is enough.
>> >
>> > After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
>> > check if launcher_pid != 0?
>
> Yes. For example, code to send a signal to autovacuum launcher
> looks as the follows. I don't think there's a reason to do
> different thing here.
>
> |       avlauncher_needs_signal = false;
> |       if (AutoVacPID != 0)
> |         kill(AutoVacPID, SIGUSR2);
>
Fixed.

>
>> >>> 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.
>> >>
>> >> The control by log_replication_commands is performed on
>> >> walsender, so this also shuld be done on the same place. Anyway
>> >> log_statement is irrelevant to walsender.
>> >
>> > Patch 006 emits all logs executed by the apply worker as a log of
>> > log_replication_command but perhaps you're right. It would be better
>> > to leave the log of log_statement if the command executed by the apply
>> > worker is a SQL command. Will fix.
>
> Here, we can choose whether a SQL command is a part of
> replication commands or not. The previous fix thought it is and
> this doesn't. (They are emitted in different forms) Is this ok?
Yes, v2 patch logs other than T_SQLCmd as a replication command, and
T_SQLCmd is logged later in exec_simple_query. The
log_replication_command logs only replication commands[1], it doesn't
mean to log commands executed on replication connection. I think such
behavior is right.

> I'm not sure why you have moved the location of the code, but if
> it should show all received commands, it might be better to be
> placed before CHECK_FOR_INTERRUPTS..

Hmm, the reason why I've moved it is that we cannot know whether given
cmd_string is a SQL command or a replication command before parsing.

>
> The comment for the code seems should be more clearly.
>
> - * compatibility. To prevent the command is logged doubly, we doesn't
> - * log it when the command is a SQL command.
> + * compatibility. SQL command are logged later according
> + * to log_statement setting.

Fixed.

>> >>> 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.
>> >>
>> >> I'm not sure the policy here, but I suppose that the assertions
>> >> there are still required irrelevantly from the nun-nullness of
>> >> the attribute.
>> >
>> > You're right. Will fix it.
>> >
>> >>> 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.
>> >>
>> >> From the point of view of time, I agree that it doesn't seem to
>> >> harm. Bt I thing it as more significant problem that
>> >> potentially-throwable function is called within the mutex
>> >> region. It potentially causes a kind of dead lock.  (That said,
>> >> theoretically dosn't occur and I'm not sure what happens by the
>> >> dead lock..)
>> >>
>> >>
>> >>> [1] https://www.postgresql.org/message-id/20170406.212441.177025736.horiguchi.kyotaro@...
>> >>
>> >> --
>> >> Kyotaro Horiguchi
>> >> NTT Open Source Software Center
>> >>
>> >
>> >
>>
>> I've attached latest v2 three patches; 001, 006 and 009. The review
>> comments I got so far are incorporated.
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
Attached current latest all patches.

[1] https://www.postgresql.org/docs/devel/static/runtime-config-logging.html#guc-log-replication-commands

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

006_Prevent_to_emit_statement_log_double_v3.patch (2K) Download Attachment
001_change_DatumGetObjectId_to_DatumGetInt32_v3.patch (1K) Download Attachment
002_Reset_on_commit_launcher_wakeup_v3.patch (2K) Download Attachment
009_Add_not_null_constraint_v3.patch (1K) Download Attachment
007_Regiter_on_shmem_exit_for_launcher_v3.patch (1K) Download Attachment
003_005_Fix_ApplyLancherWakeUp_function_v3.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Petr Jelinek-4
Thank you for working on this!

On 18/04/17 10:16, Masahiko Sawada wrote:

> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
> <[hidden email]> wrote:
>>
>>>>>> 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.
>>>>>
>>>>> IsBackendPid() is not free, I suppose that just ignoring failure
>>>>> with ESRCH is enough.
>>>>
>>>> After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
>>>> check if launcher_pid != 0?
>>
>> Yes. For example, code to send a signal to autovacuum launcher
>> looks as the follows. I don't think there's a reason to do
>> different thing here.
>>
>> |       avlauncher_needs_signal = false;
>> |       if (AutoVacPID != 0)
>> |         kill(AutoVacPID, SIGUSR2);
>>
>
> Fixed.
Yes the IsBackendPid was needed only because we didn't reset
launcher_pid and it was causing issues otherwise obviously (and I didn't
realize we are not resetting it...)

>
>>
>>>>>> 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.
>>>>>
>>>>> The control by log_replication_commands is performed on
>>>>> walsender, so this also shuld be done on the same place. Anyway
>>>>> log_statement is irrelevant to walsender.
>>>>
>>>> Patch 006 emits all logs executed by the apply worker as a log of
>>>> log_replication_command but perhaps you're right. It would be better
>>>> to leave the log of log_statement if the command executed by the apply
>>>> worker is a SQL command. Will fix.
>>
>> Here, we can choose whether a SQL command is a part of
>> replication commands or not. The previous fix thought it is and
>> this doesn't. (They are emitted in different forms) Is this ok?
>
> Yes, v2 patch logs other than T_SQLCmd as a replication command, and
> T_SQLCmd is logged later in exec_simple_query. The
> log_replication_command logs only replication commands[1], it doesn't
> mean to log commands executed on replication connection. I think such
> behavior is right.
>
>> I'm not sure why you have moved the location of the code, but if
>> it should show all received commands, it might be better to be
>> placed before CHECK_FOR_INTERRUPTS..
>
> Hmm, the reason why I've moved it is that we cannot know whether given
> cmd_string is a SQL command or a replication command before parsing.
>
Well the issue is that then the query is not logged if there was parsing
issue even when logging was specifically requested. I don't know what's
good solution here, maybe teaching exec_simple_query to not log the
query if it came from walsender.

BTW reading that function in walsender, there is :
> /*
> * CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
> * command arrives. Clean up the old stuff if there's anything.
> */
> SnapBuildClearExportedSnapshot();

and then

> /*
> * CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot. If it was
> * called outside of transaction the snapshot should be cleared here.
> */
> if (!IsTransactionBlock())
> SnapBuildClearExportedSnapshot();

The first one should not be there, it looks like a result of some merge
conflict being solved wrongly. Maybe your patch could fix that too?

>>>>
>>>>>> 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.
>>>>>
>>>>> From the point of view of time, I agree that it doesn't seem to
>>>>> harm. Bt I thing it as more significant problem that
>>>>> potentially-throwable function is called within the mutex
>>>>> region. It potentially causes a kind of dead lock.  (That said,
>>>>> theoretically dosn't occur and I'm not sure what happens by the
>>>>> dead lock..)
>>>>>
Hmm, I think doing what I attached should be fine here. We don't need to
hold spinlock for table read, just for changing the
MyLogicalRepWorker->relstate.


--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

010_dont-read-catalog-inside-spinlock.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Fujii Masao-2
On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek
<[hidden email]> wrote:

> Thank you for working on this!
>
> On 18/04/17 10:16, Masahiko Sawada wrote:
>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
>> <[hidden email]> wrote:
>>>
>>>>>>> 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.
>>>>>>
>>>>>> IsBackendPid() is not free, I suppose that just ignoring failure
>>>>>> with ESRCH is enough.
>>>>>
>>>>> After thought, since LogicalRepCtx->launcher_pid could be 0 should, we
>>>>> check if launcher_pid != 0?
>>>
>>> Yes. For example, code to send a signal to autovacuum launcher
>>> looks as the follows. I don't think there's a reason to do
>>> different thing here.
>>>
>>> |       avlauncher_needs_signal = false;
>>> |       if (AutoVacPID != 0)
>>> |         kill(AutoVacPID, SIGUSR2);
>>>
>>
>> Fixed.
>
> Yes the IsBackendPid was needed only because we didn't reset
> launcher_pid and it was causing issues otherwise obviously (and I didn't
> realize we are not resetting it...)
>
>>
>>>
>>>>>>> 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.
>>>>>>
>>>>>> The control by log_replication_commands is performed on
>>>>>> walsender, so this also shuld be done on the same place. Anyway
>>>>>> log_statement is irrelevant to walsender.
>>>>>
>>>>> Patch 006 emits all logs executed by the apply worker as a log of
>>>>> log_replication_command but perhaps you're right. It would be better
>>>>> to leave the log of log_statement if the command executed by the apply
>>>>> worker is a SQL command. Will fix.
>>>
>>> Here, we can choose whether a SQL command is a part of
>>> replication commands or not. The previous fix thought it is and
>>> this doesn't. (They are emitted in different forms) Is this ok?
>>
>> Yes, v2 patch logs other than T_SQLCmd as a replication command, and
>> T_SQLCmd is logged later in exec_simple_query. The
>> log_replication_command logs only replication commands[1], it doesn't
>> mean to log commands executed on replication connection. I think such
>> behavior is right.
>>
>>> I'm not sure why you have moved the location of the code, but if
>>> it should show all received commands, it might be better to be
>>> placed before CHECK_FOR_INTERRUPTS..
>>
>> Hmm, the reason why I've moved it is that we cannot know whether given
>> cmd_string is a SQL command or a replication command before parsing.
>>
>
> Well the issue is that then the query is not logged if there was parsing
> issue even when logging was specifically requested. I don't know what's
> good solution here, maybe teaching exec_simple_query to not log the
> query if it came from walsender.
>
> BTW reading that function in walsender, there is :
>>       /*
>>        * CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot until the next
>>        * command arrives. Clean up the old stuff if there's anything.
>>        */
>>       SnapBuildClearExportedSnapshot();
>
> and then
>
>>       /*
>>        * CREATE_REPLICATION_SLOT ... LOGICAL exports a snapshot. If it was
>>        * called outside of transaction the snapshot should be cleared here.
>>        */
>>       if (!IsTransactionBlock())
>>               SnapBuildClearExportedSnapshot();
>
> The first one should not be there, it looks like a result of some merge
> conflict being solved wrongly. Maybe your patch could fix that too?
>
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> From the point of view of time, I agree that it doesn't seem to
>>>>>> harm. Bt I thing it as more significant problem that
>>>>>> potentially-throwable function is called within the mutex
>>>>>> region. It potentially causes a kind of dead lock.  (That said,
>>>>>> theoretically dosn't occur and I'm not sure what happens by the
>>>>>> dead lock..)
>>>>>>
>
> Hmm, I think doing what I attached should be fine here.

Thanks for the patch!

> We don't need to
> hold spinlock for table read, just for changing the
> MyLogicalRepWorker->relstate.

Yes, but the update of MyLogicalRepWorker->relstate_lsn also should
be protected with the spinlock.

Regards,

--
Fujii Masao


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Petr Jelinek-4
On 18/04/17 19:27, Fujii Masao wrote:

> On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek
> <[hidden email]> wrote:
>> Thank you for working on this!
>>
>> On 18/04/17 10:16, Masahiko Sawada wrote:
>>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
>>> <[hidden email]> wrote:
>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> From the point of view of time, I agree that it doesn't seem to
>>>>>>> harm. Bt I thing it as more significant problem that
>>>>>>> potentially-throwable function is called within the mutex
>>>>>>> region. It potentially causes a kind of dead lock.  (That said,
>>>>>>> theoretically dosn't occur and I'm not sure what happens by the
>>>>>>> dead lock..)
>>>>>>>
>>
>> Hmm, I think doing what I attached should be fine here.
>
> Thanks for the patch!
>
>> We don't need to
>> hold spinlock for table read, just for changing the
>> MyLogicalRepWorker->relstate.
>
> Yes, but the update of MyLogicalRepWorker->relstate_lsn also should
> be protected with the spinlock.
>
Yes, sorry tired/blind, fixed.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

010_dont-read-catalog-inside-spinlock-v2.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Kyotaro HORIGUCHI-2
At Wed, 19 Apr 2017 04:18:18 +0200, Petr Jelinek <[hidden email]> wrote in <[hidden email]>

> On 18/04/17 19:27, Fujii Masao wrote:
> > On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek
> > <[hidden email]> wrote:
> >> Thank you for working on this!
> >>
> >> On 18/04/17 10:16, Masahiko Sawada wrote:
> >>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
> >>> <[hidden email]> wrote:
> >>>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> From the point of view of time, I agree that it doesn't seem to
> >>>>>>> harm. Bt I thing it as more significant problem that
> >>>>>>> potentially-throwable function is called within the mutex
> >>>>>>> region. It potentially causes a kind of dead lock.  (That said,
> >>>>>>> theoretically dosn't occur and I'm not sure what happens by the
> >>>>>>> dead lock..)
> >>>>>>>
> >>
> >> Hmm, I think doing what I attached should be fine here.
> >
> > Thanks for the patch!
> >
> >> We don't need to
> >> hold spinlock for table read, just for changing the
> >> MyLogicalRepWorker->relstate.
> >
> > Yes, but the update of MyLogicalRepWorker->relstate_lsn also should
> > be protected with the spinlock.
> >
>
> Yes, sorry tired/blind, fixed.

Commit has been moved from after to before of the lock section.
This causes potential race condition. (As the same as the
potential dead-lock, I'm not sure it can cause realistic problem,
though..) Isn't it better to be after the lock section?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Petr Jelinek-4
On 19/04/17 10:25, Kyotaro HORIGUCHI wrote:

> At Wed, 19 Apr 2017 04:18:18 +0200, Petr Jelinek <[hidden email]> wrote in <[hidden email]>
>> On 18/04/17 19:27, Fujii Masao wrote:
>>> On Wed, Apr 19, 2017 at 1:35 AM, Petr Jelinek
>>> <[hidden email]> wrote:
>>>> Thank you for working on this!
>>>>
>>>> On 18/04/17 10:16, Masahiko Sawada wrote:
>>>>> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
>>>>> <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> From the point of view of time, I agree that it doesn't seem to
>>>>>>>>> harm. Bt I thing it as more significant problem that
>>>>>>>>> potentially-throwable function is called within the mutex
>>>>>>>>> region. It potentially causes a kind of dead lock.  (That said,
>>>>>>>>> theoretically dosn't occur and I'm not sure what happens by the
>>>>>>>>> dead lock..)
>>>>>>>>>
>>>>
>>>> Hmm, I think doing what I attached should be fine here.
>>>
>>> Thanks for the patch!
>>>
>>>> We don't need to
>>>> hold spinlock for table read, just for changing the
>>>> MyLogicalRepWorker->relstate.
>>>
>>> Yes, but the update of MyLogicalRepWorker->relstate_lsn also should
>>> be protected with the spinlock.
>>>
>>
>> Yes, sorry tired/blind, fixed.
>
> Commit has been moved from after to before of the lock section.
> This causes potential race condition. (As the same as the
> potential dead-lock, I'm not sure it can cause realistic problem,
> though..) Isn't it better to be after the lock section?
>

We just read catalogs so there should not be locking issues.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Kyotaro HORIGUCHI-2
At Wed, 19 Apr 2017 10:33:29 +0200, Petr Jelinek <[hidden email]> wrote in <[hidden email]>
> > Commit has been moved from after to before of the lock section.
> > This causes potential race condition. (As the same as the
> > potential dead-lock, I'm not sure it can cause realistic problem,
> > though..) Isn't it better to be after the lock section?
> >
>
> We just read catalogs so there should not be locking issues.

Some other process may modify it then go to there. With a kind of
priority inversion, the process may modify the data on the memory
*before* we modify it. Of course this is rather unrealistic,
though.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Kyotaro HORIGUCHI-2
At Wed, 19 Apr 2017 17:43:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[hidden email]> wrote in <[hidden email]>

> At Wed, 19 Apr 2017 10:33:29 +0200, Petr Jelinek <[hidden email]> wrote in <[hidden email]>
> > > Commit has been moved from after to before of the lock section.
> > > This causes potential race condition. (As the same as the
> > > potential dead-lock, I'm not sure it can cause realistic problem,
> > > though..) Isn't it better to be after the lock section?
> > >
> >
> > We just read catalogs so there should not be locking issues.
>
> Some other process may modify it then go to there. With a kind of
> priority inversion, the process may modify the data on the memory
> *before* we modify it. Of course this is rather unrealistic,
> though.

A bit short.

Some other process may modify it *after* we read it then go to
there. With a kind of priority inversion, the process may modify
the data on the memory *before* we modify it. Of course this is
rather unrealistic, though.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Petr Jelinek-4
On 19/04/17 10:45, Kyotaro HORIGUCHI wrote:

> At Wed, 19 Apr 2017 17:43:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[hidden email]> wrote in <[hidden email]>
>> At Wed, 19 Apr 2017 10:33:29 +0200, Petr Jelinek <[hidden email]> wrote in <[hidden email]>
>>>> Commit has been moved from after to before of the lock section.
>>>> This causes potential race condition. (As the same as the
>>>> potential dead-lock, I'm not sure it can cause realistic problem,
>>>> though..) Isn't it better to be after the lock section?
>>>>
>>>
>>> We just read catalogs so there should not be locking issues.
>>
>> Some other process may modify it then go to there. With a kind of
>> priority inversion, the process may modify the data on the memory
>> *before* we modify it. Of course this is rather unrealistic,
>> though.
>
> A bit short.
>
> Some other process may modify it *after* we read it then go to
> there. With a kind of priority inversion, the process may modify
> the data on the memory *before* we modify it. Of course this is
> rather unrealistic, though.
>

Yeah but I think that's risk anyway in MVCC read committed database, the
only way to protect against that would be to hold lock over the catalogs
access which was what we wanted to get rid of :)

BTW the whole sync coordination is explicitly written in a way that
unless you mess with catalogs manually only the tablesync process should
do UPDATEs to that catalog (with the exception of s->r state switch
which can happen in apply and has no effect on sync because both states
mean that synchronization is done, only makes apply stop tracking the
table individually).

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Noah Misch-2
In reply to this post by Noah Misch-2
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Fujii Masao-2
In reply to this post by Masahiko Sawada
On Tue, Apr 18, 2017 at 5:16 PM, Masahiko Sawada <[hidden email]> wrote:

> On Tue, Apr 18, 2017 at 12:24 PM, Kyotaro HORIGUCHI
> <[hidden email]> wrote:
>> Hi,
>>
>> Thank you for the revised version.
>>
>> At Mon, 17 Apr 2017 23:29:28 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoA+yFJTbw0PCw-ttmh9TsTygM3=[hidden email]>
>>> On Mon, Apr 17, 2017 at 9:13 PM, Masahiko Sawada <[hidden email]> wrote:
>>> > On Mon, Apr 17, 2017 at 7:39 PM, Kyotaro HORIGUCHI
>>> > <[hidden email]> wrote:
>>> >> At Mon, 17 Apr 2017 18:02:57 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoB3L50jWwuyaGLxJzmrmXmYJAOBHzDPR=[hidden email]>
>>> >>> On Fri, Apr 14, 2017 at 4:47 AM, Fujii Masao <[hidden email]> wrote:
>>> >>> 1.
>>> >>> >
>>> >>> > In ApplyWorkerMain(), DatumGetInt32() should be used to get integer
>>> >>> > value from the argument, instead of DatumGetObjectId().
>>> >>>
>>> >>> Attached 001 patch fixes it.
>>> >>
>>> >> Hmm. I looked at the launcher side and found that it assigns bare
>>> >> integer to bgw_main_arg. It also should use Int32GetDatum.
>>> >
>>> > Yeah, I agreed. Will fix it.
>>
>> Thanks.
>>
>>> >>> 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.
>>> >>
>>> >> It is omitting the abort case. Maybe it should be
>>> >> AtEOXact_ApplyLauncher(bool commit)?
>>> >
>>> > Should we wake up the launcher process even when abort?
>>
>> No, I meant that on_commit_launcher_wakeup should be just reset
>> without waking up launcher on abort.
>
> I understood. Sounds reasonable.

ROLLBACK PREPARED should not wake up the launcher, but not even with the patch.
To fix this issue, we should call AtEOXact_ApplyLauncher() in
FinishPreparedTransaction() or somewhere?

Regards,

--
Fujii Masao


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Fujii Masao-2
In reply to this post by Noah Misch-2
On Thu, Apr 20, 2017 at 12:05 PM, Noah Misch <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: some review comments on logical rep code

Peter Eisentraut-6
On 4/20/17 12:30, Fujii Masao wrote:
> 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)?

Thank you for pitching in.

Where is your latest patch?

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
123
Previous Thread Next Thread
Loading...