Thread: Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Fujii Masao
Date:
On Mon, Mar 7, 2011 at 5:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Mar 7, 2011 at 7:51 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Efficient transaction-controlled synchronous replication.
>> If a standby is broadcasting reply messages and we have named
>> one or more standbys in synchronous_standby_names then allow
>> users who set synchronous_replication to wait for commit, which
>> then provides strict data integrity guarantees. Design avoids
>> sending and receiving transaction state information so minimises
>> bookkeeping overheads. We synchronize with the highest priority
>> standby that is connected and ready to synchronize. Other standbys
>> can be defined to takeover in case of standby failure.
>>
>> This version has very strict behaviour; more relaxed options
>> may be added at a later date.
>
> Pretty cool! I'd appreciate very much your efforts and contributions.
>
> And,, I found one bug ;) You seem to have wrongly removed the check
> of max_wal_senders in SyncRepWaitForLSN. This can make the
> backend wait for replication even if max_wal_senders = 0. I could produce
> this problematic situation in my machine. The attached patch fixes this problem.
if (strlen(SyncRepStandbyNames) > 0 && max_wal_senders == 0) ereport(ERROR, (errmsg("Synchronous
replicationrequires WAL streaming
(max_wal_senders > 0)")));
The above check should be required also after pg_ctl reload since
synchronous_standby_names can be changed by SIGHUP?
Or how about just removing that? If the patch I submitted is
committed,empty synchronous_standby_names and max_wal_senders = 0
settings is no longer unsafe.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Simon Riggs
Date:
On Mon, 2011-03-07 at 17:44 +0900, Fujii Masao wrote: > The above check should be required also after pg_ctl reload since > synchronous_standby_names can be changed by SIGHUP? > Or how about just removing that? If the patch I submitted is > committed,empty synchronous_standby_names and max_wal_senders = 0 > settings is no longer unsafe. Ah, on reload. I plugged the gap only at startup. I'll fix by changing assign_synchronous_standby_names(), not by changing lots of other parts of code and making runtime check each COMMIT. -- Simon Riggs http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Fujii Masao
Date:
On Mon, Mar 7, 2011 at 6:30 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Mon, 2011-03-07 at 17:44 +0900, Fujii Masao wrote: > >> The above check should be required also after pg_ctl reload since >> synchronous_standby_names can be changed by SIGHUP? >> Or how about just removing that? If the patch I submitted is >> committed,empty synchronous_standby_names and max_wal_senders = 0 >> settings is no longer unsafe. > > Ah, on reload. I plugged the gap only at startup. > > I'll fix by changing assign_synchronous_standby_names(), not by changing > lots of other parts of code and making runtime check each COMMIT. I don't think that the check of local variable for each COMMIT wastes the cycle so much. Anyway, the reload of the configuration file should not cause the server to end unexpectedly. IOW, GUC assign hook should use GUC_complaint_elevel instead of FATAL, in ereport. The attached patch fixes that, and includes two typo fixes. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Attachment
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Robert Haas
Date:
On Mon, Mar 7, 2011 at 4:47 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > Anyway, the reload of the configuration file should not > cause the server to end unexpectedly. IOW, GUC assign hook should > use GUC_complaint_elevel instead of FATAL, in ereport. The attached > patch fixes that, and includes two typo fixes. Committed the typo fixes, and the GUC assign hook fix as a separate commit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Robert Haas
Date:
On Mon, Mar 7, 2011 at 3:44 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Mar 7, 2011 at 5:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Mon, Mar 7, 2011 at 7:51 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> Efficient transaction-controlled synchronous replication.
>>> If a standby is broadcasting reply messages and we have named
>>> one or more standbys in synchronous_standby_names then allow
>>> users who set synchronous_replication to wait for commit, which
>>> then provides strict data integrity guarantees. Design avoids
>>> sending and receiving transaction state information so minimises
>>> bookkeeping overheads. We synchronize with the highest priority
>>> standby that is connected and ready to synchronize. Other standbys
>>> can be defined to takeover in case of standby failure.
>>>
>>> This version has very strict behaviour; more relaxed options
>>> may be added at a later date.
>>
>> Pretty cool! I'd appreciate very much your efforts and contributions.
>>
>> And,, I found one bug ;) You seem to have wrongly removed the check
>> of max_wal_senders in SyncRepWaitForLSN. This can make the
>> backend wait for replication even if max_wal_senders = 0. I could produce
>> this problematic situation in my machine. The attached patch fixes this problem.
>
> if (strlen(SyncRepStandbyNames) > 0 && max_wal_senders == 0)
> ereport(ERROR,
> (errmsg("Synchronous replication requires WAL streaming
> (max_wal_senders > 0)")));
>
> The above check should be required also after pg_ctl reload since
> synchronous_standby_names can be changed by SIGHUP?
> Or how about just removing that? If the patch I submitted is
> committed,empty synchronous_standby_names and max_wal_senders = 0
> settings is no longer unsafe.
This configuration is now harmless in the sense that it no longer
horribly breaks the entire system, but it's still pretty useless, so
this might be deemed a valuable sanity check. However, I'm reluctant
to leave it in there, because someone could change their config to
this state, pg_ctl reload, see everything working, and then later stop
the cluster and be unable to start it back up again. Since most
people don't shut their database systems down very often, they might
not discover that they have an invalid config until much later. I
think it's probably not a good idea to have configs that are valid on
reload but prevent startup, so I'm inclined to either remove this
check altogether or downgrade it to a warning.
As a side note, it's not very obvious why some parts of PostmasterMain
report problems by doing write_stderr() and exit() while other parts
use ereport(ERROR). This check and the nearby checks on WAL level are
immediately preceded and followed by other checks that use the
opposite technique.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> As a side note, it's not very obvious why some parts of PostmasterMain
> report problems by doing write_stderr() and exit() while other parts
> use ereport(ERROR). This check and the nearby checks on WAL level are
> immediately preceded and followed by other checks that use the
> opposite technique.
This question is answered in postmaster.c's header comment:
* Error Reporting:* Use write_stderr() only for reporting "interactive" errors* (essentially, bogus
argumentson the command line). Once the* postmaster is launched, use ereport(). In particular, don't use*
write_stderr() for anything that occurs after pmdaemonize.
Code that is involved in GUC variable processing is in a gray area, though,
since it can be invoked both before and after pmdaemonize. It might be
a good idea to convert all the calls into ereports and maintain a state
flag in elog.c to determine what to do.
regards, tom lane
Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
From
Robert Haas
Date:
On Fri, Mar 18, 2011 at 10:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Mar 7, 2011 at 3:44 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Mon, Mar 7, 2011 at 5:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Mon, Mar 7, 2011 at 7:51 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>> Efficient transaction-controlled synchronous replication.
>>>> If a standby is broadcasting reply messages and we have named
>>>> one or more standbys in synchronous_standby_names then allow
>>>> users who set synchronous_replication to wait for commit, which
>>>> then provides strict data integrity guarantees. Design avoids
>>>> sending and receiving transaction state information so minimises
>>>> bookkeeping overheads. We synchronize with the highest priority
>>>> standby that is connected and ready to synchronize. Other standbys
>>>> can be defined to takeover in case of standby failure.
>>>>
>>>> This version has very strict behaviour; more relaxed options
>>>> may be added at a later date.
>>>
>>> Pretty cool! I'd appreciate very much your efforts and contributions.
>>>
>>> And,, I found one bug ;) You seem to have wrongly removed the check
>>> of max_wal_senders in SyncRepWaitForLSN. This can make the
>>> backend wait for replication even if max_wal_senders = 0. I could produce
>>> this problematic situation in my machine. The attached patch fixes this problem.
>>
>> if (strlen(SyncRepStandbyNames) > 0 && max_wal_senders == 0)
>> ereport(ERROR,
>> (errmsg("Synchronous replication requires WAL streaming
>> (max_wal_senders > 0)")));
>>
>> The above check should be required also after pg_ctl reload since
>> synchronous_standby_names can be changed by SIGHUP?
>> Or how about just removing that? If the patch I submitted is
>> committed,empty synchronous_standby_names and max_wal_senders = 0
>> settings is no longer unsafe.
>
> This configuration is now harmless in the sense that it no longer
> horribly breaks the entire system, but it's still pretty useless, so
> this might be deemed a valuable sanity check. However, I'm reluctant
> to leave it in there, because someone could change their config to
> this state, pg_ctl reload, see everything working, and then later stop
> the cluster and be unable to start it back up again. Since most
> people don't shut their database systems down very often, they might
> not discover that they have an invalid config until much later. I
> think it's probably not a good idea to have configs that are valid on
> reload but prevent startup, so I'm inclined to either remove this
> check altogether or downgrade it to a warning.
Done.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company