Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 4c804fb

Browse files
committed
Clean up parsing of synchronous_standby_names GUC variable.
Commit 989be08 added a flex/bison lexer/parser to interpret synchronous_standby_names. It was done in a pretty crufty way, though, making assorted end-use sites responsible for calling the parser at the right times. That was not only vulnerable to errors of omission, but made it possible that lexer/parser errors occur at very undesirable times, and created memory leakages even if there was no error. Instead, perform the parsing once during check_synchronous_standby_names and let guc.c manage the resulting data. To do that, we have to flatten the parsed representation into a single hunk of malloc'd memory, but that is not very hard. While at it, work a little harder on making useful error reports for parsing problems; the previous code felt that "synchronous_standby_names parser returned 1" was an appropriate user-facing error message. (To be fair, it did also log a syntax error message, but separately from the GUC problem report, which is at best confusing.) It had some outright bugs in the face of invalid input, too. I (tgl) also concluded that we need to restrict unquoted names in synchronous_standby_names to be just SQL identifiers. The previous coding would accept darn near anything, which (1) makes the quoting convention both nearly-unnecessary and formally ambiguous, (2) makes it very hard to understand what is a syntax error and what is a creative interpretation of the input as a standby name, and (3) makes it impossible to further extend the syntax in future without a compatibility break. I presume that we're intending future extensions of the syntax, else this parsing infrastructure is massive overkill, so (3) is an important objection. Since we've taken a compatibility hit for non-identifier names with this change anyway, we might as well lock things down now and insist that users use double quotes for standby names that aren't identifiers. Kyotaro Horiguchi and Tom Lane
1 parent 372ff7c commit 4c804fb

File tree

7 files changed

+208
-194
lines changed

7 files changed

+208
-194
lines changed

doc/src/sgml/config.sgml

