Thread: Standbys using commas in application_name cannot become sync nodes
Hi all, Commas are authorized characters in application_name for a node in recovery, however this overlaps with the fact that synchronous_standby_names uses commas as a separator for each node name. So, if a standby node uses a comma in its name, even if its name is set in s_s_names it can never become a synchronous node because SplitIdentifierString() splits this parameter with only a comma. Even if I have never seen an standby using a comma in its application_name, this is a bug, and here are a couple of things that we could do regarding it: 1) Do not care, who is actually going to use a comma in application_name?! 2) Forbid the use of commas in application_name 3) Enhaunce a bit s_s_names splitting so as it can consider backslash+comma as part of a standby name. In short by setting s_s_names = 'foo\,bar', a standby with name 'foo,bar' would be a sync node. 4) Document the limitation and discourage the use of commas in application_name Thoughts? -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> Commas are authorized characters in application_name for a node in
> recovery, however this overlaps with the fact that
> synchronous_standby_names uses commas as a separator for each node
> name. So, if a standby node uses a comma in its name, even if its name
> is set in s_s_names it can never become a synchronous node because
> SplitIdentifierString() splits this parameter with only a comma.
> Even if I have never seen an standby using a comma in its
> application_name, this is a bug, and here are a couple of things that
> we could do regarding it:
> 1) Do not care, who is actually going to use a comma in application_name?!
> 2) Forbid the use of commas in application_name
> 3) Enhaunce a bit s_s_names splitting so as it can consider
> backslash+comma as part of a standby name. In short by setting
> s_s_names = 'foo\,bar', a standby with name 'foo,bar' would be a sync
> node.
> 4) Document the limitation and discourage the use of commas in application_name
(3) seems like a mess with likely side-effects on other uses of
SplitIdentifierString. I'd vote for (2) or (4).
regards, tom lane
On 2016-02-14 10:51:01 -0500, Tom Lane wrote: > Michael Paquier <michael.paquier@gmail.com> writes: > > Commas are authorized characters in application_name for a node in > > recovery, however this overlaps with the fact that > > synchronous_standby_names uses commas as a separator for each node > > name. So, if a standby node uses a comma in its name, even if its name > > is set in s_s_names it can never become a synchronous node because > > SplitIdentifierString() splits this parameter with only a comma. > > > Even if I have never seen an standby using a comma in its > > application_name, this is a bug, and here are a couple of things that > > we could do regarding it: > > 1) Do not care, who is actually going to use a comma in application_name?! > > 2) Forbid the use of commas in application_name > > 3) Enhaunce a bit s_s_names splitting so as it can consider > > backslash+comma as part of a standby name. In short by setting > > s_s_names = 'foo\,bar', a standby with name 'foo,bar' would be a sync > > node. > > 4) Document the limitation and discourage the use of commas in application_name > > (3) seems like a mess with likely side-effects on other uses of > SplitIdentifierString. I'd vote for (2) or (4). I know a number of applications written by clients, without my encouragement, that use application name to provide e.g. the application user triggering queries. At least two had used commas at some point. So I'm disinclined to go with 2). Andres
On Mon, Feb 15, 2016 at 12:57 AM, Andres Freund <andres@anarazel.de> wrote: > On 2016-02-14 10:51:01 -0500, Tom Lane wrote: >> Michael Paquier <michael.paquier@gmail.com> writes: >> > Commas are authorized characters in application_name for a node in >> > recovery, however this overlaps with the fact that >> > synchronous_standby_names uses commas as a separator for each node >> > name. So, if a standby node uses a comma in its name, even if its name >> > is set in s_s_names it can never become a synchronous node because >> > SplitIdentifierString() splits this parameter with only a comma. >> >> > Even if I have never seen an standby using a comma in its >> > application_name, this is a bug, and here are a couple of things that >> > we could do regarding it: >> > 1) Do not care, who is actually going to use a comma in application_name?! >> > 2) Forbid the use of commas in application_name >> > 3) Enhaunce a bit s_s_names splitting so as it can consider >> > backslash+comma as part of a standby name. In short by setting >> > s_s_names = 'foo\,bar', a standby with name 'foo,bar' would be a sync >> > node. >> > 4) Document the limitation and discourage the use of commas in application_name >> >> (3) seems like a mess with likely side-effects on other uses of >> SplitIdentifierString. I'd vote for (2) or (4). > > I know a number of applications written by clients, without my > encouragement, that use application name to provide e.g. the application > user triggering queries. At least two had used commas at some point. So > I'm disinclined to go with 2). Another possibility to consider would be to mention that in the documentation on back-branches, and simply block its use on HEAD. For the quorum sync patch (which is far from baked yet), we are going to need two extra special characters to identify quorum and priority groups of nodes, so we would need to provide the same solution as for commas. -- Michael
On Mon, Feb 15, 2016 at 9:24 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Another possibility to consider would be to mention that in the > documentation on back-branches, and simply block its use on HEAD. For > the quorum sync patch (which is far from baked yet), we are going to > need two extra special characters to identify quorum and priority > groups of nodes, so we would need to provide the same solution as for > commas. Horiguchi-san has just mentioned in [1] that one can use double-quotes in s_s_names around a node name to bypass the problem. That's undocumented though, so wouldn't we want to mention that at least? [1]: http://www.postgresql.org/message-id/20160215.141102.28792569.horiguchi.kyotaro@lab.ntt.co.jp -- Michael
On Mon, Feb 15, 2016 at 6:56 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Feb 15, 2016 at 9:24 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > Another possibility to consider would be to mention that in the > > documentation on back-branches, and simply block its use on HEAD. For > > the quorum sync patch (which is far from baked yet), we are going to > > need two extra special characters to identify quorum and priority > > groups of nodes, so we would need to provide the same solution as for > > commas. > > Horiguchi-san has just mentioned in [1] that one can use double-quotes > in s_s_names around a node name to bypass the problem. That's > undocumented though, so wouldn't we want to mention that at least? > [1]: > http://www.postgresql.org/message-id/20160215.141102.28792569.horiguchi.kyotaro@lab.ntt.co.jp > > I think that combined with option 4 (comment the possible caveat) is the best way forward. Especially since we have this ability, there is no need to restrict it and possibly break existing applications. But documenting it would clearly be an advantage. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Mon, Feb 15, 2016 at 4:33 PM, Magnus Hagander <magnus@hagander.net> wrote: > > > On Mon, Feb 15, 2016 at 6:56 AM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Mon, Feb 15, 2016 at 9:24 AM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >> > Another possibility to consider would be to mention that in the >> > documentation on back-branches, and simply block its use on HEAD. For >> > the quorum sync patch (which is far from baked yet), we are going to >> > need two extra special characters to identify quorum and priority >> > groups of nodes, so we would need to provide the same solution as for >> > commas. >> >> Horiguchi-san has just mentioned in [1] that one can use double-quotes >> in s_s_names around a node name to bypass the problem. That's >> undocumented though, so wouldn't we want to mention that at least? >> [1]: >> http://www.postgresql.org/message-id/20160215.141102.28792569.horiguchi.kyotaro@lab.ntt.co.jp >> > > I think that combined with option 4 (comment the possible caveat) is the > best way forward. Especially since we have this ability, there is no need to > restrict it and possibly break existing applications. But documenting it > would clearly be an advantage. OK, I just typed up a doc patch that I think follows those lines, here is the paragraph: + When a standby node includes in its <varname>application_name</> + value a comma (<literal>,</>), it can not be chosen as a synchronous + standby because <varname>synchronous_standby_name</> uses as a + separator character a comma. This can be avoided by double-quoting + the standby name in the list provided by this parameter. The use + of commas in <varname>application_name</> is not recommended to avoid + any confusion though. -- Michael