Re: Support for N synchronous standby servers - take 2

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.
>
> | syncrep.c: In function ‘SyncRepGetSyncStandbys’:
> | syncrep.c:601:13: warning: variable ‘next’ set but not used [-Wunused-but-set-variable]
> |    ListCell *next;
>
>
> At Thu, 24 Mar 2016 22:29:01 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCxwezOTf9kLQRhuf2y=1c_fGjCormqJfqHOmQW8EgaDg@mail.gmail.com>
>> >> > SyncRepInitConfig()->SyncRepFreeConfig() has already pfree'd that
>> >> > in the patch. Or am I missing something?
>> >
>> > Sorry, instead, the memory from strdup() will be abandoned in
>> > upper level. (Thinking for some time..) Ah, I found that the
>> > problem should be here.
>> >
>> >  > SyncRepFreeConfig(SyncRepConfigData *config)
>> >  > {
>> > ...
>> > !>      list_free(config->members);
>> >  >      pfree(config);
>> >  > }
>> >
>> > The list_free *doesn't* free the memory blocks pointed by
>> > lfirst(cell), which has been pstrdup'ed. It should be
>> > list_free_deep(config->members) instead to free it completely.
>
> Fujii> Yep, but SyncRepFreeConfig() already uses list_free_deep
> Fujii> in the latest patch.  Could you read the latest version
> Fujii> that I posted upthread.
>
> Sorry for overlooked the version. Every pair of parse(or
> SyncRepUpdateConfig) and SyncRepFreeConfig is on the same memory
> context so it seems safe (but might be fragile since it relies on
> that the caller does so.).
>
>> >> Previous(9.5 or before) s_s_names also accepts non-ASCII character and
>> >> non-printable character, and can show it without replacing these
>> >> character to '?'.
>> >
>> > Thank you for pointint it out (it was completely out of my
>> > mind..). I have no objection to keep the previous behavior.
>> >
>> >> From backward compatibility perspective, we should not choose #1 or #2.
>> >> Different behaviour between previous and current s_s_names is that
>> >> previous s_s_names doesn't accept the node name having the sort of
>> >> white-space character that isspace() returns true with.
>> >> But current s_s_names allows us to specify such a node name.
>> >> I guess that changing such behaviour is enough for fixing this issue.
>> >> Thoughts?
>> >
>>
>> 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.
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.

Regards,

--
Masahiko Sawada