Postgres Pro
Downloads
Home   >   mailing lists

Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

From Thomas Munro
Subject Re: Support for N synchronous standby servers - take 2
Date
Msg-id CAEepm=1sM+eHvfehoGzw-Qa_QmVFHnNV7cQyU-BMNZUFmd8ODg@mail.gmail.com
Whole thread Raw
In response to Re: Support for N synchronous standby servers - take 2  (Beena Emerson <memissemerson@gmail.com>)
Responses Re: Support for N synchronous standby servers - take 2  (Beena Emerson <memissemerson@gmail.com>)
Re: Support for N synchronous standby servers - take 2  (Thomas Munro <thomas.munro@enterprisedb.com>)
List pgsql-hackers
Tree view
On Fri, Sep 11, 2015 at 6:41 AM, Beena Emerson <memissemerson@gmail.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".

--

pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: WIP: Make timestamptz_out less slow.
Next
From: Noah Misch
Date:
Subject: Re: row_security GUC, BYPASSRLS