+14-13
Original file line numberDiff line numberDiff line change
@@ -2983,15 +2983,15 @@ include_dir 'conf.d'
29832983
</term>
29842984
<listitem>
29852985
<para>
2986-
Specifies a list of standby names that can support
2986+
Specifies a list of standby servers that can support
29872987
<firstterm>synchronous replication</>, as described in
29882988
<xref linkend="synchronous-replication">.
29892989
There will be one or more active synchronous standbys;
29902990
transactions waiting for commit will be allowed to proceed after
29912991
these standby servers confirm receipt of their data.
29922992
The synchronous standbys will be those whose names appear
29932993
earlier in this list, and
2994-
that is both currently connected and streaming data in real-time
2994+
that are both currently connected and streaming data in real-time
29952995
(as shown by a state of <literal>streaming</literal> in the
29962996
<link linkend="monitoring-stats-views-table">
29972997
<literal>pg_stat_replication</></link> view).
@@ -3002,7 +3002,7 @@ include_dir 'conf.d'
30023002
Specifying more than one standby name can allow very high availability.
30033003
</para>
30043004
<para>
3005-
This parameter specifies a list of standby servers by using
3005+
This parameter specifies a list of standby servers using
30063006
either of the following syntaxes:
30073007
<synopsis>
30083008
<replaceable class="parameter">num_sync</replaceable> ( <replaceable class="parameter">standby_name</replaceable> [, ...] )
@@ -3013,17 +3013,17 @@ include_dir 'conf.d'
30133013
wait for replies from,
30143014
and <replaceable class="parameter">standby_name</replaceable>
30153015
is the name of a standby server. For example, a setting of
3016-
<literal>'3 (s1, s2, s3, s4)'</> makes transaction commits wait
3017-
until their WAL records are received by three higher priority standbys
3016+
<literal>3 (s1, s2, s3, s4)</> makes transaction commits wait
3017+
until their WAL records are received by three higher-priority standbys
30183018
chosen from standby servers <literal>s1</>, <literal>s2</>,
30193019
<literal>s3</> and <literal>s4</>.
30203020
</para>
30213021
<para>
30223022
The second syntax was used before <productname>PostgreSQL</>
30233023
version 9.6 and is still supported. It's the same as the first syntax
3024-
with <replaceable class="parameter">num_sync</replaceable>=1.
3025-
For example, both settings of <literal>'1 (s1, s2)'</> and
3026-
<literal>'s1, s2'</> have the same meaning; either <literal>s1</>
3024+
with <replaceable class="parameter">num_sync</replaceable> equal to 1.
3025+
For example, <literal>1 (s1, s2)</> and
3026+
<literal>s1, s2</> have the same meaning: either <literal>s1</>
30273027
or <literal>s2</> is chosen as a synchronous standby.
30283028
</para>
30293029
<para>
@@ -3039,11 +3039,12 @@ include_dir 'conf.d'
30393039
</para>
30403040
<note>
30413041
<para>
3042-
The <replaceable class="parameter">standby_name</replaceable>
3043-
must be enclosed in double quotes if a comma (<literal>,</>),
3044-
a double quote (<literal>"</>), <!-- " font-lock sanity -->
3045-
a left parentheses (<literal>(</>), a right parentheses (<literal>)</>)
3046-
or a space is used in the name of a standby server.
3042+
Each <replaceable class="parameter">standby_name</replaceable>
3043+
should have the form of a valid SQL identifier, unless it
3044+
is <literal>*</>. You can use double-quoting if necessary. But note
3045+
that <replaceable class="parameter">standby_name</replaceable>s are
3046+
compared to standby application names case-insensitively, whether
3047+
double-quoted or not.
30473048
</para>
30483049
</note>
30493050
<para>

src/backend/replication/syncrep.c

+72-96
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ char *SyncRepStandbyNames;
7878

7979
static bool announce_next_takeover = true;
8080

81-
SyncRepConfigData *SyncRepConfig;
81+
static SyncRepConfigData *SyncRepConfig = NULL;
8282
static int SyncRepWaitMode = SYNC_REP_NO_WAIT;
8383

8484
static void SyncRepQueueInsert(int mode);
@@ -361,11 +361,6 @@ SyncRepInitConfig(void)
361361
{
362362
int priority;
363363

364-
/* Update the config data of synchronous replication */
365-
SyncRepFreeConfig(SyncRepConfig);
366-
SyncRepConfig = NULL;
367-
SyncRepUpdateConfig();
368-
369364
/*
370365
* Determine if we are a potential sync standby and remember the result
371366
* for handling replies from standby.
@@ -509,7 +504,9 @@ SyncRepGetOldestSyncRecPtr(XLogRecPtr *writePtr, XLogRecPtr *flushPtr,
509504
* Quick exit if we are not managing a sync standby or there are not
510505
* enough synchronous standbys.
511506
*/
512-
if (!(*am_sync) || list_length(sync_standbys) < SyncRepConfig->num_sync)
507+
if (!(*am_sync) ||
508+
SyncRepConfig == NULL ||
509+
list_length(sync_standbys) < SyncRepConfig->num_sync)
513510
{
514511
list_free(sync_standbys);
515512
return false;
@@ -568,14 +565,15 @@ SyncRepGetSyncStandbys(bool *am_sync)
568565
volatile WalSnd *walsnd; /* Use volatile pointer to prevent
569566
* code rearrangement */
570567

568+
/* Set default result */
569+
if (am_sync != NULL)
570+
*am_sync = false;
571+
571572
/* Quick exit if sync replication is not requested */
572573
if (SyncRepConfig == NULL)
573574
return NIL;
574575

575-
if (am_sync != NULL)
576-
*am_sync = false;
577-
578-
lowest_priority = list_length(SyncRepConfig->members);
576+
lowest_priority = SyncRepConfig->nmembers;
579577
next_highest_priority = lowest_priority + 1;
580578

581579
/*
@@ -730,9 +728,8 @@ SyncRepGetSyncStandbys(bool *am_sync)
730728
static int
731729
SyncRepGetStandbyPriority(void)
732730
{
733-
List *members;
734-
ListCell *l;
735-
int priority = 0;
731+
const char *standby_name;
732+
int priority;
736733
bool found = false;
737734

738735
/*
@@ -742,22 +739,19 @@ SyncRepGetStandbyPriority(void)
742739
if (am_cascading_walsender)
743740
return 0;
744741

745-
if (!SyncStandbysDefined())
742+
if (!SyncStandbysDefined() || SyncRepConfig == NULL)
746743
return 0;
747744

748-
members = SyncRepConfig->members;
749-
foreach(l, members)
745+
standby_name = SyncRepConfig->member_names;
746+
for (priority = 1; priority <= SyncRepConfig->nmembers; priority++)
750747
{
751-
char *standby_name = (char *) lfirst(l);
752-
753-
priority++;
754-
755748
if (pg_strcasecmp(standby_name, application_name) == 0 ||
756-
pg_strcasecmp(standby_name, "*") == 0)
749+
strcmp(standby_name, "*") == 0)
757750
{
758751
found = true;
759752
break;
760753
}
754+
standby_name += strlen(standby_name) + 1;
761755
}
762756

763757
return (found ? priority : 0);
@@ -867,50 +861,6 @@ SyncRepUpdateSyncStandbysDefined(void)
867861
}
868862
}
869863

870-
/*
871-
* Parse synchronous_standby_names and update the config data
872-
* of synchronous standbys.
873-
*/
874-
void
875-
SyncRepUpdateConfig(void)
876-
{
877-
int parse_rc;
878-
879-
if (!SyncStandbysDefined())
880-
return;
881-
882-
/*
883-
* check_synchronous_standby_names() verifies the setting value of
884-
* synchronous_standby_names before this function is called. So
885-
* syncrep_yyparse() must not cause an error here.
886-
*/
887-
syncrep_scanner_init(SyncRepStandbyNames);
888-
parse_rc = syncrep_yyparse();
889-
syncrep_scanner_finish();
890-
891-
if (parse_rc != 0)
892-
ereport(ERROR,
893-
(errcode(ERRCODE_SYNTAX_ERROR),
894-
errmsg_internal("synchronous_standby_names parser returned %d",
895-
parse_rc)));
896-
897-
SyncRepConfig = syncrep_parse_result;
898-
syncrep_parse_result = NULL;
899-
}
900-
901-
/*
902-
* Free a previously-allocated config data of synchronous replication.
903-
*/
904-
void
905-
SyncRepFreeConfig(SyncRepConfigData *config)
906-
{
907-
if (!config)
908-
return;
909-
910-
list_free_deep(config->members);
911-
pfree(config);
912-
}
913-
914864
#ifdef USE_ASSERT_CHECKING
915865
static bool
916866
SyncRepQueueIsOrderedByLSN(int mode)
@@ -955,78 +905,104 @@ SyncRepQueueIsOrderedByLSN(int mode)
955905
bool
956906
check_synchronous_standby_names(char **newval, void **extra, GucSource source)
957907
{
958-
int parse_rc;
959-
960908
if (*newval != NULL && (*newval)[0] != '\0')
961909
{
910+
int parse_rc;
911+
SyncRepConfigData *pconf;
912+
913+
/* Reset communication variables to ensure a fresh start */
914+
syncrep_parse_result = NULL;
915+
syncrep_parse_error_msg = NULL;
916+
917+
/* Parse the synchronous_standby_names string */
962918
syncrep_scanner_init(*newval);
963919
parse_rc = syncrep_yyparse();
964920
syncrep_scanner_finish();
965921

966-
if (parse_rc != 0)
922+
if (parse_rc != 0 || syncrep_parse_result == NULL)
967923
{
968924
GUC_check_errcode(ERRCODE_SYNTAX_ERROR);
969-
GUC_check_errdetail("synchronous_standby_names parser returned %d",
970-
parse_rc);
925+
if (syncrep_parse_error_msg)
926+
GUC_check_errdetail("%s", syncrep_parse_error_msg);
927+
else
928+
GUC_check_errdetail("synchronous_standby_names parser failed");
971929
return false;
972930
}
973931

974932
/*
975933
* Warn if num_sync exceeds the number of names of potential sync
976-
* standbys. This setting doesn't make sense in most cases because
977-
* it implies that enough number of sync standbys will not appear,
978-
* which makes transaction commits wait for sync replication
979-
* infinitely.
934+
* standbys. This setting doesn't make sense in most cases because it
935+
* implies that enough number of sync standbys will not appear, which
936+
* makes transaction commits wait for sync replication infinitely.
980937
*
981938
* If there are more than one standbys having the same name and
982939
* priority, we can see enough sync standbys to complete transaction
983-
* commits. However it's not recommended to run multiple standbys
984-
* with the same priority because we cannot gain full control of
985-
* the selection of sync standbys from them.
940+
* commits. However it's not recommended to run multiple standbys with
941+
* the same priority because we cannot gain full control of the
942+
* selection of sync standbys from them.
986943
*
987944
* OTOH, that setting is OK if we understand the above problem
988-
* regarding the selection of sync standbys and intentionally
989-
* specify * to match all the standbys.
945+
* regarding the selection of sync standbys and intentionally specify *
946+
* to match all the standbys.
990947
*/
991-
if (syncrep_parse_result->num_sync >
992-
list_length(syncrep_parse_result->members))
948+
if (syncrep_parse_result->num_sync > syncrep_parse_result->nmembers)
993949
{
994-
ListCell *l;
995-
bool has_asterisk = false;
950+
const char *standby_name;
951+
int i;
952+
bool has_asterisk = false;
996953

997-
foreach(l, syncrep_parse_result->members)
954+
standby_name = syncrep_parse_result->member_names;
955+
for (i = 1; i <= syncrep_parse_result->nmembers; i++)
998956
{
999-
char *standby_name = (char *) lfirst(l);
1000-
1001-
if (pg_strcasecmp(standby_name, "*") == 0)
957+
if (strcmp(standby_name, "*") == 0)
1002958
{
1003959
has_asterisk = true;
1004960
break;
1005961
}
962+
standby_name += strlen(standby_name) + 1;
1006963
}
1007964

1008965
/*
1009-
* Only the postmaster warns this inappropriate setting
1010-
* to avoid cluttering the log.
966+
* Only the postmaster warns about this inappropriate setting to
967+
* avoid cluttering the log.
1011968
*/
1012969
if (!has_asterisk && !IsUnderPostmaster)
1013970
ereport(WARNING,
1014-
(errmsg("The configured number of synchronous standbys (%d) exceeds the number of names of potential synchronous ones (%d)",
1015-
syncrep_parse_result->num_sync, list_length(syncrep_parse_result->members)),
971+
(errmsg("configured number of synchronous standbys (%d) exceeds the number of names of potential synchronous ones (%d)",
972+
syncrep_parse_result->num_sync,
973+
syncrep_parse_result->nmembers),
1016974
errhint("Specify more names of potential synchronous standbys in synchronous_standby_names.")));
1017975
}
1018976

977+
/* GUC extra value must be malloc'd, not palloc'd */
978+
pconf = (SyncRepConfigData *)
979+
malloc(syncrep_parse_result->config_size);
980+
if (pconf == NULL)
981+
return false;
982+
memcpy(pconf, syncrep_parse_result, syncrep_parse_result->config_size);
983+
984+
*extra = (void *) pconf;
985+
1019986
/*
1020-
* syncrep_yyparse sets the global syncrep_parse_result as side effect.
1021-
* But this function is required to just check, so frees it
1022-
* after parsing the parameter.
987+
* We need not explicitly clean up syncrep_parse_result. It, and any
988+
* other cruft generated during parsing, will be freed when the
989+
* current memory context is deleted. (This code is generally run in
990+
* a short-lived context used for config file processing, so that will
991+
* not be very long.)
1023992
*/
1024-
SyncRepFreeConfig(syncrep_parse_result);
1025993
}
994+
else
995+
*extra = NULL;
1026996

1027997
return true;
1028998
}
1029999

1000+
void
1001+
assign_synchronous_standby_names(const char *newval, void *extra)
1002+
{
1003+
SyncRepConfig = (SyncRepConfigData *) extra;
1004+
}
1005+
10301006
void
10311007
assign_synchronous_commit(int newval, void *extra)
10321008
{

0 commit comments

Comments
 (0)