Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit4c804fb

Browse files
committed
Clean up parsing of synchronous_standby_names GUC variable.
Commit989be08 added a flex/bison lexer/parser to interpretsynchronous_standby_names. It was done in a pretty crufty way, though,making assorted end-use sites responsible for calling the parser at theright times. That was not only vulnerable to errors of omission, but madeit 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_namesand let guc.c manage the resulting data. To do that, we have to flattenthe parsed representation into a single hunk of malloc'd memory, but thatis not very hard.While at it, work a little harder on making useful error reports forparsing problems; the previous code felt that "synchronous_standby_namesparser returned 1" was an appropriate user-facing error message. (Tobe fair, it did also log a syntax error message, but separately from theGUC problem report, which is at best confusing.) It had some outrightbugs in the face of invalid input, too.I (tgl) also concluded that we need to restrict unquoted names insynchronous_standby_names to be just SQL identifiers. The previous codingwould accept darn near anything, which (1) makes the quoting conventionboth nearly-unnecessary and formally ambiguous, (2) makes it very hard tounderstand what is a syntax error and what is a creative interpretation ofthe input as a standby name, and (3) makes it impossible to further extendthe syntax in future without a compatibility break. I presume that we'reintending future extensions of the syntax, else this parsing infrastructureis massive overkill, so (3) is an important objection. Since we've takena compatibility hit for non-identifier names with this change anyway, wemight as well lock things down now and insist that users use double quotesfor standby names that aren't identifiers.Kyotaro Horiguchi and Tom Lane
1 parent372ff7c commit4c804fb

File tree

7 files changed

+208
-194
lines changed

7 files changed

+208
-194
lines changed

‎doc/src/sgml/config.sgml

Lines changed: 14 additions & 13 deletions
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 standbynames that can support
2986+
Specifies a list of standbyservers 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-
thatis both currently connected and streaming data in real-time
2994+
thatare 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 serversbyusing
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 higherpriority 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 to1.
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

Lines changed: 72 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ char *SyncRepStandbyNames;
7878

7979
staticboolannounce_next_takeover= true;
8080

81-
SyncRepConfigData*SyncRepConfig;
81+
staticSyncRepConfigData*SyncRepConfig=NULL;
8282
staticintSyncRepWaitMode=SYNC_REP_NO_WAIT;
8383

8484
staticvoidSyncRepQueueInsert(intmode);
@@ -361,11 +361,6 @@ SyncRepInitConfig(void)
361361
{
362362
intpriority;
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
volatileWalSnd*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
returnNIL;
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
staticint
731729
SyncRepGetStandbyPriority(void)
732730
{
733-
List*members;
734-
ListCell*l;
735-
intpriority=0;
731+
constchar*standby_name;
732+
intpriority;
736733
boolfound= false;
737734

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

745-
if (!SyncStandbysDefined())
742+
if (!SyncStandbysDefined()||SyncRepConfig==NULL)
746743
return0;
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-
intparse_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
#ifdefUSE_ASSERT_CHECKING
915865
staticbool
916866
SyncRepQueueIsOrderedByLSN(intmode)
@@ -955,78 +905,104 @@ SyncRepQueueIsOrderedByLSN(int mode)
955905
bool
956906
check_synchronous_standby_names(char**newval,void**extra,GucSourcesource)
957907
{
958-
intparse_rc;
959-
960908
if (*newval!=NULL&& (*newval)[0]!='\0')
961909
{
910+
intparse_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-
*withthe same priority because we cannot gain full control of
985-
*theselection 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-
boolhas_asterisk= false;
950+
constchar*standby_name;
951+
inti;
952+
boolhas_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-
*toavoid cluttering the log.
966+
* Only the postmaster warnsaboutthis 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(constchar*newval,void*extra)
1002+
{
1003+
SyncRepConfig= (SyncRepConfigData*)extra;
1004+
}
1005+
10301006
void
10311007
assign_synchronous_commit(intnewval,void*extra)
10321008
{

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp