Re: Support for N synchronous standby servers

On Thu, Aug 28, 2014 at 12:40 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Wed, Aug 27, 2014 at 2:46 PM, Rajeev rastogi
> <rajeev(dot)rastogi(at)huawei(dot)com> wrote:
> > I have done some more review, below are my comments:
> Thanks!
>
> > 1. There are currently two loops on *num_sync, Can we simplify the
function SyncRepGetSynchronousNodes by moving the priority calculation
inside the upper loop
> > if (*num_sync == allowed_sync_nodes)
> > {
> > for (j = 0; j < *num_sync; j++)
> > {
> > Anyway we require priority only if *num_sync ==
allowed_sync_nodes condition matches.
> > So in this loop itself, we can calculate the priority as well
as assigment of new standbys with lower priority.
> > Let me know if you see any issue with this.
>
> OK, I see, yes this can minimize process a bit so I refactored the
> code by integrating the second loop to the first. This has needed the
> removal of the break portion as we need to find the highest priority
> value among the nodes already determined as synchronous.
>
> > 2. Comment inside the function SyncRepReleaseWaiters,
> > /*
> > * We should have found ourselves at least, except if it is not
expected
> > * to find any synchronous nodes.
> > */
> > Assert(num_sync > 0);
> >
> > I think "except if it is not expected to find any synchronous
nodes" is not required.
> > As if it has come till this point means at least this node is
synchronous.
> Yes, removed.
>
> > 3. Document says that s_s_num should be lesser than
max_wal_senders but code wise there is no protection for the same.
> > IMHO, s_s_num should be lesser than or equal to max_wal_senders
otherwise COMMIT will never return back the console without
> > any knowledge of user.
> > I see that some discussion has happened regarding this but I
think just adding documentation for this is not enough.
> > I am not sure what issue is observed in adding check during GUC
initialization but if there is unavoidable issue during GUC initialization
then can't we try to add check at later points.
>
> The trick here is that you cannot really return a warning at GUC
> loading level to the user as a warning could be easily triggered if
> for example s_s_num is present before max_wal_senders in
> postgresql.conf. I am open to any solutions if there are any (like an
> error when initializing WAL senders?!). Documentation seems enough for
> me to warn the user.

How about making it as a PGC_POSTMASTER parameter and then
have a check similar to below in PostmasterMain()

/*
* Check for invalid combinations of GUC settings.
*/
if (ReservedBackends >= MaxConnections)
{
write_stderr("%s: superuser_reserved_connections must be less than
max_connections\n", progname);
ExitPostmaster(1);
}

if (max_wal_senders >= MaxConnections)
{
write_stderr("%s: max_wal_senders must be less than max_connections\n",
progname);
ExitPostmaster(1);
}

if (XLogArchiveMode && wal_level == WAL_LEVEL_MINIMAL)
ereport(ERROR,
(errmsg("WAL archival (archive_mode=on) requires wal_level \"archive\",
\"hot_standby\", or \"logical\"")));

if (max_wal_senders > 0 && wal_level == WAL_LEVEL_MINIMAL)
ereport(ERROR,
(errmsg("WAL streaming (max_wal_senders > 0) requires wal_level
\"archive\", \"hot_standby\", or \"logical\"")));

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com