Re: Support for N synchronous standby servers - take 2

On Fri, Sep 11, 2015 at 6:41 AM, Beena Emerson <memissemerson(at)gmail(dot)com>
wrote:

> Hello,
>
> Please find attached the WIP patch for the proposed feature. It is built
> based on the already discussed design.
>
> Changes made:
> - add new parameter "sync_file" to provide the location of the pg_syncinfo
> file. The default is 'ConfigDir/pg_syncinfo.conf', same as for pg_hba and
> pg_ident file.
> - pg_syncinfo file will hold the sync rep information in the approved JSON
> format.
> - synchronous_standby_names can be set to 'pg_syncinfo.conf' to read the
> JSON value stored in the file.
> - All the standbys mentioned in the s_s_names or the pg_syncinfo file
> currently get the priority as 1 and all others as 0 (async)
> - Various functions in syncrep.c to read the json file and store the
> values in a struct to be used in checking the quorum status of syncrep
> standbys (SyncRepGetQuorumRecPtr function).
>
> It does not support the current behavior for synchronous_standby_names =
> '*'. I am yet to thoroughly test the patch.
>
> Thoughts?
>

This is a great feature, thanks for working on it!

Here is some initial feedback after a quick eyeballing of the patch and a
couple of test runs. I will have more soon after I figure out how to
really test it and try out the configuration system...

It crashes when async standbys connect, as already reported by Sameer
Thakur. It doesn't crash with this change:

@@ -700,6 +700,9 @@ SyncRepGetStandbyPriority(void)
if (am_cascading_walsender)
return 0;

+ if (SyncRepStandbyInfo == NULL)
+ return 0;
+
if (CheckNameList(SyncRepStandbyInfo, application_name, false))
return 1;

I got the following error from clang-602.0.53 on my Mac:

walsender.c:1955:11: error: passing 'char volatile[8192]' to parameter of
type 'void *' discards qualifiers
[-Werror,-Wincompatible-pointer-types-discards-qualifiers]
memcpy(walsnd->name, application_name,
strlen(application_name));
^~~~~~~~~~~~

I think your memcpy and explicit null termination could be replaced with
strcpy, or maybe something to limit buffer overrun damage in case of sizing
bugs elsewhere. But to get rid of that warning you'd still need to cast
away volatile... I note that you do that in SyncRepGetQuorumRecPtr when
you read the string with strcmp. But is that actually safe, with respect
to load/store reordering around spinlock operations? Do we actually need
volatile-preserving cstring copy and compare functions for this type of
thing?

In walsender_private.h:

+#define MAX_APPLICATION_NAME_LEN 8192

What is the basis for this size? application_name is a GUC with
GUC_IS_NAME set. As far as I can see, it's limited to NAMEDATALEN
(including null terminator), so why not use the exact same buffer size?

In load_syncinfo:

+ len = strlen(standby_name);
+ temp->name = malloc(len);
+ memcpy(temp->name, standby_name, len);
+ temp->name[len] = '\0';

This buffer is one byte too short, and doesn't handle malloc failure. And
generally, this code is equivalent to strdup, and could instead be pstrdup
(which raises an error on allocation failure for free). But I'm not sure
which memory context is appropriate and when this should be freed.

Same problem in sync_info_scalar:

+ state->cur_node->name = (char *)
malloc(len);
+ memcpy(state->cur_node->name, token,
strlen(token));
+ state->cur_node->name[len] = '\0';

In SyncRepGetQuorumRecPtr, some extra curly braces:

+ if (node->next)
+ {
+ SyncRepGetQuorumRecPtr(node->next, lsnlist,
node->priority_group);
+ }

... and:

+ if (*lsnlist == NIL)
+ {
+ *lsnlist = lappend(*lsnlist, lsn);
+ }

In sync_info_object_field_start:

+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("Unrecognised key
\"%s\" in file \"%s\"",
+ fname,
SYNC_FILENAME)));

I think this should use US spelling (-ized) as you have it elsewhere. Also
the primary error message should not be capitalised according to the "Error
Message Style Guide".

--
Thomas Munro
http://www.enterprisedb.com