Re: [HACKERS] logical replication and SIGHUP

[Peter Eisentraut]

On 4/9/17 23:20, Michael Paquier wrote:
> After more review, I think that got_SIGTERM should be of type volatile
> sig_atomic_t in launcher.c or that's not signal-safe. I think as well
> that for correctness errno should be saved as SetLatch() is called and
> restored afterwards. Please find attached a patch to address all that.

committed

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


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

Re: [HACKERS] logical replication and SIGHUP

[Tom Lane]

Petr Jelinek  writes:
> On 10/04/17 14:40, Tom Lane wrote:
>> Umm ... you're doing what?

> We are doing:
> +   SetConfigOption("synchronous_commit",
> +   MySubscription->synccommit ?
> "local" : "off",
> +   PGC_BACKEND, PGC_S_OVERRIDE);

That looks fine.

> I don't remember from top of my head if above is safe enough against
> config reread or not, hence the note.

Yes, the override will take care of it ...

regards, tom lane


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

Re: [HACKERS] logical replication and SIGHUP

[Petr Jelinek]

On 10/04/17 14:40, Tom Lane wrote:
> Petr Jelinek  writes:
>> Looks good to me. Just as a note, we'll have to handle this newly
>> supported config rereads in the async commit patch where we override
>> synchronous_commit GUC, but the config reread will change it back.
> 
> Umm ... you're doing what?
> 

We are doing:
+   SetConfigOption("synchronous_commit",
+   MySubscription->synccommit ?
"local" : "off",
+   PGC_BACKEND, PGC_S_OVERRIDE);

> There are mechanisms for making local changes to a GUC.  Just stomping
> on the variable is not an approved way to do it.
> 

I don't remember from top of my head if above is safe enough against
config reread or not, hence the note.

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


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

Re: [HACKERS] logical replication and SIGHUP

[Tom Lane]

Petr Jelinek  writes:
> Looks good to me. Just as a note, we'll have to handle this newly
> supported config rereads in the async commit patch where we override
> synchronous_commit GUC, but the config reread will change it back.

Umm ... you're doing what?

There are mechanisms for making local changes to a GUC.  Just stomping
on the variable is not an approved way to do it.

regards, tom lane


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

Re: [HACKERS] logical replication and SIGHUP

[Petr Jelinek]

On 10/04/17 05:20, Michael Paquier wrote:
> On Mon, Apr 10, 2017 at 11:41 AM, Noah Misch  wrote:
>> On Thu, Apr 06, 2017 at 02:21:29AM +0900, Fujii Masao wrote:
>>> Both launcher and worker don't handle SIGHUP signal and cannot
>>> reload the configuration. I think that this is a bug. Will add this as
>>> an open item barring objection.
>>
>> [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
> 
> After more review, I think that got_SIGTERM should be of type volatile
> sig_atomic_t in launcher.c or that's not signal-safe. I think as well
> that for correctness errno should be saved as SetLatch() is called and
> restored afterwards. Please find attached a patch to address all that.
> 

Looks good to me. Just as a note, we'll have to handle this newly
supported config rereads in the async commit patch where we override
synchronous_commit GUC, but the config reread will change it back.

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


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

Re: [HACKERS] logical replication and SIGHUP

[Michael Paquier]

On Mon, Apr 10, 2017 at 11:41 AM, Noah Misch  wrote:
> On Thu, Apr 06, 2017 at 02:21:29AM +0900, Fujii Masao wrote:
>> Both launcher and worker don't handle SIGHUP signal and cannot
>> reload the configuration. I think that this is a bug. Will add this as
>> an open item barring objection.
>
> [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

After more review, I think that got_SIGTERM should be of type volatile
sig_atomic_t in launcher.c or that's not signal-safe. I think as well
that for correctness errno should be saved as SetLatch() is called and
restored afterwards. Please find attached a patch to address all that.
-- 
Michael
VMware vCenter Server
www.vmware.com


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

Re: [HACKERS] logical replication and SIGHUP

[Noah Misch]

On Thu, Apr 06, 2017 at 02:21:29AM +0900, Fujii Masao wrote:
> Both launcher and worker don't handle SIGHUP signal and cannot
> reload the configuration. I think that this is a bug. Will add this as
> an open item barring objection.

[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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] logical replication and SIGHUP

[Peter Geoghegan]

On Wed, Apr 5, 2017 at 10:21 AM, Fujii Masao  wrote:
> Both launcher and worker don't handle SIGHUP signal and cannot
> reload the configuration. I think that this is a bug.

+1


-- 
Peter Geoghegan


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