Re: Support for N synchronous standby servers

On Thu, Aug 28, 2014 at 12:40 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Wed, Aug 27, 2014 at 2:46 PM, Rajeev rastogi
> <rajeev.rastogi@huawei.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