On Tue, Mar 22, 2016 at 11:08 PM, Fujii Masao <masao.fu...@gmail.com> wrote: > On Tue, Mar 22, 2016 at 9:58 PM, Kyotaro HORIGUCHI > <horiguchi.kyot...@lab.ntt.co.jp> wrote: >> Thank you for the revised patch. > > Thanks for reviewing the patch! > >> This version looks to focus on n-priority method. Stuffs for the >> other methods like n-quorum has been removed. It is okay for me. > > I don't think it's so difficult to extend this version so that > it supports also quorum commit.
Yeah, 1-nest level implementation would not so difficult.
>> StringInfo for double-quoted names seems to me to be overkill,
>> since it allocates 1024 byte block for every such name. A static
>> buffer seems enough for the usage as I said.
>
> So, what about changing the scanner code as follows?
>
> <xd>{xdstop} {
> yylval.str = pstrdup(xdbuf.data);
> pfree(xdbuf.data);
> BEGIN(INITIAL);
> return NAME;
>> The parser is called for not only for SIGHUP, but also for
>> starting of every walsender. The latter is not necessary but it
>> is the matter of trade-off between simplisity and
>> effectiveness.
>
> Could you elaborate why you think that's not necessary?
>
> BTW, in previous patch, s_s_names is parsed by postmaster during the server
> startup. A child process takes over the internal data struct for the parsed
> s_s_names when it's forked by the postmaster. This is what the previous
> patch was expecting. However, this doesn't work in EXEC_BACKEND environment.
> In that environment, the data struct should be passed to a child process via
> the special file (like write_nondefault_variables() does), or it should
> be constructed during walsender startup (like latest version of the patch
> does). IMO the latter is simpler.
Thank you for updating patch.
Followings are random review comments.
==
+ for (cell = list_head(pending); cell; cell = next)
Can we use foreach() instead?
==
+ pending = list_delete_cell(pending, cell, prev);
+
+ if (list_length(pending) == 0)
+ {
+ list_free(pending);
+ return result; /*
Exit if pending list is empty */
+ }
If pending list become empty after deleting element, we can return.
It's a small optimisation.
==
If num_sync is greater than the number of members of sync standby
list, we'd rather return error message immediately.
Thoughts?
==
I got assertion error when master server is set up with empty s_s_names.
Because current patch always tries to parse s_s_names and use it
regardless value of parameter.
Attached patch incorporates above comments.
Please find it.
Regards,
--
Masahiko Sawada
multi_sync_replication_v17.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
