Re: Support for N synchronous standby servers - take 2

Hello,

At Mon, 28 Mar 2016 18:38:22 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoAJMDV1EUKMfeyaV24arx4pzUjGHYbY4ZxzKpkiCUvh0Q@mail.gmail.com>
sawada.mshk> On Mon, Mar 28, 2016 at 5:50 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > Thank you for the new patch. Sorry to have overlooked some
> > versions. I'm looking the  v19 patch now.
> >
> > make complains for an unused variable.

Thank you. I'll have a closer look on it a bit later.

> >> Attached latest patch incorporating all review comments so far.
> >>
> >> Aside from the review comments, I did following changes;
> >> - Add logic to avoid fatal exit in yy_fatal_error().
> >
> > Maybe good catch, but..
> >
> >> syncrep_scanstr(const char *str)
> > ..
> >>   * Regain control after a fatal, internal flex error.  It may have
> >>   * corrupted parser state.  Consequently, abandon the file, but trust
> >                                              ~~~~~~~~~~~~~~~~
> >>   * that the state remains sane enough for syncrep_yy_delete_buffer().
> >                                              ~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > guc-file.l actually abandones the config file but syncrep_scanner
> > reads only a value of an item in it. And, the latter is
> > eventually true but a bit hard to understand.
> >
> > The patch will emit a mysterious error message like this.
> >
> >> invalid value for parameter "synchronous_standby_names": "2[a,b,c]"
> >> configuration file ".../postgresql.conf" contains errors
> >
> > This is utterly wrong. A bit related to that, it seems to me that
> > syncrep_scan.l doesn't need the same mechanism with
> > guc-file.l. The nature of the modification would be making
> > call_*_check_hook to be tri-state instead of boolean. So just
> > cathing errors in call_*_check_hook and ereport()'ing as SQL
> > parser does seems enough, but either will do for me.
> 
> Well, I think that call_*_check_hook can not catch such a fatal error.

As mentioned in my comment, SQL parser converts yy_fatal_error
into ereport(ERROR), which can be caught by the upper PG_TRY (by
#define'ing fprintf). So it is doable if you mind exit().

> Because if yy_fatal_error() is called without preventing logic when
> reloading configuration file, postmaster process will abnormal exit
> immediately as well as wal sender process.


> >> - Improve regression test cases.
> >
> > I forgot to mention that, but additionalORDER BY makes the test
> > robust.
> >
> > I doubt the validity of the behavior in the following test.
> >
> >> # Change the synchronous_standby_names = '2[standby1,*,standby2]' and check sync_state
> >
> > Is this regarded as a correct as a value for it?
> 
> Since previous s_s_names (9.5 or before) can accept this value, I
> didn't change behaviour.
> And I added this test case for checking backward compatibility more finely.

I understand that and it's fine. But we need a explanation for
the reason above in the test case or somewhere else.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center