synchronous_commit and synchronous_replication Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.
On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark <gsstark@mit.edu> wrote:
On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
What makes more sense to me after having thought about this more
carefully is to simply make a blanket rule that when
synchronous_replication=on, synchronous_commit has no effect. That is
easy to understand and document.For what it's worth "has no effect" doesn't make much sense to me.
It's a boolean, either commits are going to block or they're not.What happened to the idea of a three-way switch?
synchronous_commit = off
synchronous_commit = disk
synchronous_commit = replicaWith "on" being a synonym for "disk" for backwards compatibility.
Then we could add more options later for more complex conditions like
waiting for one server in each data centre or waiting for one of a
certain set of servers ignoring the less reliable mirrors, etc.This is similar to what I suggested upthread, except that I suggested
on/local/off, with the default being on. That way if you set
synchronous_standby_names, you get synchronous replication without
changing another setting, but you can say local instead if for some
reason you want the middle behavior. If we're going to do it all with
one GUC, I think that way makes more sense. If you're running sync
rep, you might still have some transactions that you don't care about,
but that's what async commit is for. It's a funny kind of transaction
that we're OK with losing if we have a failover but we're not OK with
losing if we have a local crash from which we recover without failing
over.I'm OK with this.
The attached patch merges synchronous_replication into synchronous_commit.
With the patch, valid values of synchronous_commit are "on" (waits for local
flush and sync rep), "off" (waits for neither local flush nor sync
rep), and "local"
(waits for only local flush).
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
merge_gucs_v1.patchapplication/octet-stream; name=merge_gucs_v1.patchDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 1505,1514 **** SET ENABLE_SEQSCAN TO OFF;
</indexterm>
<listitem>
<para>
! Specifies whether transaction commit will wait for WAL records
! to be written to disk before the command returns a <quote>success</>
! indication to the client. The default, and safe, setting is
! <literal>on</>. When <literal>off</>, there can be a delay between
when success is reported to the client and when the transaction is
really guaranteed to be safe against a server crash. (The maximum
delay is three times <xref linkend="guc-wal-writer-delay">.) Unlike
--- 1505,1527 ----
</indexterm>
<listitem>
<para>
! Specifies the synchronization level for transaction commit, defining
! what actions must take place before the command returns a <quote>success</>
! indication to the client. Valid values are <literal>on</>, <literal>off</>,
! and <literal>local</>. The default, and safe, value is <literal>on</>,
! which waits for the transaction's WAL records to be flushed to disk and
! replicated to the standby server. When <literal>on</>, there will be a
! delay while the client waits for confirmation of successful replication.
! That delay will increase depending upon the physical distance and network
! activity between primary and standby. The commit wait will last until
! a reply from the current synchronous standby indicates it has written
! the commit record of the transaction to durable storage.
! <literal>local</> waits for local flush to disk, but not synchronous
! replication. The synchronization level of <literal>on</> is the same as
! that of <literal>local</> if <xref linkend="guc-synchronous-standby-names">
! is empty or <xref linkend="guc-max-wal-senders"> is zero. <literal>off</>
! waits for neither local flush to disk nor synchronous replication.
! When <literal>off</>, there can be a delay between
when success is reported to the client and when the transaction is
really guaranteed to be safe against a server crash. (The maximum
delay is three times <xref linkend="guc-wal-writer-delay">.) Unlike
***************
*** 2057,2095 **** SET ENABLE_SEQSCAN TO OFF;
</para>
<variablelist>
- <varlistentry id="guc-synchronous-replication" xreflabel="synchronous_replication">
- <term><varname>synchronous_replication</varname> (<type>boolean</type>)</term>
- <indexterm>
- <primary><varname>synchronous_replication</> configuration parameter</primary>
- </indexterm>
- <listitem>
- <para>
- Specifies whether transaction commit will wait for WAL records
- to be replicated before the command returns a <quote>success</>
- indication to the client. The default setting is <literal>off</>.
- When <literal>on</>, there will be a delay while the client waits
- for confirmation of successful replication. That delay will
- increase depending upon the physical distance and network activity
- between primary and standby. The commit wait will last until a
- reply from the current synchronous standby indicates it has written
- the commit record of the transaction to durable storage. This
- parameter has no effect if
- <xref linkend="guc-synchronous-standby-names"> is empty or
- <xref linkend="guc-max-wal-senders"> is zero.
- </para>
- <para>
- This parameter can be changed at any time; the
- behavior for any one transaction is determined by the setting in
- effect when it commits. It is therefore possible, and useful, to have
- some transactions replicate synchronously and others asynchronously.
- For example, to make a single multistatement transaction commit
- asynchronously when the default is synchronous replication, issue
- <command>SET LOCAL synchronous_replication TO OFF</> within the
- transaction.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="guc-synchronous-standby-names" xreflabel="synchronous_standby_names">
<term><varname>synchronous_standby_names</varname> (<type>string</type>)</term>
<indexterm>
--- 2070,2075 ----
***************
*** 2124,2130 **** SET ENABLE_SEQSCAN TO OFF;
If a standby is removed from the list of servers then it will stop
being the synchronous standby, allowing another to take its place.
If the list is empty, synchronous replication will not be
! possible, whatever the setting of <varname>synchronous_replication</>.
Standbys may also be added to the list without restarting the server.
</para>
</listitem>
--- 2104,2110 ----
If a standby is removed from the list of servers then it will stop
being the synchronous standby, allowing another to take its place.
If the list is empty, synchronous replication will not be
! possible, whatever the setting of <varname>synchronous_commit</>.
Standbys may also be added to the list without restarting the server.
</para>
</listitem>
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***************
*** 933,945 **** primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
synchronous replication easily just by setting this on the primary:
<programlisting>
! synchronous_replication = on
</programlisting>
! When <varname>synchronous_replication</> is set, a commit will wait
! for confirmation that the standby has received the commit record,
! even if that takes a very long time.
! <varname>synchronous_replication</> can be set by individual
users, so can be configured in the configuration file, for particular
users or databases, or dynamically by applications.
</para>
--- 933,945 ----
synchronous replication easily just by setting this on the primary:
<programlisting>
! synchronous_commit = on
</programlisting>
! When <varname>synchronous_commit</> is set to <literal>on</>,
! a commit will wait for confirmation that the standby has received
! the commit record, even if that takes a very long time.
! <varname>synchronous_commit</> can be set by individual
users, so can be configured in the configuration file, for particular
users or databases, or dynamically by applications.
</para>
***************
*** 964,977 **** synchronous_replication = on
transferred to standby servers.
</para>
- <para>
- Note also that <varname>synchronous_commit</> is used when the user
- specifies <varname>synchronous_replication</>, overriding even an
- explicit setting of <varname>synchronous_commit</> to <literal>off</>.
- This is because we must write WAL to disk on primary before we replicate
- to ensure the standby never gets ahead of the primary.
- </para>
-
</sect3>
<sect3 id="synchronous-replication-performance">
--- 964,969 ----
***************
*** 1019,1027 **** synchronous_replication = on
<title>Planning for High Availability</title>
<para>
! Commits made when synchronous_replication is set will wait until
! the sync standby responds. The response may never occur if the last,
! or only, standby should crash.
</para>
<para>
--- 1011,1019 ----
<title>Planning for High Availability</title>
<para>
! Commits made when <varname>synchronous_commit</> is set to <literal>on</>
! will wait until the sync standby responds. The response may never occur
! if the last, or only, standby should crash.
</para>
<para>
***************
*** 1073,1080 **** synchronous_replication = on
If you need to re-create a standby server while transactions are
waiting, make sure that the commands to run pg_start_backup() and
pg_stop_backup() are run in a session with
! synchronous_replication = off, otherwise those requests will wait
! forever for the standby to appear.
</para>
</sect3>
--- 1065,1072 ----
If you need to re-create a standby server while transactions are
waiting, make sure that the commands to run pg_start_backup() and
pg_stop_backup() are run in a session with
! <varname>synchronous_commit</> = <literal>off</>, otherwise those
! requests will wait forever for the standby to appear.
</para>
</sect3>
*** a/doc/src/sgml/release-9.1.sgml
--- b/doc/src/sgml/release-9.1.sgml
***************
*** 656,662 ****
<link linkend="guc-synchronous-standby-names"><varname>synchronous_standby_names</varname></link>
setting. Synchronous replication can be enabled or disabled on a
per-transaction basis using the
! <link linkend="guc-synchronous-replication"><varname>synchronous_replication</></link>
setting. This allows the primary to wait for a standby to write the
transaction information to disk before acknowledging the commit.
</para>
--- 656,662 ----
<link linkend="guc-synchronous-standby-names"><varname>synchronous_standby_names</varname></link>
setting. Synchronous replication can be enabled or disabled on a
per-transaction basis using the
! <link linkend="guc-synchronous-commit"><varname>synchronous_commit</></link>
setting. This allows the primary to wait for a standby to write the
transaction information to disk before acknowledging the commit.
</para>
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***************
*** 68,74 **** bool XactReadOnly;
bool DefaultXactDeferrable = false;
bool XactDeferrable;
! bool XactSyncCommit = true;
int CommitDelay = 0; /* precommit delay in microseconds */
int CommitSiblings = 5; /* # concurrent xacts needed to sleep */
--- 68,74 ----
bool DefaultXactDeferrable = false;
bool XactDeferrable;
! int XactSyncCommit = SYNCHRONOUS_COMMIT_ON;
int CommitDelay = 0; /* precommit delay in microseconds */
int CommitSiblings = 5; /* # concurrent xacts needed to sleep */
***************
*** 1056,1062 **** RecordTransactionCommit(void)
* if all to-be-deleted tables are temporary though, since they are lost
* anyway if we crash.)
*/
! if ((wrote_xlog && XactSyncCommit) || forceSyncCommit || nrels > 0 || SyncRepRequested())
{
/*
* Synchronous commit case:
--- 1056,1063 ----
* if all to-be-deleted tables are temporary though, since they are lost
* anyway if we crash.)
*/
! if ((wrote_xlog && XactSyncCommit >= SYNCHRONOUS_COMMIT_ON) ||
! forceSyncCommit || nrels > 0)
{
/*
* Synchronous commit case:
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
***************
*** 1531,1537 **** AutoVacWorkerMain(int argc, char *argv[])
* if we are waiting for standbys to connect. This is important to
* ensure we aren't blocked from performing anti-wraparound tasks.
*/
! SetConfigOption("synchronous_replication", "off", PGC_SUSET, PGC_S_OVERRIDE);
/*
* Get the info about the database we're going to work on.
--- 1531,1538 ----
* if we are waiting for standbys to connect. This is important to
* ensure we aren't blocked from performing anti-wraparound tasks.
*/
! if (XactSyncCommit == SYNCHRONOUS_COMMIT_ON)
! SetConfigOption("synchronous_commit", "local", PGC_SUSET, PGC_S_OVERRIDE);
/*
* Get the info about the database we're going to work on.
*** a/src/backend/replication/syncrep.c
--- b/src/backend/replication/syncrep.c
***************
*** 63,69 ****
#include "utils/ps_status.h"
/* User-settable parameters for sync rep */
- bool synchronous_replication = false; /* Only set in user backends */
char *SyncRepStandbyNames;
#define SyncStandbysDefined() \
--- 63,68 ----
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 351,356 **** static const struct config_enum_entry constraint_exclusion_options[] = {
--- 351,373 ----
};
/*
+ * Although only "on", "off", and "local" are documented, we
+ * accept all the likely variants of "on" and "off".
+ */
+ static const struct config_enum_entry synchronous_commit_options[] = {
+ {"local", SYNCHRONOUS_COMMIT_LOCAL, false},
+ {"on", SYNCHRONOUS_COMMIT_ON, false},
+ {"off", SYNCHRONOUS_COMMIT_OFF, false},
+ {"true", SYNCHRONOUS_COMMIT_ON, true},
+ {"false", SYNCHRONOUS_COMMIT_OFF, true},
+ {"yes", SYNCHRONOUS_COMMIT_ON, true},
+ {"no", SYNCHRONOUS_COMMIT_OFF, true},
+ {"1", SYNCHRONOUS_COMMIT_ON, true},
+ {"0", SYNCHRONOUS_COMMIT_OFF, true},
+ {NULL, 0, false}
+ };
+
+ /*
* Options for enum values stored in other modules
*/
extern const struct config_enum_entry wal_level_options[];
***************
*** 747,768 **** static struct config_bool ConfigureNamesBool[] =
true, NULL, NULL
},
{
- {"synchronous_commit", PGC_USERSET, WAL_SETTINGS,
- gettext_noop("Sets immediate fsync at commit."),
- NULL
- },
- &XactSyncCommit,
- true, NULL, NULL
- },
- {
- {"synchronous_replication", PGC_USERSET, WAL_REPLICATION,
- gettext_noop("Requests synchronous replication."),
- NULL
- },
- &synchronous_replication,
- false, NULL, NULL
- },
- {
{"zero_damaged_pages", PGC_SUSET, DEVELOPER_OPTIONS,
gettext_noop("Continues processing past damaged page headers."),
gettext_noop("Detection of a damaged page header normally causes PostgreSQL to "
--- 764,769 ----
***************
*** 2909,2914 **** static struct config_enum ConfigureNamesEnum[] =
--- 2910,2925 ----
},
{
+ {"synchronous_commit", PGC_USERSET, WAL_SETTINGS,
+ gettext_noop("Sets the current transaction's synchronization level."),
+ NULL
+ },
+ &XactSyncCommit,
+ SYNCHRONOUS_COMMIT_ON, synchronous_commit_options,
+ NULL, NULL
+ },
+
+ {
{"trace_recovery_messages", PGC_SIGHUP, DEVELOPER_OPTIONS,
gettext_noop("Enables logging of recovery-related debugging information."),
gettext_noop("Each level includes all the levels that follow it. The later"
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 153,159 ****
#wal_level = minimal # minimal, archive, or hot_standby
# (change requires restart)
#fsync = on # turns forced synchronization on or off
! #synchronous_commit = on # immediate fsync at commit
#wal_sync_method = fsync # the default is the first option
# supported by the operating system:
# open_datasync
--- 153,159 ----
#wal_level = minimal # minimal, archive, or hot_standby
# (change requires restart)
#fsync = on # turns forced synchronization on or off
! #synchronous_commit = on # synchronization level; on, off, or local
#wal_sync_method = fsync # the default is the first option
# supported by the operating system:
# open_datasync
***************
*** 184,193 ****
#archive_timeout = 0 # force a logfile segment switch after this
# number of seconds; 0 disables
- # - Replication - User Settings
-
- #synchronous_replication = off # does commit wait for reply from standby
-
# - Streaming Replication - Server Settings
#synchronous_standby_names = '' # standby servers that provide sync rep
--- 184,189 ----
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
***************
*** 52,59 **** extern bool XactReadOnly;
extern bool DefaultXactDeferrable;
extern bool XactDeferrable;
! /* Asynchronous commits */
! extern bool XactSyncCommit;
/* Kluge for 2PC support */
extern bool MyXactAccessedTempRel;
--- 52,66 ----
extern bool DefaultXactDeferrable;
extern bool XactDeferrable;
! /* Synchronous commit level */
! extern int XactSyncCommit;
!
! typedef enum
! {
! SYNCHRONOUS_COMMIT_OFF, /* asynchronous commit */
! SYNCHRONOUS_COMMIT_ON, /* wait for local flush and sync rep */
! SYNCHRONOUS_COMMIT_LOCAL /* wait for only local flush */
! } SyncCommitLevel;
/* Kluge for 2PC support */
extern bool MyXactAccessedTempRel;
*** a/src/include/replication/syncrep.h
--- b/src/include/replication/syncrep.h
***************
*** 20,26 ****
#include "utils/guc.h"
#define SyncRepRequested() \
! (synchronous_replication && max_wal_senders > 0)
/* syncRepState */
#define SYNC_REP_NOT_WAITING 0
--- 20,26 ----
#include "utils/guc.h"
#define SyncRepRequested() \
! (max_wal_senders > 0 && XactSyncCommit == SYNCHRONOUS_COMMIT_ON)
/* syncRepState */
#define SYNC_REP_NOT_WAITING 0
***************
*** 28,34 ****
#define SYNC_REP_WAIT_COMPLETE 2
/* user-settable parameters for synchronous replication */
- extern bool synchronous_replication;
extern char *SyncRepStandbyNames;
/* called by user backend */
--- 28,33 ----
On Mon, Apr 4, 2011 at 4:54 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark <gsstark@mit.edu> wrote:
On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
What makes more sense to me after having thought about this more
carefully is to simply make a blanket rule that when
synchronous_replication=on, synchronous_commit has no effect. That is
easy to understand and document.For what it's worth "has no effect" doesn't make much sense to me.
It's a boolean, either commits are going to block or they're not.What happened to the idea of a three-way switch?
synchronous_commit = off
synchronous_commit = disk
synchronous_commit = replicaWith "on" being a synonym for "disk" for backwards compatibility.
Then we could add more options later for more complex conditions like
waiting for one server in each data centre or waiting for one of a
certain set of servers ignoring the less reliable mirrors, etc.This is similar to what I suggested upthread, except that I suggested
on/local/off, with the default being on. That way if you set
synchronous_standby_names, you get synchronous replication without
changing another setting, but you can say local instead if for some
reason you want the middle behavior. If we're going to do it all with
one GUC, I think that way makes more sense. If you're running sync
rep, you might still have some transactions that you don't care about,
but that's what async commit is for. It's a funny kind of transaction
that we're OK with losing if we have a failover but we're not OK with
losing if we have a local crash from which we recover without failing
over.I'm OK with this.
The attached patch merges synchronous_replication into synchronous_commit.
With the patch, valid values of synchronous_commit are "on" (waits for local
flush and sync rep), "off" (waits for neither local flush nor sync
rep), and "local"
(waits for only local flush).
Committed with some additional hacking. In particular, I believe that
your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
synchronous_replication by synchronous_commit in the docs was a bit
too formulaic; in particular, the section on setting up a basic sync
rep configuration said that all you needed to do was set
synchronous_commit=on, which clearly made no sense, since that was
neither necessary (since that's the default) nor sufficient (since you
have to set synchronous_standby_names).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, Apr 4, 2011 at 4:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
I'm OK with this.
The attached patch merges synchronous_replication into synchronous_commit.
With the patch, valid values of synchronous_commit are "on" (waits for local
flush and sync rep), "off" (waits for neither local flush nor sync
rep), and "local"
(waits for only local flush).Committed with some additional hacking. In particular, I believe that
your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
synchronous_replication by synchronous_commit in the docs was a bit
too formulaic; in particular, the section on setting up a basic sync
rep configuration said that all you needed to do was set
synchronous_commit=on, which clearly made no sense, since that was
neither necessary (since that's the default) nor sufficient (since you
have to set synchronous_standby_names).
Err, woops. Actually, I'm wrong about the first point: your coding
worked, but I had to adjust it when I reordered the enum. I think the
new ordering is more logical, but YMMV.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Apr 5, 2011 at 6:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Committed with some additional hacking. In particular, I believe that
your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
synchronous_replication by synchronous_commit in the docs was a bit
too formulaic; in particular, the section on setting up a basic sync
rep configuration said that all you needed to do was set
synchronous_commit=on, which clearly made no sense, since that was
neither necessary (since that's the default) nor sufficient (since you
have to set synchronous_standby_names).Err, woops. Actually, I'm wrong about the first point: your coding
worked, but I had to adjust it when I reordered the enum. I think the
new ordering is more logical
Yes. Thanks a lot!
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Hi,
Robert Haas <robertmhaas@gmail.com> writes:
The attached patch merges synchronous_replication into synchronous_commit.
Committed
Without discussion? I would think that this patch is stepping on the
other one toes and that maybe would need to make a decision about sync
rep behavior before to commit this change.
Maybe it's just me, but I'm struggling to understand current community
processes and decisions.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Apr 5, 2011 at 4:53 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
Hi,
Robert Haas <robertmhaas@gmail.com> writes:
The attached patch merges synchronous_replication into synchronous_commit.
Committed
Without discussion? I would think that this patch is stepping on the
other one toes and that maybe would need to make a decision about sync
rep behavior before to commit this change.
Hmm.. I think that we reached the consensus about merging two GUCs
in previous discussion. You argue that synchronization level should be
controlled in separate two parameters?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Fujii Masao <masao.fujii@gmail.com> writes:
Hmm.. I think that we reached the consensus about merging two GUCs
in previous discussion. You argue that synchronization level should be
controlled in separate two parameters?
No, sorry about confusion. One GUC is better. What I'm wondering is
why commit it *now*, because I think we didn't yet decide on what the
supported behaviors supported in 9.1 should be.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 05.04.2011 11:31, Dimitri Fontaine wrote:
Fujii Masao<masao.fujii@gmail.com> writes:
Hmm.. I think that we reached the consensus about merging two GUCs
in previous discussion. You argue that synchronization level should be
controlled in separate two parameters?No, sorry about confusion. One GUC is better. What I'm wondering is
why commit it *now*, because I think we didn't yet decide on what the
supported behaviors supported in 9.1 should be.
What do you mean by "supported behaviors"?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
No, sorry about confusion. One GUC is better. What I'm wondering is
why commit it *now*, because I think we didn't yet decide on what the
supported behaviors supported in 9.1 should be.What do you mean by "supported behaviors"?
Well, I'm thinking about Simon's patch that some decided on procedural
grounds only that it was too late to apply in 9.1, and some others were
saying that given the cost benefit ratio of such a small patch that the
design had already been agreed on, it is legible for 9.1.
I think that we just expressed opinions on the async|recv|fsync|apply
patch, and that we've yet to reach a consensus and make a decision.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Apr 5, 2011 at 3:53 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
The attached patch merges synchronous_replication into synchronous_commit.
Committed
Without discussion? I would think that this patch is stepping on the
other one toes and that maybe would need to make a decision about sync
rep behavior before to commit this change.
Err, I thought we did. We had a protracted discussion of Simon's
patch: 9 people expressed an opinion; 6 were opposed.
With respect to this patch, the basic design was discussed previously
and Simon, Fujii Masao, Greg Stark and myself all were basically in
favor of something along these lines, and to the best of my
recollection no one spoke against it.
Maybe it's just me, but I'm struggling to understand current community
processes and decisions.
Well, I've already spent a fair amount of time trying to explain my
understanding of it, and for my trouble I got accused of being
long-winded. Which is probably true, but makes me think I should
probably keep this response short. I'm not unwilling to talk about
it, though, and perhaps someone else would like to chime in.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
For what it's worth it seems to me this patch makrmes it at least
conceptually easier to add new modes like Simon plans, not harder. It's
worth making sure we pick names that still make sense when the new
functionality goes in of course.
The other question is whether it's "fair" that one kind of patch goes in and
not the other. Personally I feel changes to GUCs are the kind of thing we
most often want to do in alpha. Patches that change functionality require a
higher barrier and need to be fixing user complaints or bugs. My perception
was that Simon's patch was ggreenberg latter.
On Apr 5, 2011 12:52 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:
On Tue, Apr 5, 2011 at 3:53 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr>
wrote:
Robert Haas <robertmhaas@gmail.com> writes:
The attached patch merges synchronous_replication into
synchronous_commit.
Show quoted text
Committed
Without discussion? I would think that this patch is stepping on the
other one toes and that maybe would need to make a decision about sync
rep behavior before to commit this change.Err, I thought we did. We had a protracted discussion of Simon's
patch: 9 people expressed an opinion; 6 were opposed.With respect to this patch, the basic design was discussed previously
and Simon, Fujii Masao, Greg Stark and myself all were basically in
favor of something along these lines, and to the best of my
recollection no one spoke against it.Maybe it's just me, but I'm struggling to understand current community
processes and decisions.Well, I've already spent a fair amount of time trying to explain my
understanding of it, and for my trouble I got accused of being
long-winded. Which is probably true, but makes me think I should
probably keep this response short. I'm not unwilling to talk about
it, though, and perhaps someone else would like to chime in.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote:
Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
Maybe it's just me, but I'm struggling to understand current
community processes and decisions.
Well, I've already spent a fair amount of time trying to explain
my understanding of it, and for my trouble I got accused of being
long-winded. Which is probably true, but makes me think I should
probably keep this response short. I'm not unwilling to talk
about it, though, and perhaps someone else would like to chime in.
I rather liked the brief comment in a recent post of yours where you
said that at this point we should only be accepting patches which
stabilize what has already been committed, rather than new features
which might require further stabilization. I don't know whether the
patch under discussion satisfies that test, but that should be the
main consideration at this point in the release cycle, in my view.
Of course, with anything this complex there will be gray areas where
people could have honest disagreement.
-Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Robert Haas <robertmhaas@gmail.com> wrote:
Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
Maybe it's just me, but I'm struggling to understand current
community processes and decisions.
Well, I've already spent a fair amount of time trying to explain
my understanding of it, and for my trouble I got accused of being
long-winded. Which is probably true, but makes me think I should
probably keep this response short. I'm not unwilling to talk
about it, though, and perhaps someone else would like to chime in.
I rather liked the brief comment in a recent post of yours where you
said that at this point we should only be accepting patches which
stabilize what has already been committed, rather than new features
which might require further stabilization.
Quite. While we're on the subject, why did that int->money patch get
committed so quickly? I had assumed that was 9.2 material, because it
didn't seem to be addressing any new-in-9.1 issue. I'm not going to ask
for it to be backed out, but I am wondering what the decision process
was.
regards, tom lane
On Tue, Apr 5, 2011 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Robert Haas <robertmhaas@gmail.com> wrote:
Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
Maybe it's just me, but I'm struggling to understand current
community processes and decisions.Well, I've already spent a fair amount of time trying to explain
my understanding of it, and for my trouble I got accused of being
long-winded. Which is probably true, but makes me think I should
probably keep this response short. I'm not unwilling to talk
about it, though, and perhaps someone else would like to chime in.I rather liked the brief comment in a recent post of yours where you
said that at this point we should only be accepting patches which
stabilize what has already been committed, rather than new features
which might require further stabilization.Quite. While we're on the subject, why did that int->money patch get
committed so quickly? I had assumed that was 9.2 material, because it
didn't seem to be addressing any new-in-9.1 issue. I'm not going to ask
for it to be backed out, but I am wondering what the decision process
was.
Well, I posted a note about this on Thursday:
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01930.php
I didn't feel strongly that it needed to be done, but there seemed to
be some support for doing it:
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01940.php
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01943.php
But I'm wondering whether that was really the right decision. It
might have been better just to drop it, and I'll certainly back it out
if people feel that's more appropriate.
I am also wondering about the open issue of supporting comments to
SQL/MED objects. I thought that was pretty straightforward, but given
that it took me three commits to get servers and foreign data wrappers
squared away and then it turned out that we're still missing support
for user mappings, I've been vividly reminded of the danger of
seemingly harmless commits. Now I'm thinking that I should have just
replied to the initial report with "good point, but it's not a new
regression, so we'll fix it in 9.2". But given that part of the work
has already been done, I'm not sure whether I should (a) finish it, so
we don't have to revisit this in 9.2, (b) leave it well enough alone,
and we'll finish it in 9.2, or (c) back out what's already been done
and plan to fix the whole thing in 9.2.
Everything else on the open items list appears to be a bona fide bug,
though the generate_series thing is not a new regression.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
I am also wondering about the open issue of supporting comments to
SQL/MED objects. I thought that was pretty straightforward, but given
that it took me three commits to get servers and foreign data wrappers
squared away and then it turned out that we're still missing support
for user mappings, I've been vividly reminded of the danger of
seemingly harmless commits. Now I'm thinking that I should have just
replied to the initial report with "good point, but it's not a new
regression, so we'll fix it in 9.2". But given that part of the work
has already been done, I'm not sure whether I should (a) finish it, so
we don't have to revisit this in 9.2, (b) leave it well enough alone,
and we'll finish it in 9.2, or (c) back out what's already been done
and plan to fix the whole thing in 9.2.
On further review, I think (a) is not even an option worth discussing.
The permissions-checking logic for user mappings is quite different
from what we do in the general case, and it seems likely to me that
cleaning this up is going to require far more time and thought than we
ought to be putting into what is really a relatively minor wart. In
retrospect, it seems clear that this wasn't worth messing with in the
first place at this late date in the release cycle.
If there are any other items on the open items list that seem like
things we should not be worrying about right now, please point them
out. I'm likely guilty of tunnel vision, as I have been heavily
focused on trying to make the list go to zero, and of course
committing stuff is only one of two possible ways to get them off the
list - the other is to decide that that they shouldn't have been added
in the first place.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
I am also wondering about the open issue of supporting comments to
SQL/MED objects. �I thought that was pretty straightforward, but given
that it took me three commits to get servers and foreign data wrappers
squared away and then it turned out that we're still missing support
for user mappings, I've been vividly reminded of the danger of
seemingly harmless commits. �Now I'm thinking that I should have just
replied to the initial report with "good point, but it's not a new
regression, so we'll fix it in 9.2". �But given that part of the work
has already been done, I'm not sure whether I should (a) finish it, so
we don't have to revisit this in 9.2, (b) leave it well enough alone,
and we'll finish it in 9.2, or (c) back out what's already been done
and plan to fix the whole thing in 9.2.
On further review, I think (a) is not even an option worth discussing.
The permissions-checking logic for user mappings is quite different
from what we do in the general case, and it seems likely to me that
cleaning this up is going to require far more time and thought than we
ought to be putting into what is really a relatively minor wart. In
retrospect, it seems clear that this wasn't worth messing with in the
first place at this late date in the release cycle.
I agree that we should leave user mappings alone at the moment. I don't
see a need to back out the work that's been done for the other object
types, unless you think there may be flaws in that.
regards, tom lane