Re: Support for N synchronous standby servers - take 2

On Fri, Sep 11, 2015 at 3:41 AM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> Please find attached the WIP patch for the proposed feature. It is built
> based on the already discussed design.
>
> Changes made:
> - add new parameter "sync_file" to provide the location of the pg_syncinfo
> file. The default is 'ConfigDir/pg_syncinfo.conf', same as for pg_hba and
> pg_ident file.

I am not sure that's really necessary. We could just hardcode its location.

> - pg_syncinfo file will hold the sync rep information in the approved JSON
> format.

OK. Have you considered as well the approach to add support for
multi-line GUC parameters? This has been mentioned a couple of time
above as well, with something like that I imagine:
param = 'value1,' \
'value2,' \
'value3'
and this reads as 'value1,value2,value3'. This would benefit as well
for other parameters.

> - synchronous_standby_names can be set to 'pg_syncinfo.conf' to read the
> JSON value stored in the file.

Check.

> - All the standbys mentioned in the s_s_names or the pg_syncinfo file
> currently get the priority as 1 and all others as 0 (async)
> - Various functions in syncrep.c to read the json file and store the values
> in a struct to be used in checking the quorum status of syncrep standbys
> (SyncRepGetQuorumRecPtr function).
> It does not support the current behavior for synchronous_standby_names = '*'.
> I am yet to thoroughly test the patch.

As this patch adds a whole new infrastructure, this is going to need
complex test setups with many configurations that will require either
bash-ing a bunch of new things, and we are not protected from bugs in
those scripts either or manual manipulation mistakes during the tests.
What I think looks really necessary with this patch is to have
included a set of tests to prove that the patch actually does what it
should with complex scenarios and that it does it correctly. So we had
better perhaps move on with this patch first:
https://commitfest.postgresql.org/6/197/

And it would be really nice to get the tests of this patch integrated
with it as well. We are not protected from bugs in this patch as well,
but if we have an infrastructure centralized this will add a level of
confidence that we are doing things the right way. Your patch offers
as well a good occasion to see if there would be some generic routines
that would be helpful in this recovery test suite.
Regards,
--
Michael