Re: Support for N synchronous standby servers - take 2

On Mon, Feb 15, 2016 at 2:54 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Feb 15, 2016 at 2:11 PM, Kyotaro HORIGUCHI wrote:
>> Surprizingly yes. The list is handled as an identifier list and
>> parsed by SplitIdentifierString thus it can accept double-quoted
>> names.
>

Attached latest version patch which has only feature logic so far.
I'm writing document patch about this feature now, so this version
patch doesn't have document and regression test patch.

> | $ postgres
> | FATAL: syntax error: unexpected character "*"
> Mmm.. It should be tough to find what has happened..

I'm trying to implement better error message, but that change is not
included in this version patch yet.

> malloc/free are used in create_name_node and other functions to
> be used in scanner, but syncgroup_gram.y is said to use
> palloc/pfree. Maybe they should use the same memory
> allocation/freeing functions.

Setting like this, I think that we use malloc/free funcion when we
allocate/free memory for SyncRepStandbys variables.
OTOH, we use palloc/pfree function during parsing SyncRepStandbyString.
Am I missing something?

> I suppose SyncRepGetSyncedLsnsFn, or SyncRepGetSyncedLsnsPriority
> can return InvalidXLogRecPtr as cur_*_pos even when it returns
> true. And, I suppose comparison of LSN values with
> InvalidXLogRecPtr is not well-defined. Anyway the condition goes
> wrong when cur_write_pos = InvalidXLogRecPtr (but ret = true).

In this version patch, it's not possible to return InvalidXLogRecPtr
with got_lsns = false (was ret = false).
So we can ensure that we got valid LSNs when got_lsns = true.

> At a glance, SyncRepGetSyncedLsnsPriority and
> SyncRepGetSyncStandbysPriority does almost the same thing and both
> runs loops over group members. Couldn't they run at once?

Yeah, I've optimized that logic.

> We may want to be careful with the use of '[' in application_name.
> I am not much thrilled with forbidding the use of []() in application_name, so we may
> want to recommend user to use a backslash when using s_s_names when a
> group is defined.
> s_s_names='abc, def, " abc,""def"'
>
> Result list is ["abc", "def", " abc,\"def"]
>
> Simplly supporting the same notation addresses the problem and
> accepts strings like the following.
>
> s_s_names='2["comma,name", "foo[bar,baz]"]'

I've changed s_s_names parser so that it can handle special 4
characters (\,\ \[\]) and can handle double-quoted string accurately
same as what SplitIdentifierString does.
We can not use special 4 characters (\,\ \[ \]) without using
double-quoted string. Also if we use "(double-quote) character in
double-quoted string, we should use ""(double double-quotes).
For example,
if application_name = 'hoge " bar', s_s_name = '"hoge "" bar"' would be matched.

Other given comments are fixed.

Remaining tasks are;
- Document patch.
- Regression test patch.
- Syntax error message for s_s_names improvement.

Regards,

--
Masahiko Sawada