public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v2 3/4] Add a new MODE_SINGLE_QUERY to the core parser and use it in pg_parse_query. 32+ messages / 7 participants [nested] [flat]
* [PATCH v2 3/4] Add a new MODE_SINGLE_QUERY to the core parser and use it in pg_parse_query. @ 2021-04-21 17:33 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Julien Rouhaud @ 2021-04-21 17:33 UTC (permalink / raw) If a third-party module provides a parser_hook, pg_parse_query() switches to single-query parsing so multi-query commands using different grammar can work properly. If the third-party module supports the full set of SQL we support, or want to prevent fallback on the core parser, it can ignore the MODE_SINGLE_QUERY mode and parse the full query string. In that case they must return a List with more than one RawStmt or a single RawStmt with a 0 length to stop the parsing phase, or raise an ERROR. Otherwise, plugins should parse a single query only and always return a List containing a single RawStmt with a properly set length (possibly 0 if it was a single query without end of query delimiter). If the command is valid but doesn't contain any statements (e.g. a single semi-colon), a single RawStmt with a NULL stmt field should be returned, containing the consumed query string length so we can move to the next command in a single pass rather than 1 byte at a time. Also, third-party modules can choose to ignore some or all of parsing error if they want to implement only subset of postgres suppoted syntax, or even a totally different syntax, and fall-back on core grammar for unhandled case. In thase case, they should set the error flag to true. The returned List will be ignored and the same offset of the input string will be parsed using the core parser. Finally, note that third-party plugins that wants to fallback on other grammar should first try to call a previous parser hook if any before setting the error switch and returning. --- .../pg_stat_statements/pg_stat_statements.c | 3 +- src/backend/commands/tablecmds.c | 2 +- src/backend/executor/spi.c | 4 +- src/backend/parser/gram.y | 29 +++- src/backend/parser/parse_type.c | 2 +- src/backend/parser/parser.c | 15 +- src/backend/parser/scan.l | 26 +++- src/backend/tcop/postgres.c | 138 ++++++++++++++++-- src/include/parser/parser.h | 5 +- src/include/parser/scanner.h | 6 +- src/include/tcop/tcopprot.h | 3 +- src/pl/plpgsql/src/pl_gram.y | 2 +- src/pl/plpgsql/src/pl_scanner.c | 2 +- 13 files changed, 210 insertions(+), 27 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 09433c8c96..d852575613 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2718,7 +2718,8 @@ fill_in_constant_lengths(JumbleState *jstate, const char *query, yyscanner = scanner_init(query, &yyextra, &ScanKeywords, - ScanKeywordTokens); + ScanKeywordTokens, + 0); /* we don't want to re-emit any escape string warnings */ yyextra.escape_string_warning = false; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 028e8ac46b..284933c693 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -12677,7 +12677,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, * parse_analyze() or the rewriter, but instead we need to pass them * through parse_utilcmd.c to make them ready for execution. */ - raw_parsetree_list = raw_parser(cmd, RAW_PARSE_DEFAULT); + raw_parsetree_list = raw_parser(cmd, RAW_PARSE_DEFAULT, 0); querytree_list = NIL; foreach(list_item, raw_parsetree_list) { diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index b8bd05e894..f05b3ce9e7 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -2120,7 +2120,7 @@ _SPI_prepare_plan(const char *src, SPIPlanPtr plan) /* * Parse the request string into a list of raw parse trees. */ - raw_parsetree_list = raw_parser(src, plan->parse_mode); + raw_parsetree_list = raw_parser(src, plan->parse_mode, 0); /* * Do parse analysis and rule rewrite for each raw parsetree, storing the @@ -2228,7 +2228,7 @@ _SPI_prepare_oneshot_plan(const char *src, SPIPlanPtr plan) /* * Parse the request string into a list of raw parse trees. */ - raw_parsetree_list = raw_parser(src, plan->parse_mode); + raw_parsetree_list = raw_parser(src, plan->parse_mode, 0); /* * Construct plancache entries, but don't do parse analysis yet. diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 9ee90e3f13..2cac062ef4 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -626,7 +626,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %token <str> IDENT UIDENT FCONST SCONST USCONST BCONST XCONST Op %token <ival> ICONST PARAM %token TYPECAST DOT_DOT COLON_EQUALS EQUALS_GREATER -%token LESS_EQUALS GREATER_EQUALS NOT_EQUALS +%token LESS_EQUALS GREATER_EQUALS NOT_EQUALS END_OF_FILE /* * If you want to make any keyword changes, update the keyword table in @@ -753,6 +753,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %token MODE_PLPGSQL_ASSIGN1 %token MODE_PLPGSQL_ASSIGN2 %token MODE_PLPGSQL_ASSIGN3 +%token MODE_SINGLE_QUERY /* Precedence: lowest to highest */ @@ -858,6 +859,32 @@ parse_toplevel: pg_yyget_extra(yyscanner)->parsetree = list_make1(makeRawStmt((Node *) n, 0)); } + | MODE_SINGLE_QUERY toplevel_stmt ';' + { + RawStmt *raw = makeRawStmt($2, 0); + updateRawStmtEnd(raw, @3 + 1); + /* NOTE: we can return a raw statement containing a NULL stmt. + * This is done to allow pg_parse_query to ignore that part of + * the input string and move to the next command. + */ + pg_yyget_extra(yyscanner)->parsetree = list_make1(raw); + YYACCEPT; + } + /* + * We need to explicitly look for EOF to parse non-semicolon + * terminated statements in single query mode, as we could + * otherwise successfully parse the beginning of an otherwise + * invalid query. + */ + | MODE_SINGLE_QUERY toplevel_stmt END_OF_FILE + { + /* NOTE: we can return a raw statement containing a NULL stmt. + * This is done to allow pg_parse_query to ignore that part of + * the input string. + */ + pg_yyget_extra(yyscanner)->parsetree = list_make1(makeRawStmt($2, 0)); + YYACCEPT; + } ; /* diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c index abe131ebeb..e9a7b5d62a 100644 --- a/src/backend/parser/parse_type.c +++ b/src/backend/parser/parse_type.c @@ -746,7 +746,7 @@ typeStringToTypeName(const char *str) ptserrcontext.previous = error_context_stack; error_context_stack = &ptserrcontext; - raw_parsetree_list = raw_parser(str, RAW_PARSE_TYPE_NAME); + raw_parsetree_list = raw_parser(str, RAW_PARSE_TYPE_NAME, 0); error_context_stack = ptserrcontext.previous; diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c index 875de7ba28..418c50ee8f 100644 --- a/src/backend/parser/parser.c +++ b/src/backend/parser/parser.c @@ -37,17 +37,25 @@ static char *str_udeescape(const char *str, char escape, * * Returns a list of raw (un-analyzed) parse trees. The contents of the * list have the form required by the specified RawParseMode. + * + * For all mode different from MODE_SINGLE_QUERY, caller should provide a 0 + * offset as the whole input string should be parsed. Otherwise, caller should + * provide the wanted offset in the input string, or -1 if no offset is + * required. */ List * -raw_parser(const char *str, RawParseMode mode) +raw_parser(const char *str, RawParseMode mode, int offset) { core_yyscan_t yyscanner; base_yy_extra_type yyextra; int yyresult; + Assert((mode != MODE_SINGLE_QUERY && offset == 0) || + (mode == MODE_SINGLE_QUERY && offset != 0)); + /* initialize the flex scanner */ yyscanner = scanner_init(str, &yyextra.core_yy_extra, - &ScanKeywords, ScanKeywordTokens); + &ScanKeywords, ScanKeywordTokens, offset); /* base_yylex() only needs us to initialize the lookahead token, if any */ if (mode == RAW_PARSE_DEFAULT) @@ -61,7 +69,8 @@ raw_parser(const char *str, RawParseMode mode) MODE_PLPGSQL_EXPR, /* RAW_PARSE_PLPGSQL_EXPR */ MODE_PLPGSQL_ASSIGN1, /* RAW_PARSE_PLPGSQL_ASSIGN1 */ MODE_PLPGSQL_ASSIGN2, /* RAW_PARSE_PLPGSQL_ASSIGN2 */ - MODE_PLPGSQL_ASSIGN3 /* RAW_PARSE_PLPGSQL_ASSIGN3 */ + MODE_PLPGSQL_ASSIGN3, /* RAW_PARSE_PLPGSQL_ASSIGN3 */ + MODE_SINGLE_QUERY /* RAW_PARSE_SINGLE_QUERY */ }; yyextra.have_lookahead = true; diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l index 9f9d8a1706..8ccbe95ac6 100644 --- a/src/backend/parser/scan.l +++ b/src/backend/parser/scan.l @@ -1041,7 +1041,10 @@ other . <<EOF>> { SET_YYLLOC(); - yyterminate(); + if (yyextra->return_eof) + return END_OF_FILE; + else + yyterminate(); } %% @@ -1189,8 +1192,10 @@ core_yyscan_t scanner_init(const char *str, core_yy_extra_type *yyext, const ScanKeywordList *keywordlist, - const uint16 *keyword_tokens) + const uint16 *keyword_tokens, + int offset) { + YY_BUFFER_STATE state; Size slen = strlen(str); yyscan_t scanner; @@ -1213,13 +1218,28 @@ scanner_init(const char *str, yyext->scanbuflen = slen; memcpy(yyext->scanbuf, str, slen); yyext->scanbuf[slen] = yyext->scanbuf[slen + 1] = YY_END_OF_BUFFER_CHAR; - yy_scan_buffer(yyext->scanbuf, slen + 2, scanner); + state = yy_scan_buffer(yyext->scanbuf, slen + 2, scanner); /* initialize literal buffer to a reasonable but expansible size */ yyext->literalalloc = 1024; yyext->literalbuf = (char *) palloc(yyext->literalalloc); yyext->literallen = 0; + /* + * Note that pg_parse_query will set a -1 offset rather than 0 for the + * first query of a possibly multi-query string if it wants us to return an + * EOF token. + */ + yyext->return_eof = (offset != 0); + + /* + * Adjust the offset in the input string. This is required in single-query + * mode, as we need to register the same token locations as we would have + * in normal mode with multi-statement query string. + */ + if (offset > 0) + state->yy_buf_pos += offset; + return scanner; } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index e941b59b85..9331628add 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -602,17 +602,137 @@ ProcessClientWriteInterrupt(bool blocked) List * pg_parse_query(const char *query_string) { - List *raw_parsetree_list = NIL; + List *result = NIL; + int stmt_len, offset; TRACE_POSTGRESQL_QUERY_PARSE_START(query_string); if (log_parser_stats) ResetUsage(); - if (parser_hook) - raw_parsetree_list = (*parser_hook) (query_string, RAW_PARSE_DEFAULT); - else - raw_parsetree_list = raw_parser(query_string, RAW_PARSE_DEFAULT); + stmt_len = 0; /* lazily computed when needed */ + offset = 0; + + while(true) + { + List *raw_parsetree_list; + RawStmt *raw; + bool error = false; + + /*---------------- + * Start parsing the input string. If a third-party module provided a + * parser_hook, we switch to single-query parsing so multi-query + * commands using different grammar can work properly. + * If the third-party modules support the full set of SQL we support, + * or want to prevent fallback on the core parser, it can ignore the + * RAW_PARSE_SINGLE_QUERY flag and parse the full query string. + * In that case they must return a List with more than one RawStmt or a + * single RawStmt with a 0 length to stop the parsing phase, or raise + * an ERROR. + * + * Otherwise, plugins should parse a single query only and always + * return a List containing a single RawStmt with a properly set length + * (possibly 0 if it was a single query without end of query + * delimiter). If the command is valid but doesn't contain any + * statements (e.g. a single semi-colon), a single RawStmt with a NULL + * stmt field should be returned, containing the consumed query string + * length so we can move to the next command in a single pass rather + * than 1 byte at a time. + * + * Also, third-party modules can choose to ignore some or all of + * parsing error if they want to implement only subset of postgres + * suppoted syntax, or even a totally different syntax, and fall-back + * on core grammar for unhandled case. In thase case, they should set + * the error flag to true. The returned List will be ignored and the + * same offset of the input string will be parsed using the core + * parser. + * + * Finally, note that third-party modules that wants to fallback on + * other grammar should first try to call a previous parser hook if any + * before setting the error switch and returning . + */ + if (parser_hook) + raw_parsetree_list = (*parser_hook) (query_string, + RAW_PARSE_SINGLE_QUERY, + offset, + &error); + + /* + * If a third-party module couldn't parse a single query or if no + * third-party module is configured, fallback on core parser. + */ + if (error || !parser_hook) + { + /* Send a -1 offset to raw_parser to specify that it should + * explicitly detect EOF during parsing. scanner_init() will treat + * it the same as a 0 offset. + */ + raw_parsetree_list = raw_parser(query_string, + error ? RAW_PARSE_SINGLE_QUERY : RAW_PARSE_DEFAULT, + (error && offset == 0) ? -1 : offset); + } + + /* + * If there are no third-party plugin, or none of the parsers found a + * valid query, or if a third party module consumed the whole + * query string we're done. + */ + if (!parser_hook || raw_parsetree_list == NIL || + list_length(raw_parsetree_list) > 1) + { + /* + * Warn third-party plugins if they mix "single query" and "whole + * input string" strategy rather than silently accepting it and + * maybe allow fallback on core grammar even if they want to avoid + * that. This way plugin authors can be warned early of the issue. + */ + if (result != NIL) + { + Assert(parser_hook != NULL); + elog(ERROR, "parser_hook should parse a single statement at " + "a time or consume the whole input string at once"); + } + result = raw_parsetree_list; + break; + } + + if (stmt_len == 0) + stmt_len = strlen(query_string); + + raw = linitial_node(RawStmt, raw_parsetree_list); + + /* + * In single-query mode, the parser will return statement location info + * relative to the beginning of complete original string, not the part + * we just parsed, so adjust the location info. + */ + if (offset > 0 && raw->stmt_len > 0) + { + Assert(raw->stmt_len > offset); + raw->stmt_location = offset; + raw->stmt_len -= offset; + } + + /* Ignore the statement if it didn't contain any command. */ + if (raw->stmt) + result = lappend(result, raw); + + if (raw->stmt_len == 0) + { + /* The statement was the whole string, we're done. */ + break; + } + else if (raw->stmt_len + offset >= stmt_len) + { + /* We consumed all of the input string, we're done. */ + break; + } + else + { + /* Advance the offset to the next command. */ + offset += raw->stmt_len; + } + } if (log_parser_stats) ShowUsage("PARSER STATISTICS"); @@ -620,13 +740,13 @@ pg_parse_query(const char *query_string) #ifdef COPY_PARSE_PLAN_TREES /* Optional debugging check: pass raw parsetrees through copyObject() */ { - List *new_list = copyObject(raw_parsetree_list); + List *new_list = copyObject(result); /* This checks both copyObject() and the equal() routines... */ - if (!equal(new_list, raw_parsetree_list)) + if (!equal(new_list, result)) elog(WARNING, "copyObject() failed to produce an equal raw parse tree"); else - raw_parsetree_list = new_list; + result = new_list; } #endif @@ -638,7 +758,7 @@ pg_parse_query(const char *query_string) TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string); - return raw_parsetree_list; + return result; } /* diff --git a/src/include/parser/parser.h b/src/include/parser/parser.h index 853b0f1606..5694ae791a 100644 --- a/src/include/parser/parser.h +++ b/src/include/parser/parser.h @@ -41,7 +41,8 @@ typedef enum RAW_PARSE_PLPGSQL_EXPR, RAW_PARSE_PLPGSQL_ASSIGN1, RAW_PARSE_PLPGSQL_ASSIGN2, - RAW_PARSE_PLPGSQL_ASSIGN3 + RAW_PARSE_PLPGSQL_ASSIGN3, + RAW_PARSE_SINGLE_QUERY } RawParseMode; /* Values for the backslash_quote GUC */ @@ -59,7 +60,7 @@ extern PGDLLIMPORT bool standard_conforming_strings; /* Primary entry point for the raw parsing functions */ -extern List *raw_parser(const char *str, RawParseMode mode); +extern List *raw_parser(const char *str, RawParseMode mode, int offset); /* Utility functions exported by gram.y (perhaps these should be elsewhere) */ extern List *SystemFuncName(char *name); diff --git a/src/include/parser/scanner.h b/src/include/parser/scanner.h index 0d8182faa0..a2e97be5d5 100644 --- a/src/include/parser/scanner.h +++ b/src/include/parser/scanner.h @@ -113,6 +113,9 @@ typedef struct core_yy_extra_type /* state variables for literal-lexing warnings */ bool warn_on_first_escape; bool saw_non_ascii; + + /* state variable for returning an EOF token in single query mode */ + bool return_eof; } core_yy_extra_type; /* @@ -136,7 +139,8 @@ extern PGDLLIMPORT const uint16 ScanKeywordTokens[]; extern core_yyscan_t scanner_init(const char *str, core_yy_extra_type *yyext, const ScanKeywordList *keywordlist, - const uint16 *keyword_tokens); + const uint16 *keyword_tokens, + int offset); extern void scanner_finish(core_yyscan_t yyscanner); extern int core_yylex(core_YYSTYPE *lvalp, YYLTYPE *llocp, core_yyscan_t yyscanner); diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index 131dc2b22e..27201dde1d 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -45,7 +45,8 @@ typedef enum extern PGDLLIMPORT int log_statement; /* Hook for plugins to get control in pg_parse_query() */ -typedef List *(*parser_hook_type) (const char *str, RawParseMode mode); +typedef List *(*parser_hook_type) (const char *str, RawParseMode mode, + int offset, bool *error); extern PGDLLIMPORT parser_hook_type parser_hook; extern List *pg_parse_query(const char *query_string); diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 3fcca43b90..e5a8a6477a 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -3656,7 +3656,7 @@ check_sql_expr(const char *stmt, RawParseMode parseMode, int location) error_context_stack = &syntax_errcontext; oldCxt = MemoryContextSwitchTo(plpgsql_compile_tmp_cxt); - (void) raw_parser(stmt, parseMode); + (void) raw_parser(stmt, parseMode, 0); MemoryContextSwitchTo(oldCxt); /* Restore former ereport callback */ diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c index e4c7a91ab5..a2886c42ec 100644 --- a/src/pl/plpgsql/src/pl_scanner.c +++ b/src/pl/plpgsql/src/pl_scanner.c @@ -587,7 +587,7 @@ plpgsql_scanner_init(const char *str) { /* Start up the core scanner */ yyscanner = scanner_init(str, &core_yy, - &ReservedPLKeywords, ReservedPLKeywordTokens); + &ReservedPLKeywords, ReservedPLKeywordTokens, 0); /* * scanorig points to the original string, which unlike the scanner's -- 2.31.1 --p53pz3r7l3llx7gx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v2-0004-Teach-sqlol-to-use-the-new-MODE_SINGLE_QUERY-pars.patch" ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-03 18:10 Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-03 18:10 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <[email protected]> wrote: > > > [....] how about we revert > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=007693f2a3ac2ac19affcb03ad43cdb36..., > > Would you ever see "conflict" as false and "invalidation_reason" as > non-null for a logical slot? No. Because both conflict and invalidation_reason are decided based on the invalidation reason i.e. value of slot_contents.data.invalidated. IOW, a logical slot that reports conflict as true must have been invalidated. Do you have any thoughts on reverting 007693f and introducing invalidation_reason? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-03 21:44 Nathan Bossart <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 3 replies; 32+ messages in thread From: Nathan Bossart @ 2024-03-03 21:44 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote: > On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <[email protected]> wrote: >> Would you ever see "conflict" as false and "invalidation_reason" as >> non-null for a logical slot? > > No. Because both conflict and invalidation_reason are decided based on > the invalidation reason i.e. value of slot_contents.data.invalidated. > IOW, a logical slot that reports conflict as true must have been > invalidated. > > Do you have any thoughts on reverting 007693f and introducing > invalidation_reason? Unless I am misinterpreting some details, ISTM we could rename this column to invalidation_reason and use it for both logical and physical slots. I'm not seeing a strong need for another column. Perhaps I am missing something... -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-04 01:32 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 2 siblings, 0 replies; 32+ messages in thread From: Michael Paquier @ 2024-03-04 01:32 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]>; Amit Kapila <[email protected]> On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote: >> Do you have any thoughts on reverting 007693f and introducing >> invalidation_reason? > > Unless I am misinterpreting some details, ISTM we could rename this column > to invalidation_reason and use it for both logical and physical slots. I'm > not seeing a strong need for another column. Perhaps I am missing > something... And also, please don't be hasty in taking a decision that would involve a revert of 007693f without informing the committer of this commit about that. I am adding Amit Kapila in CC of this thread for awareness. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-04 08:41 Bertrand Drouvot <[email protected]> parent: Nathan Bossart <[email protected]> 2 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-04 08:41 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote: > > On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <[email protected]> wrote: > >> Would you ever see "conflict" as false and "invalidation_reason" as > >> non-null for a logical slot? > > > > No. Because both conflict and invalidation_reason are decided based on > > the invalidation reason i.e. value of slot_contents.data.invalidated. > > IOW, a logical slot that reports conflict as true must have been > > invalidated. > > > > Do you have any thoughts on reverting 007693f and introducing > > invalidation_reason? > > Unless I am misinterpreting some details, ISTM we could rename this column > to invalidation_reason and use it for both logical and physical slots. I'm > not seeing a strong need for another column. Yeah having two columns was more for convenience purpose. Without the "conflict" one, a slot conflicting with recovery would be "a logical slot having a non NULL invalidation_reason". I'm also fine with one column if most of you prefer that way. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-05 19:20 Bharath Rupireddy <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-05 19:20 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot <[email protected]> wrote: > > On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > > On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote: > > > On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <[email protected]> wrote: > > >> Would you ever see "conflict" as false and "invalidation_reason" as > > >> non-null for a logical slot? > > > > > > No. Because both conflict and invalidation_reason are decided based on > > > the invalidation reason i.e. value of slot_contents.data.invalidated. > > > IOW, a logical slot that reports conflict as true must have been > > > invalidated. > > > > > > Do you have any thoughts on reverting 007693f and introducing > > > invalidation_reason? > > > > Unless I am misinterpreting some details, ISTM we could rename this column > > to invalidation_reason and use it for both logical and physical slots. I'm > > not seeing a strong need for another column. > > Yeah having two columns was more for convenience purpose. Without the "conflict" > one, a slot conflicting with recovery would be "a logical slot having a non NULL > invalidation_reason". > > I'm also fine with one column if most of you prefer that way. While we debate on the above, please find the attached v7 patch set after rebasing. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [application/x-patch] v7-0001-Track-invalidation_reason-in-pg_replication_slots.patch (6.6K, ../../CALj2ACXBz9HG767ink9nQdz0n6EdZz5g_qutek2ffTP6LNDbGw@mail.gmail.com/2-v7-0001-Track-invalidation_reason-in-pg_replication_slots.patch) download | inline diff: From 906f8829f7b6bf1da4b37edf2e4d5a46a7227400 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Tue, 5 Mar 2024 18:56:00 +0000 Subject: [PATCH v7 1/4] Track invalidation_reason in pg_replication_slots Currently the reason for replication slot invalidation is not tracked in pg_replication_slots. A recent commit 007693f2a added conflict_reason to show the reasons for slot invalidation, but only for logical slots. This commit adds invalidation_reason to pg_replication_slots to show invalidation reasons for both physical and logical slots. --- doc/src/sgml/system-views.sgml | 32 ++++++++++++++++++++++++++++ src/backend/catalog/system_views.sql | 3 ++- src/backend/replication/slotfuncs.c | 12 ++++++++--- src/include/catalog/pg_proc.dat | 6 +++--- src/test/regress/expected/rules.out | 5 +++-- 5 files changed, 49 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index be90edd0e2..cce88c14bb 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2581,6 +2581,38 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx </para></entry> </row> + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>invalidation_reason</structfield> <type>text</type> + </para> + <para> + The reason for the slot's invalidation. <literal>NULL</literal> if the + slot is currently actively being used. The non-NULL values indicate that + the slot is marked as invalidated. Possible values are: + <itemizedlist spacing="compact"> + <listitem> + <para> + <literal>wal_removed</literal> means that the required WAL has been + removed. + </para> + </listitem> + <listitem> + <para> + <literal>rows_removed</literal> means that the required rows have + been removed. + </para> + </listitem> + <listitem> + <para> + <literal>wal_level_insufficient</literal> means that the + primary doesn't have a <xref linkend="guc-wal-level"/> sufficient to + perform logical decoding. + </para> + </listitem> + </itemizedlist> + </para></entry> + </row> + </tbody> </tgroup> </table> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 04227a72d1..c39f0d73d3 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1025,7 +1025,8 @@ CREATE VIEW pg_replication_slots AS L.two_phase, L.conflict_reason, L.failover, - L.synced + L.synced, + L.invalidation_reason FROM pg_get_replication_slots() AS L LEFT JOIN pg_database D ON (L.datoid = D.oid); diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 768a304723..a7a250b7c5 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -239,7 +239,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) Datum pg_get_replication_slots(PG_FUNCTION_ARGS) { -#define PG_GET_REPLICATION_SLOTS_COLS 17 +#define PG_GET_REPLICATION_SLOTS_COLS 18 ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; XLogRecPtr currlsn; int slotno; @@ -263,6 +263,7 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) bool nulls[PG_GET_REPLICATION_SLOTS_COLS]; WALAvailability walstate; int i; + ReplicationSlotInvalidationCause cause; if (!slot->in_use) continue; @@ -409,12 +410,12 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) values[i++] = BoolGetDatum(slot_contents.data.two_phase); + cause = slot_contents.data.invalidated; + if (slot_contents.data.database == InvalidOid) nulls[i++] = true; else { - ReplicationSlotInvalidationCause cause = slot_contents.data.invalidated; - if (cause == RS_INVAL_NONE) nulls[i++] = true; else @@ -425,6 +426,11 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) values[i++] = BoolGetDatum(slot_contents.data.synced); + if (cause == RS_INVAL_NONE) + nulls[i++] = true; + else + values[i++] = CStringGetTextDatum(SlotInvalidationCauses[cause]); + Assert(i == PG_GET_REPLICATION_SLOTS_COLS); tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 291ed876fc..17eae8847b 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11120,9 +11120,9 @@ proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f', proretset => 't', provolatile => 's', prorettype => 'record', proargtypes => '', - proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool}', - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced}', + proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason}', prosrc => 'pg_get_replication_slots' }, { oid => '3786', descr => 'set up a logical replication slot', proname => 'pg_create_logical_replication_slot', provolatile => 'v', diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 0cd2c64fca..e77bb36afe 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1475,8 +1475,9 @@ pg_replication_slots| SELECT l.slot_name, l.two_phase, l.conflict_reason, l.failover, - l.synced - FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced) + l.synced, + l.invalidation_reason + FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason) LEFT JOIN pg_database d ON ((l.datoid = d.oid))); pg_roles| SELECT pg_authid.rolname, pg_authid.rolsuper, -- 2.34.1 [application/x-patch] v7-0002-Add-XID-based-replication-slot-invalidation.patch (12.8K, ../../CALj2ACXBz9HG767ink9nQdz0n6EdZz5g_qutek2ffTP6LNDbGw@mail.gmail.com/3-v7-0002-Add-XID-based-replication-slot-invalidation.patch) download | inline diff: From af48594e7a99277219384f0da702f26a48527aa3 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Tue, 5 Mar 2024 18:57:25 +0000 Subject: [PATCH v7 2/4] Add XID based replication slot invalidation Currently postgres has the ability to invalidate inactive replication slots based on the amount of WAL (set via max_slot_wal_keep_size GUC) that will be needed for the slots in case they become active. However, choosing a default value for max_slot_wal_keep_size is tricky. Because the amount of WAL a customer generates, and their allocated storage will vary greatly in production, making it difficult to pin down a one-size-fits-all value. It is often easy for developers to set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5 billion, after which the slots get invalidated. To achieve the above, postgres uses replication slot xmin (the oldest transaction that this slot needs the database to retain) or catalog_xmin (the oldest transaction affecting the system catalogs that this slot needs the database to retain), and a new GUC max_slot_xid_age. The checkpointer then looks at all replication slots invalidating the slots based on the age set. --- doc/src/sgml/config.sgml | 21 ++++ src/backend/access/transam/xlog.c | 10 ++ src/backend/replication/slot.c | 44 ++++++- src/backend/utils/misc/guc_tables.c | 10 ++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/replication/slot.h | 3 + src/test/recovery/meson.build | 1 + src/test/recovery/t/050_invalidate_slots.pl | 108 ++++++++++++++++++ 8 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/050_invalidate_slots.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index b38cbd714a..7a8360cd32 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4544,6 +4544,27 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows </listitem> </varlistentry> + <varlistentry id="guc-max-slot-xid-age" xreflabel="max_slot_xid_age"> + <term><varname>max_slot_xid_age</varname> (<type>integer</type>) + <indexterm> + <primary><varname>max_slot_xid_age</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Invalidate replication slots whose <literal>xmin</literal> (the oldest + transaction that this slot needs the database to retain) or + <literal>catalog_xmin</literal> (the oldest transaction affecting the + system catalogs that this slot needs the database to retain) has reached + the age specified by this setting. A value of zero (which is default) + disables this feature. Users can set this value anywhere from zero to + two billion. This parameter can only be set in the + <filename>postgresql.conf</filename> file or on the server command + line. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp"> <term><varname>track_commit_timestamp</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 20a5f86209..36ae2ac6a4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7147,6 +7147,11 @@ CreateCheckPoint(int flags) if (PriorRedoPtr != InvalidXLogRecPtr) UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr); + /* Invalidate replication slots based on xmin or catalog_xmin age */ + if (max_slot_xid_age > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, + InvalidOid, InvalidTransactionId); + /* * Delete old log files, those no longer needed for last checkpoint to * prevent the disk holding the xlog from growing full. @@ -7597,6 +7602,11 @@ CreateRestartPoint(int flags) */ XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size); + /* Invalidate replication slots based on xmin or catalog_xmin age */ + if (max_slot_xid_age > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, + InvalidOid, InvalidTransactionId); + /* * Retreat _logSegNo using the current end of xlog replayed or received, * whichever is later. diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 2614f98ddd..febe57ff47 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -85,10 +85,11 @@ const char *const SlotInvalidationCauses[] = { [RS_INVAL_WAL_REMOVED] = "wal_removed", [RS_INVAL_HORIZON] = "rows_removed", [RS_INVAL_WAL_LEVEL] = "wal_level_insufficient", + [RS_INVAL_XID_AGE] = "xid_aged", }; /* Maximum number of invalidation causes */ -#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL +#define RS_INVAL_MAX_CAUSES RS_INVAL_XID_AGE StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), "array length mismatch"); @@ -118,6 +119,7 @@ ReplicationSlot *MyReplicationSlot = NULL; /* GUC variable */ int max_replication_slots = 10; /* the maximum number of replication * slots */ +int max_slot_xid_age = 0; static void ReplicationSlotShmemExit(int code, Datum arg); static void ReplicationSlotDropPtr(ReplicationSlot *slot); @@ -1446,6 +1448,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause, case RS_INVAL_WAL_LEVEL: appendStringInfoString(&err_detail, _("Logical decoding on standby requires wal_level >= logical on the primary server.")); break; + case RS_INVAL_XID_AGE: + appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age.")); + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1562,6 +1567,42 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, if (SlotIsLogical(s)) conflict = cause; break; + case RS_INVAL_XID_AGE: + { + TransactionId xid_cur = ReadNextTransactionId(); + TransactionId xid_limit; + TransactionId xid_slot; + + if (TransactionIdIsNormal(s->data.xmin)) + { + xid_slot = s->data.xmin; + + xid_limit = xid_slot + max_slot_xid_age; + if (xid_limit < FirstNormalTransactionId) + xid_limit += FirstNormalTransactionId; + + if (TransactionIdFollowsOrEquals(xid_cur, xid_limit)) + { + conflict = cause; + break; + } + } + if (TransactionIdIsNormal(s->data.catalog_xmin)) + { + xid_slot = s->data.catalog_xmin; + + xid_limit = xid_slot + max_slot_xid_age; + if (xid_limit < FirstNormalTransactionId) + xid_limit += FirstNormalTransactionId; + + if (TransactionIdFollowsOrEquals(xid_cur, xid_limit)) + { + conflict = cause; + break; + } + } + } + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1716,6 +1757,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given * db; dboid may be InvalidOid for shared relations * - RS_INVAL_WAL_LEVEL: is logical + * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age * * NB - this runs as part of checkpoint, so avoid raising errors if possible. */ diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 45013582a7..3ed642dcaf 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2954,6 +2954,16 @@ struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"max_slot_xid_age", PGC_SIGHUP, REPLICATION_SENDING, + gettext_noop("Age of the transaction ID at which a replication slot gets invalidated."), + gettext_noop("The transaction is the oldest transaction (including the one affecting the system catalogs) that a replication slot needs the database to retain.") + }, + &max_slot_xid_age, + 0, 0, 2000000000, + NULL, NULL, NULL + }, + { {"commit_delay", PGC_SUSET, WAL_SETTINGS, gettext_noop("Sets the delay in microseconds between transaction commit and " diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index edcc0282b2..50019d7c25 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -334,6 +334,7 @@ #wal_sender_timeout = 60s # in milliseconds; 0 disables #track_commit_timestamp = off # collect timestamp of transaction commit # (change requires restart) +#max_slot_xid_age = 0 # - Primary Server - diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index acbf567150..ad9fd1e94b 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -53,6 +53,8 @@ typedef enum ReplicationSlotInvalidationCause RS_INVAL_HORIZON, /* wal_level insufficient for slot */ RS_INVAL_WAL_LEVEL, + /* slot's xmin or catalog_xmin has reached the age */ + RS_INVAL_XID_AGE, } ReplicationSlotInvalidationCause; extern PGDLLIMPORT const char *const SlotInvalidationCauses[]; @@ -226,6 +228,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot; /* GUCs */ extern PGDLLIMPORT int max_replication_slots; +extern PGDLLIMPORT int max_slot_xid_age; /* shmem initialization functions */ extern Size ReplicationSlotsShmemSize(void); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index c67249500e..d698c3ec73 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -50,6 +50,7 @@ tests += { 't/039_end_of_wal.pl', 't/040_standby_failover_slots_sync.pl', 't/041_checkpoint_at_promote.pl', + 't/050_invalidate_slots.pl', ], }, } diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl new file mode 100644 index 0000000000..2f482b56e8 --- /dev/null +++ b/src/test/recovery/t/050_invalidate_slots.pl @@ -0,0 +1,108 @@ +# Copyright (c) 2024, PostgreSQL Global Development Group + +# Test for replication slots invalidation +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; +use Time::HiRes qw(usleep); + +# Initialize primary node, setting wal-segsize to 1MB +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']); +$primary->append_conf( + 'postgresql.conf', q{ +checkpoint_timeout = 1h +}); +$primary->start; +$primary->safe_psql( + 'postgres', qq[ + SELECT pg_create_physical_replication_slot('sb1_slot'); +]); + +# Take backup +my $backup_name = 'my_backup'; +$primary->backup($backup_name); + +# Create a standby linking to the primary using the replication slot +my $standby1 = PostgreSQL::Test::Cluster->new('standby1'); +$standby1->init_from_backup($primary, $backup_name, has_streaming => 1); + +# Enable hs_feedback. The slot should gain an xmin. We set the status interval +# so we'll see the results promptly. +$standby1->append_conf( + 'postgresql.conf', q{ +primary_slot_name = 'sb1_slot' +hot_standby_feedback = on +wal_receiver_status_interval = 1 +}); +$standby1->start; + +# Create some content on primary to move xmin +$primary->safe_psql('postgres', + "CREATE TABLE tab_int AS SELECT generate_series(1,10) AS a"); + +# Wait until standby has replayed enough data +$primary->wait_for_catchup($standby1); + +$primary->poll_query_until( + 'postgres', qq[ + SELECT xmin IS NOT NULL + FROM pg_catalog.pg_replication_slots + WHERE slot_name = 'sb1_slot'; +]) or die "Timed out waiting for slot xmin to advance"; + +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET max_slot_xid_age = 500; +]); +$primary->reload; + +# Stop standby to make the replication slot's xmin on primary to age +$standby1->stop; + +my $logstart = -s $primary->logfile; + +# Do some work to advance xmin +$primary->safe_psql( + 'postgres', q{ +do $$ +begin + for i in 10000..11000 loop + -- use an exception block so that each iteration eats an XID + begin + insert into tab_int values (i); + exception + when division_by_zero then null; + end; + end loop; +end$$; +}); + +my $invalidated = 0; +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) +{ + $primary->safe_psql('postgres', "CHECKPOINT"); + if ($primary->log_contains( + 'invalidating obsolete replication slot "sb1_slot"', $logstart)) + { + $invalidated = 1; + last; + } + usleep(100_000); +} +ok($invalidated, 'check that slot sb1_slot invalidation has been logged'); + +# Wait for the inactive replication slots to be invalidated. +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE slot_name = 'sb1_slot' AND + invalidation_reason = 'xid_aged'; +]) + or die + "Timed out while waiting for replication slot sb1_slot to be invalidated"; + +done_testing(); -- 2.34.1 [application/x-patch] v7-0003-Track-inactive-replication-slot-information.patch (9.9K, ../../CALj2ACXBz9HG767ink9nQdz0n6EdZz5g_qutek2ffTP6LNDbGw@mail.gmail.com/4-v7-0003-Track-inactive-replication-slot-information.patch) download | inline diff: From c4501bcffd6149174245d16b00ca2571e46bb6cb Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Tue, 5 Mar 2024 18:57:42 +0000 Subject: [PATCH v7 3/4] Track inactive replication slot information Currently postgres doesn't track metrics like the time at which the slot became inactive, and the total number of times the slot became inactive in its lifetime. This commit adds two new metrics last_inactive_at of type timestamptz and inactive_count of type numeric to ReplicationSlotPersistentData. Whenever a slot becomes inactive, the current timestamp and inactive count are persisted to disk. These metrics are useful in the following ways: - To improve replication slot monitoring tools. For instance, one can build a monitoring tool that signals a) when replication slots is lying inactive for a day or so using last_inactive_at metric, b) when a replication slot is becoming inactive too frequently using last_inactive_at metric. - To implement timeout-based inactive replication slot management capability in postgres. Increases SLOT_VERSION due to the added two new metrics. --- doc/src/sgml/system-views.sgml | 20 +++++++++++++ src/backend/catalog/system_views.sql | 4 ++- src/backend/replication/slot.c | 43 ++++++++++++++++++++++------ src/backend/replication/slotfuncs.c | 15 +++++++++- src/include/catalog/pg_proc.dat | 6 ++-- src/include/replication/slot.h | 6 ++++ src/test/regress/expected/rules.out | 6 ++-- 7 files changed, 84 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index cce88c14bb..0dfd472b02 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2771,6 +2771,26 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx ID of role </para></entry> </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_inactive_at</structfield> <type>timestamptz</type> + </para> + <para> + The time at which the slot became inactive. + <literal>NULL</literal> if the slot is currently actively being + used. + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>inactive_count</structfield> <type>numeric</type> + </para> + <para> + The total number of times the slot became inactive in its lifetime. + </para></entry> + </row> </tbody> </tgroup> </table> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index c39f0d73d3..a5a78a9910 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1026,7 +1026,9 @@ CREATE VIEW pg_replication_slots AS L.conflict_reason, L.failover, L.synced, - L.invalidation_reason + L.invalidation_reason, + L.last_inactive_at, + L.inactive_count FROM pg_get_replication_slots() AS L LEFT JOIN pg_database D ON (L.datoid = D.oid); diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index febe57ff47..8066ea3b28 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -108,7 +108,7 @@ StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), sizeof(ReplicationSlotOnDisk) - ReplicationSlotOnDiskConstantSize #define SLOT_MAGIC 0x1051CA1 /* format identifier */ -#define SLOT_VERSION 5 /* version for new files */ +#define SLOT_VERSION 6 /* version for new files */ /* Control array for replication slot management */ ReplicationSlotCtlData *ReplicationSlotCtl = NULL; @@ -363,6 +363,8 @@ ReplicationSlotCreate(const char *name, bool db_specific, slot->data.two_phase_at = InvalidXLogRecPtr; slot->data.failover = failover; slot->data.synced = synced; + slot->data.last_inactive_at = 0; + slot->data.inactive_count = 0; /* and then data only present in shared memory */ slot->just_dirtied = false; @@ -589,6 +591,17 @@ retry: if (am_walsender) { + if (s->data.persistency == RS_PERSISTENT) + { + SpinLockAcquire(&s->mutex); + s->data.last_inactive_at = 0; + SpinLockRelease(&s->mutex); + + /* Write this slot to disk */ + ReplicationSlotMarkDirty(); + ReplicationSlotSave(); + } + ereport(log_replication_commands ? LOG : DEBUG1, SlotIsLogical(s) ? errmsg("acquired logical replication slot \"%s\"", @@ -656,16 +669,20 @@ ReplicationSlotRelease(void) ConditionVariableBroadcast(&slot->active_cv); } - MyReplicationSlot = NULL; - - /* might not have been set when we've been a plain slot */ - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); - MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; - ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; - LWLockRelease(ProcArrayLock); - if (am_walsender) { + if (slot->data.persistency == RS_PERSISTENT) + { + SpinLockAcquire(&slot->mutex); + slot->data.last_inactive_at = GetCurrentTimestamp(); + slot->data.inactive_count++; + SpinLockRelease(&slot->mutex); + + /* Write this slot to disk */ + ReplicationSlotMarkDirty(); + ReplicationSlotSave(); + } + ereport(log_replication_commands ? LOG : DEBUG1, is_logical ? errmsg("released logical replication slot \"%s\"", @@ -675,6 +692,14 @@ ReplicationSlotRelease(void) pfree(slotname); } + + MyReplicationSlot = NULL; + + /* might not have been set when we've been a plain slot */ + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; + ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; + LWLockRelease(ProcArrayLock); } /* diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index a7a250b7c5..3bb4e9223e 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -239,10 +239,11 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) Datum pg_get_replication_slots(PG_FUNCTION_ARGS) { -#define PG_GET_REPLICATION_SLOTS_COLS 18 +#define PG_GET_REPLICATION_SLOTS_COLS 20 ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; XLogRecPtr currlsn; int slotno; + char buf[256]; /* * We don't require any special permission to see this function's data @@ -431,6 +432,18 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) else values[i++] = CStringGetTextDatum(SlotInvalidationCauses[cause]); + if (slot_contents.data.last_inactive_at > 0) + values[i++] = TimestampTzGetDatum(slot_contents.data.last_inactive_at); + else + nulls[i++] = true; + + /* Convert to numeric. */ + snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count); + values[i++] = DirectFunctionCall3(numeric_in, + CStringGetDatum(buf), + ObjectIdGetDatum(0), + Int32GetDatum(-1)); + Assert(i == PG_GET_REPLICATION_SLOTS_COLS); tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 17eae8847b..681e329293 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11120,9 +11120,9 @@ proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f', proretset => 't', provolatile => 's', prorettype => 'record', proargtypes => '', - proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text}', - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason}', + proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,text,timestamptz,numeric}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced,invalidation_reason,last_inactive_at,inactive_count}', prosrc => 'pg_get_replication_slots' }, { oid => '3786', descr => 'set up a logical replication slot', proname => 'pg_create_logical_replication_slot', provolatile => 'v', diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index ad9fd1e94b..83b47425ea 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -129,6 +129,12 @@ typedef struct ReplicationSlotPersistentData * for logical slots on the primary server. */ bool failover; + + /* When did this slot become inactive last time? */ + TimestampTz last_inactive_at; + + /* How many times the slot has been inactive? */ + uint64 inactive_count; } ReplicationSlotPersistentData; /* diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index e77bb36afe..b451c324f9 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1476,8 +1476,10 @@ pg_replication_slots| SELECT l.slot_name, l.conflict_reason, l.failover, l.synced, - l.invalidation_reason - FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason) + l.invalidation_reason, + l.last_inactive_at, + l.inactive_count + FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced, invalidation_reason, last_inactive_at, inactive_count) LEFT JOIN pg_database d ON ((l.datoid = d.oid))); pg_roles| SELECT pg_authid.rolname, pg_authid.rolsuper, -- 2.34.1 [application/x-patch] v7-0004-Add-inactive_timeout-based-replication-slot-inval.patch (11.3K, ../../CALj2ACXBz9HG767ink9nQdz0n6EdZz5g_qutek2ffTP6LNDbGw@mail.gmail.com/5-v7-0004-Add-inactive_timeout-based-replication-slot-inval.patch) download | inline diff: From d29ac5e3004dc512300c9d07dc9d026ba979d066 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Tue, 5 Mar 2024 18:59:05 +0000 Subject: [PATCH v7 4/4] Add inactive_timeout based replication slot invalidation Currently postgres has the ability to invalidate inactive replication slots based on the amount of WAL (set via max_slot_wal_keep_size GUC) that will be needed for the slots in case they become active. However, choosing a default value for max_slot_wal_keep_size is tricky. Because the amount of WAL a customer generates, and their allocated storage will vary greatly in production, making it difficult to pin down a one-size-fits-all value. It is often easy for developers to set a timeout of say 1 or 2 or 3 days, after which the inactive slots get dropped. To achieve the above, postgres uses replication slot metric inactive_at (the time at which the slot became inactive), and a new GUC inactive_replication_slot_timeout. The checkpointer then looks at all replication slots invalidating the inactive slots based on the timeout set. --- doc/src/sgml/config.sgml | 18 +++++ src/backend/access/transam/xlog.c | 10 +++ src/backend/replication/slot.c | 22 +++++- src/backend/utils/misc/guc_tables.c | 12 +++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/replication/slot.h | 3 + src/test/recovery/t/050_invalidate_slots.pl | 79 +++++++++++++++++++ 7 files changed, 144 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7a8360cd32..f5c299ef73 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4565,6 +4565,24 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows </listitem> </varlistentry> + <varlistentry id="guc-inactive-replication-slot-timeout" xreflabel="inactive_replication_slot_timeout"> + <term><varname>inactive_replication_slot_timeout</varname> (<type>integer</type>) + <indexterm> + <primary><varname>inactive_replication_slot_timeout</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Invalidate replication slots that are inactive for longer than this + amount of time at the next checkpoint. If this value is specified + without units, it is taken as seconds. A value of zero (which is + default) disables the timeout mechanism. This parameter can only be + set in the <filename>postgresql.conf</filename> file or on the server + command line. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp"> <term><varname>track_commit_timestamp</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 36ae2ac6a4..166c3ed794 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7152,6 +7152,11 @@ CreateCheckPoint(int flags) InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, InvalidOid, InvalidTransactionId); + /* Invalidate inactive replication slots based on timeout */ + if (inactive_replication_slot_timeout > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0, + InvalidOid, InvalidTransactionId); + /* * Delete old log files, those no longer needed for last checkpoint to * prevent the disk holding the xlog from growing full. @@ -7607,6 +7612,11 @@ CreateRestartPoint(int flags) InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, InvalidOid, InvalidTransactionId); + /* Invalidate inactive replication slots based on timeout */ + if (inactive_replication_slot_timeout > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0, + InvalidOid, InvalidTransactionId); + /* * Retreat _logSegNo using the current end of xlog replayed or received, * whichever is later. diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 8066ea3b28..060cb7d66e 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -86,10 +86,11 @@ const char *const SlotInvalidationCauses[] = { [RS_INVAL_HORIZON] = "rows_removed", [RS_INVAL_WAL_LEVEL] = "wal_level_insufficient", [RS_INVAL_XID_AGE] = "xid_aged", + [RS_INVAL_INACTIVE_TIMEOUT] = "inactive_timeout", }; /* Maximum number of invalidation causes */ -#define RS_INVAL_MAX_CAUSES RS_INVAL_XID_AGE +#define RS_INVAL_MAX_CAUSES RS_INVAL_INACTIVE_TIMEOUT StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), "array length mismatch"); @@ -120,6 +121,7 @@ ReplicationSlot *MyReplicationSlot = NULL; int max_replication_slots = 10; /* the maximum number of replication * slots */ int max_slot_xid_age = 0; +int inactive_replication_slot_timeout = 0; static void ReplicationSlotShmemExit(int code, Datum arg); static void ReplicationSlotDropPtr(ReplicationSlot *slot); @@ -1476,6 +1478,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause, case RS_INVAL_XID_AGE: appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age.")); break; + case RS_INVAL_INACTIVE_TIMEOUT: + appendStringInfoString(&err_detail, _("The slot has been inactive for more than the time specified by inactive_replication_slot_timeout.")); + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1628,6 +1633,20 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, } } break; + case RS_INVAL_INACTIVE_TIMEOUT: + if (s->data.last_inactive_at > 0) + { + TimestampTz now; + + Assert(s->data.persistency == RS_PERSISTENT); + Assert(s->active_pid == 0); + + now = GetCurrentTimestamp(); + if (TimestampDifferenceExceeds(s->data.last_inactive_at, now, + inactive_replication_slot_timeout * 1000)) + conflict = cause; + } + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1783,6 +1802,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, * db; dboid may be InvalidOid for shared relations * - RS_INVAL_WAL_LEVEL: is logical * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age + * - RS_INVAL_INACTIVE_TIMEOUT: inactive slot timeout occurs * * NB - this runs as part of checkpoint, so avoid raising errors if possible. */ diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 3ed642dcaf..06e3e87f4a 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2964,6 +2964,18 @@ struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"inactive_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING, + gettext_noop("Sets the amount of time to wait before invalidating an " + "inactive replication slot."), + NULL, + GUC_UNIT_S + }, + &inactive_replication_slot_timeout, + 0, 0, INT_MAX, + NULL, NULL, NULL + }, + { {"commit_delay", PGC_SUSET, WAL_SETTINGS, gettext_noop("Sets the delay in microseconds between transaction commit and " diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 50019d7c25..092aaf1bec 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -261,6 +261,7 @@ #recovery_prefetch = try # prefetch pages referenced in the WAL? #wal_decode_buffer_size = 512kB # lookahead window used for prefetching # (change requires restart) +#inactive_replication_slot_timeout = 0 # in seconds; 0 disables # - Archiving - diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 83b47425ea..708cbee324 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -55,6 +55,8 @@ typedef enum ReplicationSlotInvalidationCause RS_INVAL_WAL_LEVEL, /* slot's xmin or catalog_xmin has reached the age */ RS_INVAL_XID_AGE, + /* inactive slot timeout has occurred */ + RS_INVAL_INACTIVE_TIMEOUT, } ReplicationSlotInvalidationCause; extern PGDLLIMPORT const char *const SlotInvalidationCauses[]; @@ -235,6 +237,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot; /* GUCs */ extern PGDLLIMPORT int max_replication_slots; extern PGDLLIMPORT int max_slot_xid_age; +extern PGDLLIMPORT int inactive_replication_slot_timeout; /* shmem initialization functions */ extern Size ReplicationSlotsShmemSize(void); diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl index 2f482b56e8..4c66dd4a4e 100644 --- a/src/test/recovery/t/050_invalidate_slots.pl +++ b/src/test/recovery/t/050_invalidate_slots.pl @@ -105,4 +105,83 @@ $primary->poll_query_until( or die "Timed out while waiting for replication slot sb1_slot to be invalidated"; +$primary->safe_psql( + 'postgres', qq[ + SELECT pg_create_physical_replication_slot('sb2_slot'); +]); + +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET max_slot_xid_age = 0; +]); +$primary->reload; + +# Create a standby linking to the primary using the replication slot +my $standby2 = PostgreSQL::Test::Cluster->new('standby2'); +$standby2->init_from_backup($primary, $backup_name, has_streaming => 1); +$standby2->append_conf( + 'postgresql.conf', q{ +primary_slot_name = 'sb2_slot' +}); +$standby2->start; + +# Wait until standby has replayed enough data +$primary->wait_for_catchup($standby2); + +# The inactive replication slot info should be null when the slot is active +my $result = $primary->safe_psql( + 'postgres', qq[ + SELECT last_inactive_at IS NULL, inactive_count = 0 AS OK + FROM pg_replication_slots WHERE slot_name = 'sb2_slot'; +]); +is($result, "t|t", + 'check the inactive replication slot info for an active slot'); + +# Set timeout so that the next checkpoint will invalidate the inactive +# replication slot. +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET inactive_replication_slot_timeout TO '1s'; +]); +$primary->reload; + +$logstart = -s $primary->logfile; + +# Stop standby to make the replication slot on primary inactive +$standby2->stop; + +# Wait for the inactive replication slot info to be updated +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE last_inactive_at IS NOT NULL AND + inactive_count = 1 AND slot_name = 'sb2_slot'; +]) + or die + "Timed out while waiting for inactive replication slot info to be updated"; + +$invalidated = 0; +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) +{ + $primary->safe_psql('postgres', "CHECKPOINT"); + if ($primary->log_contains( + 'invalidating obsolete replication slot "sb2_slot"', $logstart)) + { + $invalidated = 1; + last; + } + usleep(100_000); +} +ok($invalidated, 'check that slot sb2_slot invalidation has been logged'); + +# Wait for the inactive replication slots to be invalidated. +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE slot_name = 'sb2_slot' AND + invalidation_reason = 'inactive_timeout'; +]) + or die + "Timed out while waiting for inactive replication slot sb2_slot to be invalidated"; + done_testing(); -- 2.34.1 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-05 19:44 Nathan Bossart <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Nathan Bossart @ 2024-03-05 19:44 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 06, 2024 at 12:50:38AM +0530, Bharath Rupireddy wrote: > On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot > <[email protected]> wrote: >> On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: >> > Unless I am misinterpreting some details, ISTM we could rename this column >> > to invalidation_reason and use it for both logical and physical slots. I'm >> > not seeing a strong need for another column. >> >> Yeah having two columns was more for convenience purpose. Without the "conflict" >> one, a slot conflicting with recovery would be "a logical slot having a non NULL >> invalidation_reason". >> >> I'm also fine with one column if most of you prefer that way. > > While we debate on the above, please find the attached v7 patch set > after rebasing. It looks like Bertrand is okay with reusing the same column for both logical and physical slots, which IIUC is what you initially proposed in v1 of the patch set. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-06 09:12 Bertrand Drouvot <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-06 09:12 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Tue, Mar 05, 2024 at 01:44:43PM -0600, Nathan Bossart wrote: > On Wed, Mar 06, 2024 at 12:50:38AM +0530, Bharath Rupireddy wrote: > > On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot > > <[email protected]> wrote: > >> On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > >> > Unless I am misinterpreting some details, ISTM we could rename this column > >> > to invalidation_reason and use it for both logical and physical slots. I'm > >> > not seeing a strong need for another column. > >> > >> Yeah having two columns was more for convenience purpose. Without the "conflict" > >> one, a slot conflicting with recovery would be "a logical slot having a non NULL > >> invalidation_reason". > >> > >> I'm also fine with one column if most of you prefer that way. > > > > While we debate on the above, please find the attached v7 patch set > > after rebasing. > > It looks like Bertrand is okay with reusing the same column for both > logical and physical slots Yeah, I'm okay with one column. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-06 09:16 Bharath Rupireddy <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 3 replies; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-06 09:16 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 6, 2024 at 2:42 PM Bertrand Drouvot <[email protected]> wrote: > > Hi, > > On Tue, Mar 05, 2024 at 01:44:43PM -0600, Nathan Bossart wrote: > > On Wed, Mar 06, 2024 at 12:50:38AM +0530, Bharath Rupireddy wrote: > > > On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot > > > <[email protected]> wrote: > > >> On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > > >> > Unless I am misinterpreting some details, ISTM we could rename this column > > >> > to invalidation_reason and use it for both logical and physical slots. I'm > > >> > not seeing a strong need for another column. > > >> > > >> Yeah having two columns was more for convenience purpose. Without the "conflict" > > >> one, a slot conflicting with recovery would be "a logical slot having a non NULL > > >> invalidation_reason". > > >> > > >> I'm also fine with one column if most of you prefer that way. > > > > > > While we debate on the above, please find the attached v7 patch set > > > after rebasing. > > > > It looks like Bertrand is okay with reusing the same column for both > > logical and physical slots > > Yeah, I'm okay with one column. Thanks. v8-0001 is how it looks. Please see the v8 patch set with this change. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] v8-0001-Track-invalidation_reason-in-pg_replication_slots.patch (17.2K, ../../CALj2ACWgm++=TkXWo9spm+7ibGxcYL3SsYtT4tbW3=HT1VYs0Q@mail.gmail.com/2-v8-0001-Track-invalidation_reason-in-pg_replication_slots.patch) download | inline diff: From a03f366f5e14a4db7cf3f89f1adf8a311490651b Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 6 Mar 2024 08:44:29 +0000 Subject: [PATCH v8 1/4] Track invalidation_reason in pg_replication_slots Currently the reason for replication slot invalidation is not tracked in pg_replication_slots. A recent commit 007693f2a added conflict_reason to show the reasons for slot invalidation, but only for logical slots. This commit renames conflict_reason to invalidation_reason, and adds the support to show invalidation reasons for both physical and logical slots. --- doc/src/sgml/ref/pgupgrade.sgml | 2 +- doc/src/sgml/system-views.sgml | 11 ++-- src/backend/catalog/system_views.sql | 2 +- src/backend/replication/logical/slotsync.c | 2 +- src/backend/replication/slot.c | 6 +-- src/backend/replication/slotfuncs.c | 11 +--- src/bin/pg_upgrade/info.c | 4 +- src/include/catalog/pg_proc.dat | 2 +- src/include/replication/slot.h | 2 +- .../t/035_standby_logical_decoding.pl | 50 +++++++++---------- .../t/040_standby_failover_slots_sync.pl | 4 +- src/test/regress/expected/rules.out | 4 +- 12 files changed, 47 insertions(+), 53 deletions(-) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index 58c6c2df8b..50d13f3c1e 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -453,7 +453,7 @@ make prefix=/usr/local/pgsql.new install <para> All slots on the old cluster must be usable, i.e., there are no slots whose - <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>conflict_reason</structfield> + <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>invalidation_reason</structfield> is not <literal>NULL</literal>. </para> </listitem> diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index be90edd0e2..c519b4a7f8 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2525,13 +2525,14 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx <row> <entry role="catalog_table_entry"><para role="column_definition"> - <structfield>conflict_reason</structfield> <type>text</type> + <structfield>invalidation_reason</structfield> <type>text</type> </para> <para> - The reason for the logical slot's conflict with recovery. It is always - NULL for physical slots, as well as for logical slots which are not - invalidated. The non-NULL values indicate that the slot is marked - as invalidated. Possible values are: + The reason for the slot's invalidation. <literal>NULL</literal> if the + slot is currently actively being used. The non-NULL values indicate that + the slot is marked as invalidated. In case of logical slots, it + represents the reason for the logical slot's conflict with recovery. + Possible values are: <itemizedlist spacing="compact"> <listitem> <para> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 04227a72d1..1dbfcef9f1 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1023,7 +1023,7 @@ CREATE VIEW pg_replication_slots AS L.wal_status, L.safe_wal_size, L.two_phase, - L.conflict_reason, + L.invalidation_reason, L.failover, L.synced FROM pg_get_replication_slots() AS L diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index ad0fc6a04b..80ffc24213 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -664,7 +664,7 @@ synchronize_slots(WalReceiverConn *wrconn) bool started_tx = false; const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn," " restart_lsn, catalog_xmin, two_phase, failover," - " database, conflict_reason" + " database, invalidation_reason" " FROM pg_catalog.pg_replication_slots" " WHERE failover and NOT temporary"; diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 02ae27499b..b0f48229cb 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -2333,17 +2333,17 @@ RestoreSlotFromDisk(const char *name) * ReplicationSlotInvalidationCause. */ ReplicationSlotInvalidationCause -GetSlotInvalidationCause(const char *conflict_reason) +GetSlotInvalidationCause(const char *invalidation_reason) { ReplicationSlotInvalidationCause cause; ReplicationSlotInvalidationCause result = RS_INVAL_NONE; bool found PG_USED_FOR_ASSERTS_ONLY = false; - Assert(conflict_reason); + Assert(invalidation_reason); for (cause = RS_INVAL_NONE; cause <= RS_INVAL_MAX_CAUSES; cause++) { - if (strcmp(SlotInvalidationCauses[cause], conflict_reason) == 0) + if (strcmp(SlotInvalidationCauses[cause], invalidation_reason) == 0) { found = true; result = cause; diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 768a304723..758498d29d 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -409,17 +409,10 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) values[i++] = BoolGetDatum(slot_contents.data.two_phase); - if (slot_contents.data.database == InvalidOid) + if (slot_contents.data.invalidated == RS_INVAL_NONE) nulls[i++] = true; else - { - ReplicationSlotInvalidationCause cause = slot_contents.data.invalidated; - - if (cause == RS_INVAL_NONE) - nulls[i++] = true; - else - values[i++] = CStringGetTextDatum(SlotInvalidationCauses[cause]); - } + values[i++] = CStringGetTextDatum(SlotInvalidationCauses[slot_contents.data.invalidated]); values[i++] = BoolGetDatum(slot_contents.data.failover); diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index 183c2f84eb..9683c91d4a 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -667,13 +667,13 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check) * removed. */ res = executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase, failover, " - "%s as caught_up, conflict_reason IS NOT NULL as invalid " + "%s as caught_up, invalidation_reason IS NOT NULL as invalid " "FROM pg_catalog.pg_replication_slots " "WHERE slot_type = 'logical' AND " "database = current_database() AND " "temporary IS FALSE;", live_check ? "FALSE" : - "(CASE WHEN conflict_reason IS NOT NULL THEN FALSE " + "(CASE WHEN invalidation_reason IS NOT NULL THEN FALSE " "ELSE (SELECT pg_catalog.binary_upgrade_logical_slot_has_caught_up(slot_name)) " "END)"); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 291ed876fc..69140a0bf0 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11122,7 +11122,7 @@ proargtypes => '', proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool}', proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,conflict_reason,failover,synced}', + proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,invalidation_reason,failover,synced}', prosrc => 'pg_get_replication_slots' }, { oid => '3786', descr => 'set up a logical replication slot', proname => 'pg_create_logical_replication_slot', provolatile => 'v', diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index acbf567150..02a96b0e19 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -272,6 +272,6 @@ extern void CheckPointReplicationSlots(bool is_shutdown); extern void CheckSlotRequirements(void); extern void CheckSlotPermissions(void); extern ReplicationSlotInvalidationCause - GetSlotInvalidationCause(const char *conflict_reason); + GetSlotInvalidationCause(const char *invalidation_reason); #endif /* SLOT_H */ diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl index 2659d4bb52..a02ae84991 100644 --- a/src/test/recovery/t/035_standby_logical_decoding.pl +++ b/src/test/recovery/t/035_standby_logical_decoding.pl @@ -168,8 +168,8 @@ sub change_hot_standby_feedback_and_wait_for_xmins } } -# Check conflict_reason in pg_replication_slots. -sub check_slots_conflict_reason +# Check invalidation_reason in pg_replication_slots. +sub check_slots_invalidation_reason { my ($slot_prefix, $reason) = @_; @@ -178,15 +178,15 @@ sub check_slots_conflict_reason $res = $node_standby->safe_psql( 'postgres', qq( - select conflict_reason from pg_replication_slots where slot_name = '$active_slot';)); + select invalidation_reason from pg_replication_slots where slot_name = '$active_slot';)); - is($res, "$reason", "$active_slot conflict_reason is $reason"); + is($res, "$reason", "$active_slot invalidation_reason is $reason"); $res = $node_standby->safe_psql( 'postgres', qq( - select conflict_reason from pg_replication_slots where slot_name = '$inactive_slot';)); + select invalidation_reason from pg_replication_slots where slot_name = '$inactive_slot';)); - is($res, "$reason", "$inactive_slot conflict_reason is $reason"); + is($res, "$reason", "$inactive_slot invalidation_reason is $reason"); } # Drop the slots, re-create them, change hot_standby_feedback, @@ -293,13 +293,13 @@ $node_primary->safe_psql('testdb', qq[SELECT * FROM pg_create_physical_replication_slot('$primary_slotname');] ); -# Check conflict_reason is NULL for physical slot +# Check invalidation_reason is NULL for physical slot $res = $node_primary->safe_psql( 'postgres', qq[ - SELECT conflict_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';] + SELECT invalidation_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';] ); -is($res, 't', "Physical slot reports conflict_reason as NULL"); +is($res, 't', "Physical slot reports invalidation_reason as NULL"); my $backup_name = 'b1'; $node_primary->backup($backup_name); @@ -512,8 +512,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Check invalidation in the logfile and in pg_stat_database_conflicts check_for_invalidation('vacuum_full_', 1, 'with vacuum FULL on pg_class'); -# Verify conflict_reason is 'rows_removed' in pg_replication_slots -check_slots_conflict_reason('vacuum_full_', 'rows_removed'); +# Verify invalidation_reason is 'rows_removed' in pg_replication_slots +check_slots_invalidation_reason('vacuum_full_', 'rows_removed'); $handle = make_slot_active($node_standby, 'vacuum_full_', 0, \$stdout, \$stderr); @@ -531,8 +531,8 @@ change_hot_standby_feedback_and_wait_for_xmins(1, 1); ################################################## $node_standby->restart; -# Verify conflict_reason is retained across a restart. -check_slots_conflict_reason('vacuum_full_', 'rows_removed'); +# Verify invalidation_reason is retained across a restart. +check_slots_invalidation_reason('vacuum_full_', 'rows_removed'); ################################################## # Verify that invalidated logical slots do not lead to retaining WAL. @@ -540,7 +540,7 @@ check_slots_conflict_reason('vacuum_full_', 'rows_removed'); # Get the restart_lsn from an invalidated slot my $restart_lsn = $node_standby->safe_psql('postgres', - "SELECT restart_lsn from pg_replication_slots WHERE slot_name = 'vacuum_full_activeslot' and conflict_reason is not null;" + "SELECT restart_lsn from pg_replication_slots WHERE slot_name = 'vacuum_full_activeslot' and invalidation_reason is not null;" ); chomp($restart_lsn); @@ -591,8 +591,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Check invalidation in the logfile and in pg_stat_database_conflicts check_for_invalidation('row_removal_', $logstart, 'with vacuum on pg_class'); -# Verify conflict_reason is 'rows_removed' in pg_replication_slots -check_slots_conflict_reason('row_removal_', 'rows_removed'); +# Verify invalidation_reason is 'rows_removed' in pg_replication_slots +check_slots_invalidation_reason('row_removal_', 'rows_removed'); $handle = make_slot_active($node_standby, 'row_removal_', 0, \$stdout, \$stderr); @@ -627,8 +627,8 @@ $node_primary->wait_for_replay_catchup($node_standby); check_for_invalidation('shared_row_removal_', $logstart, 'with vacuum on pg_authid'); -# Verify conflict_reason is 'rows_removed' in pg_replication_slots -check_slots_conflict_reason('shared_row_removal_', 'rows_removed'); +# Verify invalidation_reason is 'rows_removed' in pg_replication_slots +check_slots_invalidation_reason('shared_row_removal_', 'rows_removed'); $handle = make_slot_active($node_standby, 'shared_row_removal_', 0, \$stdout, \$stderr); @@ -680,7 +680,7 @@ ok( $node_standby->poll_query_until( is( $node_standby->safe_psql( 'postgres', q[select bool_or(conflicting) from - (select conflict_reason is not NULL as conflicting + (select invalidation_reason is not NULL as conflicting from pg_replication_slots WHERE slot_type = 'logical')]), 'f', 'Logical slots are reported as non conflicting'); @@ -719,8 +719,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Check invalidation in the logfile and in pg_stat_database_conflicts check_for_invalidation('pruning_', $logstart, 'with on-access pruning'); -# Verify conflict_reason is 'rows_removed' in pg_replication_slots -check_slots_conflict_reason('pruning_', 'rows_removed'); +# Verify invalidation_reason is 'rows_removed' in pg_replication_slots +check_slots_invalidation_reason('pruning_', 'rows_removed'); $handle = make_slot_active($node_standby, 'pruning_', 0, \$stdout, \$stderr); @@ -825,8 +825,8 @@ SKIP: $logstart), "activeslot slot invalidation is logged with injection point"); - # Verify conflict_reason is 'rows_removed' in pg_replication_slots. - check_slots_conflict_reason('injection_', 'rows_removed'); + # Verify invalidation_reason is 'rows_removed' in pg_replication_slots. + check_slots_invalidation_reason('injection_', 'rows_removed'); # Detach from the injection point $node_standby->safe_psql('testdb', @@ -875,8 +875,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Check invalidation in the logfile and in pg_stat_database_conflicts check_for_invalidation('wal_level_', $logstart, 'due to wal_level'); -# Verify conflict_reason is 'wal_level_insufficient' in pg_replication_slots -check_slots_conflict_reason('wal_level_', 'wal_level_insufficient'); +# Verify invalidation_reason is 'wal_level_insufficient' in pg_replication_slots + check_slots_invalidation_reason('wal_level_', 'wal_level_insufficient'); $handle = make_slot_active($node_standby, 'wal_level_', 0, \$stdout, \$stderr); diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl index 021c58f621..2e1d01f750 100644 --- a/src/test/recovery/t/040_standby_failover_slots_sync.pl +++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl @@ -228,7 +228,7 @@ $standby1->safe_psql('postgres', "CHECKPOINT"); # Check if the synced slot is invalidated is( $standby1->safe_psql( 'postgres', - q{SELECT conflict_reason = 'wal_removed' FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';} + q{SELECT invalidation_reason = 'wal_removed' FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';} ), "t", 'synchronized slot has been invalidated'); @@ -274,7 +274,7 @@ $standby1->wait_for_log(qr/dropped replication slot "lsub1_slot" of dbid [0-9]+/ # flagged as 'synced' is( $standby1->safe_psql( 'postgres', - q{SELECT conflict_reason IS NULL AND synced AND NOT temporary FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';} + q{SELECT invalidation_reason IS NULL AND synced AND NOT temporary FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';} ), "t", 'logical slot is re-synced'); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 0cd2c64fca..08b0a34d55 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1473,10 +1473,10 @@ pg_replication_slots| SELECT l.slot_name, l.wal_status, l.safe_wal_size, l.two_phase, - l.conflict_reason, + l.invalidation_reason, l.failover, l.synced - FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, conflict_reason, failover, synced) + FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, invalidation_reason, failover, synced) LEFT JOIN pg_database d ON ((l.datoid = d.oid))); pg_roles| SELECT pg_authid.rolname, pg_authid.rolsuper, -- 2.34.1 [application/octet-stream] v8-0002-Add-XID-age-based-replication-slot-invalidation.patch (12.8K, ../../CALj2ACWgm++=TkXWo9spm+7ibGxcYL3SsYtT4tbW3=HT1VYs0Q@mail.gmail.com/3-v8-0002-Add-XID-age-based-replication-slot-invalidation.patch) download | inline diff: From 4be9a47c3cce539b8b5879f8c0359f552d1d419a Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 6 Mar 2024 08:45:03 +0000 Subject: [PATCH v8 2/4] Add XID age based replication slot invalidation Currently postgres has the ability to invalidate inactive replication slots based on the amount of WAL (set via max_slot_wal_keep_size GUC) that will be needed for the slots in case they become active. However, choosing a default value for max_slot_wal_keep_size is tricky. Because the amount of WAL a customer generates, and their allocated storage will vary greatly in production, making it difficult to pin down a one-size-fits-all value. It is often easy for developers to set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5 billion, after which the slots get invalidated. To achieve the above, postgres uses replication slot xmin (the oldest transaction that this slot needs the database to retain) or catalog_xmin (the oldest transaction affecting the system catalogs that this slot needs the database to retain), and a new GUC max_slot_xid_age. The checkpointer then looks at all replication slots invalidating the slots based on the age set. --- doc/src/sgml/config.sgml | 21 ++++ src/backend/access/transam/xlog.c | 10 ++ src/backend/replication/slot.c | 44 ++++++- src/backend/utils/misc/guc_tables.c | 10 ++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/replication/slot.h | 3 + src/test/recovery/meson.build | 1 + src/test/recovery/t/050_invalidate_slots.pl | 108 ++++++++++++++++++ 8 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 src/test/recovery/t/050_invalidate_slots.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index b38cbd714a..7a8360cd32 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4544,6 +4544,27 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows </listitem> </varlistentry> + <varlistentry id="guc-max-slot-xid-age" xreflabel="max_slot_xid_age"> + <term><varname>max_slot_xid_age</varname> (<type>integer</type>) + <indexterm> + <primary><varname>max_slot_xid_age</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Invalidate replication slots whose <literal>xmin</literal> (the oldest + transaction that this slot needs the database to retain) or + <literal>catalog_xmin</literal> (the oldest transaction affecting the + system catalogs that this slot needs the database to retain) has reached + the age specified by this setting. A value of zero (which is default) + disables this feature. Users can set this value anywhere from zero to + two billion. This parameter can only be set in the + <filename>postgresql.conf</filename> file or on the server command + line. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp"> <term><varname>track_commit_timestamp</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 20a5f86209..36ae2ac6a4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7147,6 +7147,11 @@ CreateCheckPoint(int flags) if (PriorRedoPtr != InvalidXLogRecPtr) UpdateCheckPointDistanceEstimate(RedoRecPtr - PriorRedoPtr); + /* Invalidate replication slots based on xmin or catalog_xmin age */ + if (max_slot_xid_age > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, + InvalidOid, InvalidTransactionId); + /* * Delete old log files, those no longer needed for last checkpoint to * prevent the disk holding the xlog from growing full. @@ -7597,6 +7602,11 @@ CreateRestartPoint(int flags) */ XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size); + /* Invalidate replication slots based on xmin or catalog_xmin age */ + if (max_slot_xid_age > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, + InvalidOid, InvalidTransactionId); + /* * Retreat _logSegNo using the current end of xlog replayed or received, * whichever is later. diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index b0f48229cb..f05990aeb8 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -86,10 +86,11 @@ const char *const SlotInvalidationCauses[] = { [RS_INVAL_WAL_REMOVED] = "wal_removed", [RS_INVAL_HORIZON] = "rows_removed", [RS_INVAL_WAL_LEVEL] = "wal_level_insufficient", + [RS_INVAL_XID_AGE] = "xid_aged", }; /* Maximum number of invalidation causes */ -#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL +#define RS_INVAL_MAX_CAUSES RS_INVAL_XID_AGE StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), "array length mismatch"); @@ -119,6 +120,7 @@ ReplicationSlot *MyReplicationSlot = NULL; /* GUC variable */ int max_replication_slots = 10; /* the maximum number of replication * slots */ +int max_slot_xid_age = 0; static void ReplicationSlotShmemExit(int code, Datum arg); static void ReplicationSlotDropPtr(ReplicationSlot *slot); @@ -1447,6 +1449,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause, case RS_INVAL_WAL_LEVEL: appendStringInfoString(&err_detail, _("Logical decoding on standby requires wal_level >= logical on the primary server.")); break; + case RS_INVAL_XID_AGE: + appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age.")); + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1563,6 +1568,42 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, if (SlotIsLogical(s)) conflict = cause; break; + case RS_INVAL_XID_AGE: + { + TransactionId xid_cur = ReadNextTransactionId(); + TransactionId xid_limit; + TransactionId xid_slot; + + if (TransactionIdIsNormal(s->data.xmin)) + { + xid_slot = s->data.xmin; + + xid_limit = xid_slot + max_slot_xid_age; + if (xid_limit < FirstNormalTransactionId) + xid_limit += FirstNormalTransactionId; + + if (TransactionIdFollowsOrEquals(xid_cur, xid_limit)) + { + conflict = cause; + break; + } + } + if (TransactionIdIsNormal(s->data.catalog_xmin)) + { + xid_slot = s->data.catalog_xmin; + + xid_limit = xid_slot + max_slot_xid_age; + if (xid_limit < FirstNormalTransactionId) + xid_limit += FirstNormalTransactionId; + + if (TransactionIdFollowsOrEquals(xid_cur, xid_limit)) + { + conflict = cause; + break; + } + } + } + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1725,6 +1766,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, * - RS_INVAL_HORIZON: requires a snapshot <= the given horizon in the given * db; dboid may be InvalidOid for shared relations * - RS_INVAL_WAL_LEVEL: is logical + * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age * * NB - this runs as part of checkpoint, so avoid raising errors if possible. */ diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 45013582a7..3ed642dcaf 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2954,6 +2954,16 @@ struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"max_slot_xid_age", PGC_SIGHUP, REPLICATION_SENDING, + gettext_noop("Age of the transaction ID at which a replication slot gets invalidated."), + gettext_noop("The transaction is the oldest transaction (including the one affecting the system catalogs) that a replication slot needs the database to retain.") + }, + &max_slot_xid_age, + 0, 0, 2000000000, + NULL, NULL, NULL + }, + { {"commit_delay", PGC_SUSET, WAL_SETTINGS, gettext_noop("Sets the delay in microseconds between transaction commit and " diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index edcc0282b2..50019d7c25 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -334,6 +334,7 @@ #wal_sender_timeout = 60s # in milliseconds; 0 disables #track_commit_timestamp = off # collect timestamp of transaction commit # (change requires restart) +#max_slot_xid_age = 0 # - Primary Server - diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 02a96b0e19..4b7ae36f11 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -53,6 +53,8 @@ typedef enum ReplicationSlotInvalidationCause RS_INVAL_HORIZON, /* wal_level insufficient for slot */ RS_INVAL_WAL_LEVEL, + /* slot's xmin or catalog_xmin has reached the age */ + RS_INVAL_XID_AGE, } ReplicationSlotInvalidationCause; extern PGDLLIMPORT const char *const SlotInvalidationCauses[]; @@ -226,6 +228,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot; /* GUCs */ extern PGDLLIMPORT int max_replication_slots; +extern PGDLLIMPORT int max_slot_xid_age; /* shmem initialization functions */ extern Size ReplicationSlotsShmemSize(void); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index c67249500e..d698c3ec73 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -50,6 +50,7 @@ tests += { 't/039_end_of_wal.pl', 't/040_standby_failover_slots_sync.pl', 't/041_checkpoint_at_promote.pl', + 't/050_invalidate_slots.pl', ], }, } diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl new file mode 100644 index 0000000000..2f482b56e8 --- /dev/null +++ b/src/test/recovery/t/050_invalidate_slots.pl @@ -0,0 +1,108 @@ +# Copyright (c) 2024, PostgreSQL Global Development Group + +# Test for replication slots invalidation +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; +use Time::HiRes qw(usleep); + +# Initialize primary node, setting wal-segsize to 1MB +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']); +$primary->append_conf( + 'postgresql.conf', q{ +checkpoint_timeout = 1h +}); +$primary->start; +$primary->safe_psql( + 'postgres', qq[ + SELECT pg_create_physical_replication_slot('sb1_slot'); +]); + +# Take backup +my $backup_name = 'my_backup'; +$primary->backup($backup_name); + +# Create a standby linking to the primary using the replication slot +my $standby1 = PostgreSQL::Test::Cluster->new('standby1'); +$standby1->init_from_backup($primary, $backup_name, has_streaming => 1); + +# Enable hs_feedback. The slot should gain an xmin. We set the status interval +# so we'll see the results promptly. +$standby1->append_conf( + 'postgresql.conf', q{ +primary_slot_name = 'sb1_slot' +hot_standby_feedback = on +wal_receiver_status_interval = 1 +}); +$standby1->start; + +# Create some content on primary to move xmin +$primary->safe_psql('postgres', + "CREATE TABLE tab_int AS SELECT generate_series(1,10) AS a"); + +# Wait until standby has replayed enough data +$primary->wait_for_catchup($standby1); + +$primary->poll_query_until( + 'postgres', qq[ + SELECT xmin IS NOT NULL + FROM pg_catalog.pg_replication_slots + WHERE slot_name = 'sb1_slot'; +]) or die "Timed out waiting for slot xmin to advance"; + +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET max_slot_xid_age = 500; +]); +$primary->reload; + +# Stop standby to make the replication slot's xmin on primary to age +$standby1->stop; + +my $logstart = -s $primary->logfile; + +# Do some work to advance xmin +$primary->safe_psql( + 'postgres', q{ +do $$ +begin + for i in 10000..11000 loop + -- use an exception block so that each iteration eats an XID + begin + insert into tab_int values (i); + exception + when division_by_zero then null; + end; + end loop; +end$$; +}); + +my $invalidated = 0; +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) +{ + $primary->safe_psql('postgres', "CHECKPOINT"); + if ($primary->log_contains( + 'invalidating obsolete replication slot "sb1_slot"', $logstart)) + { + $invalidated = 1; + last; + } + usleep(100_000); +} +ok($invalidated, 'check that slot sb1_slot invalidation has been logged'); + +# Wait for the inactive replication slots to be invalidated. +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE slot_name = 'sb1_slot' AND + invalidation_reason = 'xid_aged'; +]) + or die + "Timed out while waiting for replication slot sb1_slot to be invalidated"; + +done_testing(); -- 2.34.1 [application/octet-stream] v8-0003-Track-inactive-replication-slot-information.patch (9.8K, ../../CALj2ACWgm++=TkXWo9spm+7ibGxcYL3SsYtT4tbW3=HT1VYs0Q@mail.gmail.com/4-v8-0003-Track-inactive-replication-slot-information.patch) download | inline diff: From 543713209087881e82c3a63731e149e92499260c Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 6 Mar 2024 08:45:54 +0000 Subject: [PATCH v8 3/4] Track inactive replication slot information Currently postgres doesn't track metrics like the time at which the slot became inactive, and the total number of times the slot became inactive in its lifetime. This commit adds two new metrics last_inactive_at of type timestamptz and inactive_count of type numeric to ReplicationSlotPersistentData. Whenever a slot becomes inactive, the current timestamp and inactive count are persisted to disk. These metrics are useful in the following ways: - To improve replication slot monitoring tools. For instance, one can build a monitoring tool that signals a) when replication slots is lying inactive for a day or so using last_inactive_at metric, b) when a replication slot is becoming inactive too frequently using last_inactive_at metric. - To implement timeout-based inactive replication slot management capability in postgres. Increases SLOT_VERSION due to the added two new metrics. --- doc/src/sgml/system-views.sgml | 20 +++++++++++++ src/backend/catalog/system_views.sql | 4 ++- src/backend/replication/slot.c | 43 ++++++++++++++++++++++------ src/backend/replication/slotfuncs.c | 15 +++++++++- src/include/catalog/pg_proc.dat | 6 ++-- src/include/replication/slot.h | 6 ++++ src/test/regress/expected/rules.out | 6 ++-- 7 files changed, 84 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index c519b4a7f8..7909623453 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -2740,6 +2740,26 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx ID of role </para></entry> </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_inactive_at</structfield> <type>timestamptz</type> + </para> + <para> + The time at which the slot became inactive. + <literal>NULL</literal> if the slot is currently actively being + used. + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>inactive_count</structfield> <type>numeric</type> + </para> + <para> + The total number of times the slot became inactive in its lifetime. + </para></entry> + </row> </tbody> </tgroup> </table> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 1dbfcef9f1..763a4e668b 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1025,7 +1025,9 @@ CREATE VIEW pg_replication_slots AS L.two_phase, L.invalidation_reason, L.failover, - L.synced + L.synced, + L.last_inactive_at, + L.inactive_count FROM pg_get_replication_slots() AS L LEFT JOIN pg_database D ON (L.datoid = D.oid); diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index f05990aeb8..9e323b58b3 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -109,7 +109,7 @@ StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), sizeof(ReplicationSlotOnDisk) - ReplicationSlotOnDiskConstantSize #define SLOT_MAGIC 0x1051CA1 /* format identifier */ -#define SLOT_VERSION 5 /* version for new files */ +#define SLOT_VERSION 6 /* version for new files */ /* Control array for replication slot management */ ReplicationSlotCtlData *ReplicationSlotCtl = NULL; @@ -364,6 +364,8 @@ ReplicationSlotCreate(const char *name, bool db_specific, slot->data.two_phase_at = InvalidXLogRecPtr; slot->data.failover = failover; slot->data.synced = synced; + slot->data.last_inactive_at = 0; + slot->data.inactive_count = 0; /* and then data only present in shared memory */ slot->just_dirtied = false; @@ -590,6 +592,17 @@ retry: if (am_walsender) { + if (s->data.persistency == RS_PERSISTENT) + { + SpinLockAcquire(&s->mutex); + s->data.last_inactive_at = 0; + SpinLockRelease(&s->mutex); + + /* Write this slot to disk */ + ReplicationSlotMarkDirty(); + ReplicationSlotSave(); + } + ereport(log_replication_commands ? LOG : DEBUG1, SlotIsLogical(s) ? errmsg("acquired logical replication slot \"%s\"", @@ -657,16 +670,20 @@ ReplicationSlotRelease(void) ConditionVariableBroadcast(&slot->active_cv); } - MyReplicationSlot = NULL; - - /* might not have been set when we've been a plain slot */ - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); - MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; - ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; - LWLockRelease(ProcArrayLock); - if (am_walsender) { + if (slot->data.persistency == RS_PERSISTENT) + { + SpinLockAcquire(&slot->mutex); + slot->data.last_inactive_at = GetCurrentTimestamp(); + slot->data.inactive_count++; + SpinLockRelease(&slot->mutex); + + /* Write this slot to disk */ + ReplicationSlotMarkDirty(); + ReplicationSlotSave(); + } + ereport(log_replication_commands ? LOG : DEBUG1, is_logical ? errmsg("released logical replication slot \"%s\"", @@ -676,6 +693,14 @@ ReplicationSlotRelease(void) pfree(slotname); } + + MyReplicationSlot = NULL; + + /* might not have been set when we've been a plain slot */ + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; + ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; + LWLockRelease(ProcArrayLock); } /* diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 758498d29d..3e287cba66 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -239,10 +239,11 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) Datum pg_get_replication_slots(PG_FUNCTION_ARGS) { -#define PG_GET_REPLICATION_SLOTS_COLS 17 +#define PG_GET_REPLICATION_SLOTS_COLS 19 ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; XLogRecPtr currlsn; int slotno; + char buf[256]; /* * We don't require any special permission to see this function's data @@ -418,6 +419,18 @@ pg_get_replication_slots(PG_FUNCTION_ARGS) values[i++] = BoolGetDatum(slot_contents.data.synced); + if (slot_contents.data.last_inactive_at > 0) + values[i++] = TimestampTzGetDatum(slot_contents.data.last_inactive_at); + else + nulls[i++] = true; + + /* Convert to numeric. */ + snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count); + values[i++] = DirectFunctionCall3(numeric_in, + CStringGetDatum(buf), + ObjectIdGetDatum(0), + Int32GetDatum(-1)); + Assert(i == PG_GET_REPLICATION_SLOTS_COLS); tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 69140a0bf0..0071ce4cf8 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11120,9 +11120,9 @@ proname => 'pg_get_replication_slots', prorows => '10', proisstrict => 'f', proretset => 't', provolatile => 's', prorettype => 'record', proargtypes => '', - proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool}', - proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', - proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,invalidation_reason,failover,synced}', + proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,text,bool,bool,timestamptz,numeric}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,invalidation_reason,failover,synced,last_inactive_at,inactive_count}', prosrc => 'pg_get_replication_slots' }, { oid => '3786', descr => 'set up a logical replication slot', proname => 'pg_create_logical_replication_slot', provolatile => 'v', diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 4b7ae36f11..7d668918b0 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -129,6 +129,12 @@ typedef struct ReplicationSlotPersistentData * for logical slots on the primary server. */ bool failover; + + /* When did this slot become inactive last time? */ + TimestampTz last_inactive_at; + + /* How many times the slot has been inactive? */ + uint64 inactive_count; } ReplicationSlotPersistentData; /* diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 08b0a34d55..b63f5ea5da 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1475,8 +1475,10 @@ pg_replication_slots| SELECT l.slot_name, l.two_phase, l.invalidation_reason, l.failover, - l.synced - FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, invalidation_reason, failover, synced) + l.synced, + l.last_inactive_at, + l.inactive_count + FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, invalidation_reason, failover, synced, last_inactive_at, inactive_count) LEFT JOIN pg_database d ON ((l.datoid = d.oid))); pg_roles| SELECT pg_authid.rolname, pg_authid.rolsuper, -- 2.34.1 [application/octet-stream] v8-0004-Add-inactive_timeout-based-replication-slot-inval.patch (11.3K, ../../CALj2ACWgm++=TkXWo9spm+7ibGxcYL3SsYtT4tbW3=HT1VYs0Q@mail.gmail.com/5-v8-0004-Add-inactive_timeout-based-replication-slot-inval.patch) download | inline diff: From 1d3d286b97607bce67cdfbf7cab6f0a9b734a204 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 6 Mar 2024 08:46:47 +0000 Subject: [PATCH v8 4/4] Add inactive_timeout based replication slot invalidation Currently postgres has the ability to invalidate inactive replication slots based on the amount of WAL (set via max_slot_wal_keep_size GUC) that will be needed for the slots in case they become active. However, choosing a default value for max_slot_wal_keep_size is tricky. Because the amount of WAL a customer generates, and their allocated storage will vary greatly in production, making it difficult to pin down a one-size-fits-all value. It is often easy for developers to set a timeout of say 1 or 2 or 3 days, after which the inactive slots get dropped. To achieve the above, postgres uses replication slot metric inactive_at (the time at which the slot became inactive), and a new GUC inactive_replication_slot_timeout. The checkpointer then looks at all replication slots invalidating the inactive slots based on the timeout set. --- doc/src/sgml/config.sgml | 18 +++++ src/backend/access/transam/xlog.c | 10 +++ src/backend/replication/slot.c | 22 +++++- src/backend/utils/misc/guc_tables.c | 12 +++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/replication/slot.h | 3 + src/test/recovery/t/050_invalidate_slots.pl | 79 +++++++++++++++++++ 7 files changed, 144 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7a8360cd32..f5c299ef73 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4565,6 +4565,24 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows </listitem> </varlistentry> + <varlistentry id="guc-inactive-replication-slot-timeout" xreflabel="inactive_replication_slot_timeout"> + <term><varname>inactive_replication_slot_timeout</varname> (<type>integer</type>) + <indexterm> + <primary><varname>inactive_replication_slot_timeout</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Invalidate replication slots that are inactive for longer than this + amount of time at the next checkpoint. If this value is specified + without units, it is taken as seconds. A value of zero (which is + default) disables the timeout mechanism. This parameter can only be + set in the <filename>postgresql.conf</filename> file or on the server + command line. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp"> <term><varname>track_commit_timestamp</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 36ae2ac6a4..166c3ed794 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7152,6 +7152,11 @@ CreateCheckPoint(int flags) InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, InvalidOid, InvalidTransactionId); + /* Invalidate inactive replication slots based on timeout */ + if (inactive_replication_slot_timeout > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0, + InvalidOid, InvalidTransactionId); + /* * Delete old log files, those no longer needed for last checkpoint to * prevent the disk holding the xlog from growing full. @@ -7607,6 +7612,11 @@ CreateRestartPoint(int flags) InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, InvalidOid, InvalidTransactionId); + /* Invalidate inactive replication slots based on timeout */ + if (inactive_replication_slot_timeout > 0) + InvalidateObsoleteReplicationSlots(RS_INVAL_INACTIVE_TIMEOUT, 0, + InvalidOid, InvalidTransactionId); + /* * Retreat _logSegNo using the current end of xlog replayed or received, * whichever is later. diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 9e323b58b3..2360682e05 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -87,10 +87,11 @@ const char *const SlotInvalidationCauses[] = { [RS_INVAL_HORIZON] = "rows_removed", [RS_INVAL_WAL_LEVEL] = "wal_level_insufficient", [RS_INVAL_XID_AGE] = "xid_aged", + [RS_INVAL_INACTIVE_TIMEOUT] = "inactive_timeout", }; /* Maximum number of invalidation causes */ -#define RS_INVAL_MAX_CAUSES RS_INVAL_XID_AGE +#define RS_INVAL_MAX_CAUSES RS_INVAL_INACTIVE_TIMEOUT StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1), "array length mismatch"); @@ -121,6 +122,7 @@ ReplicationSlot *MyReplicationSlot = NULL; int max_replication_slots = 10; /* the maximum number of replication * slots */ int max_slot_xid_age = 0; +int inactive_replication_slot_timeout = 0; static void ReplicationSlotShmemExit(int code, Datum arg); static void ReplicationSlotDropPtr(ReplicationSlot *slot); @@ -1477,6 +1479,9 @@ ReportSlotInvalidation(ReplicationSlotInvalidationCause cause, case RS_INVAL_XID_AGE: appendStringInfoString(&err_detail, _("The replication slot's xmin or catalog_xmin reached the age specified by max_slot_xid_age.")); break; + case RS_INVAL_INACTIVE_TIMEOUT: + appendStringInfoString(&err_detail, _("The slot has been inactive for more than the time specified by inactive_replication_slot_timeout.")); + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1629,6 +1634,20 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, } } break; + case RS_INVAL_INACTIVE_TIMEOUT: + if (s->data.last_inactive_at > 0) + { + TimestampTz now; + + Assert(s->data.persistency == RS_PERSISTENT); + Assert(s->active_pid == 0); + + now = GetCurrentTimestamp(); + if (TimestampDifferenceExceeds(s->data.last_inactive_at, now, + inactive_replication_slot_timeout * 1000)) + conflict = cause; + } + break; case RS_INVAL_NONE: pg_unreachable(); } @@ -1792,6 +1811,7 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, * db; dboid may be InvalidOid for shared relations * - RS_INVAL_WAL_LEVEL: is logical * - RS_INVAL_XID_AGE: slot's xmin or catalog_xmin has reached the age + * - RS_INVAL_INACTIVE_TIMEOUT: inactive slot timeout occurs * * NB - this runs as part of checkpoint, so avoid raising errors if possible. */ diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 3ed642dcaf..06e3e87f4a 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2964,6 +2964,18 @@ struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"inactive_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SENDING, + gettext_noop("Sets the amount of time to wait before invalidating an " + "inactive replication slot."), + NULL, + GUC_UNIT_S + }, + &inactive_replication_slot_timeout, + 0, 0, INT_MAX, + NULL, NULL, NULL + }, + { {"commit_delay", PGC_SUSET, WAL_SETTINGS, gettext_noop("Sets the delay in microseconds between transaction commit and " diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 50019d7c25..092aaf1bec 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -261,6 +261,7 @@ #recovery_prefetch = try # prefetch pages referenced in the WAL? #wal_decode_buffer_size = 512kB # lookahead window used for prefetching # (change requires restart) +#inactive_replication_slot_timeout = 0 # in seconds; 0 disables # - Archiving - diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index 7d668918b0..7ae98046a4 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -55,6 +55,8 @@ typedef enum ReplicationSlotInvalidationCause RS_INVAL_WAL_LEVEL, /* slot's xmin or catalog_xmin has reached the age */ RS_INVAL_XID_AGE, + /* inactive slot timeout has occurred */ + RS_INVAL_INACTIVE_TIMEOUT, } ReplicationSlotInvalidationCause; extern PGDLLIMPORT const char *const SlotInvalidationCauses[]; @@ -235,6 +237,7 @@ extern PGDLLIMPORT ReplicationSlot *MyReplicationSlot; /* GUCs */ extern PGDLLIMPORT int max_replication_slots; extern PGDLLIMPORT int max_slot_xid_age; +extern PGDLLIMPORT int inactive_replication_slot_timeout; /* shmem initialization functions */ extern Size ReplicationSlotsShmemSize(void); diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl index 2f482b56e8..4c66dd4a4e 100644 --- a/src/test/recovery/t/050_invalidate_slots.pl +++ b/src/test/recovery/t/050_invalidate_slots.pl @@ -105,4 +105,83 @@ $primary->poll_query_until( or die "Timed out while waiting for replication slot sb1_slot to be invalidated"; +$primary->safe_psql( + 'postgres', qq[ + SELECT pg_create_physical_replication_slot('sb2_slot'); +]); + +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET max_slot_xid_age = 0; +]); +$primary->reload; + +# Create a standby linking to the primary using the replication slot +my $standby2 = PostgreSQL::Test::Cluster->new('standby2'); +$standby2->init_from_backup($primary, $backup_name, has_streaming => 1); +$standby2->append_conf( + 'postgresql.conf', q{ +primary_slot_name = 'sb2_slot' +}); +$standby2->start; + +# Wait until standby has replayed enough data +$primary->wait_for_catchup($standby2); + +# The inactive replication slot info should be null when the slot is active +my $result = $primary->safe_psql( + 'postgres', qq[ + SELECT last_inactive_at IS NULL, inactive_count = 0 AS OK + FROM pg_replication_slots WHERE slot_name = 'sb2_slot'; +]); +is($result, "t|t", + 'check the inactive replication slot info for an active slot'); + +# Set timeout so that the next checkpoint will invalidate the inactive +# replication slot. +$primary->safe_psql( + 'postgres', qq[ + ALTER SYSTEM SET inactive_replication_slot_timeout TO '1s'; +]); +$primary->reload; + +$logstart = -s $primary->logfile; + +# Stop standby to make the replication slot on primary inactive +$standby2->stop; + +# Wait for the inactive replication slot info to be updated +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE last_inactive_at IS NOT NULL AND + inactive_count = 1 AND slot_name = 'sb2_slot'; +]) + or die + "Timed out while waiting for inactive replication slot info to be updated"; + +$invalidated = 0; +for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++) +{ + $primary->safe_psql('postgres', "CHECKPOINT"); + if ($primary->log_contains( + 'invalidating obsolete replication slot "sb2_slot"', $logstart)) + { + $invalidated = 1; + last; + } + usleep(100_000); +} +ok($invalidated, 'check that slot sb2_slot invalidation has been logged'); + +# Wait for the inactive replication slots to be invalidated. +$primary->poll_query_until( + 'postgres', qq[ + SELECT COUNT(slot_name) = 1 FROM pg_replication_slots + WHERE slot_name = 'sb2_slot' AND + invalidation_reason = 'inactive_timeout'; +]) + or die + "Timed out while waiting for inactive replication slot sb2_slot to be invalidated"; + done_testing(); -- 2.34.1 ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-06 10:26 Bertrand Drouvot <[email protected]> parent: Bharath Rupireddy <[email protected]> 2 siblings, 0 replies; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-06 10:26 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Wed, Mar 06, 2024 at 02:46:57PM +0530, Bharath Rupireddy wrote: > On Wed, Mar 6, 2024 at 2:42 PM Bertrand Drouvot > <[email protected]> wrote: > > Yeah, I'm okay with one column. > > Thanks. v8-0001 is how it looks. Please see the v8 patch set with this change. Thanks! A few comments: 1 === + The reason for the slot's invalidation. <literal>NULL</literal> if the + slot is currently actively being used. s/currently actively being used/not invalidated/ ? (I mean it could be valid and not being used). 2 === + the slot is marked as invalidated. In case of logical slots, it + represents the reason for the logical slot's conflict with recovery. s/the reason for the logical slot's conflict with recovery./the recovery conflict reason./ ? 3 === @@ -667,13 +667,13 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo, bool live_check) * removed. */ res = executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase, failover, " - "%s as caught_up, conflict_reason IS NOT NULL as invalid " + "%s as caught_up, invalidation_reason IS NOT NULL as invalid " "FROM pg_catalog.pg_replication_slots " "WHERE slot_type = 'logical' AND " "database = current_database() AND " "temporary IS FALSE;", live_check ? "FALSE" : - "(CASE WHEN conflict_reason IS NOT NULL THEN FALSE " + "(CASE WHEN invalidation_reason IS NOT NULL THEN FALSE " Yeah that's fine because there is logical slot filtering here. 4 === -GetSlotInvalidationCause(const char *conflict_reason) +GetSlotInvalidationCause(const char *invalidation_reason) Should we change the comment "Maps a conflict reason" above this function? 5 === -# Check conflict_reason is NULL for physical slot +# Check invalidation_reason is NULL for physical slot $res = $node_primary->safe_psql( 'postgres', qq[ - SELECT conflict_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';] + SELECT invalidation_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';] ); I don't think this test is needed anymore: it does not make that much sense since it's done after the primary database initialization and startup. 6 === @@ -680,7 +680,7 @@ ok( $node_standby->poll_query_until( is( $node_standby->safe_psql( 'postgres', q[select bool_or(conflicting) from - (select conflict_reason is not NULL as conflicting + (select invalidation_reason is not NULL as conflicting from pg_replication_slots WHERE slot_type = 'logical')]), 'f', 'Logical slots are reported as non conflicting'); What about? " # Verify slots are reported as valid in pg_replication_slots is( $node_standby->safe_psql( 'postgres', q[select bool_or(invalidated) from (select invalidation_reason is not NULL as invalidated from pg_replication_slots WHERE slot_type = 'logical')]), 'f', 'Logical slots are reported as valid'); " Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-06 10:58 Amit Kapila <[email protected]> parent: Nathan Bossart <[email protected]> 2 siblings, 0 replies; 32+ messages in thread From: Amit Kapila @ 2024-03-06 10:58 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Bertrand Drouvot <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 4, 2024 at 3:14 AM Nathan Bossart <[email protected]> wrote: > > On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote: > > On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <[email protected]> wrote: > >> Would you ever see "conflict" as false and "invalidation_reason" as > >> non-null for a logical slot? > > > > No. Because both conflict and invalidation_reason are decided based on > > the invalidation reason i.e. value of slot_contents.data.invalidated. > > IOW, a logical slot that reports conflict as true must have been > > invalidated. > > > > Do you have any thoughts on reverting 007693f and introducing > > invalidation_reason? > > Unless I am misinterpreting some details, ISTM we could rename this column > to invalidation_reason and use it for both logical and physical slots. I'm > not seeing a strong need for another column. Perhaps I am missing > something... > IIUC, the current conflict_reason is primarily used to determine logical slots on standby that got invalidated due to recovery time conflict. On the primary, it will also show logical slots that got invalidated due to the corresponding WAL got removed. Is that understanding correct? If so, we are already sort of overloading this column. However, now adding more invalidation reasons that won't happen during recovery conflict handling will change entirely the purpose (as per the name we use) of this variable. I think invalidation_reason could depict this column correctly but OTOH I guess it would lose its original meaning/purpose. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-06 11:19 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 2 siblings, 1 reply; 32+ messages in thread From: Amit Kapila @ 2024-03-06 11:19 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 6, 2024 at 2:47 PM Bharath Rupireddy <[email protected]> wrote: > > > Thanks. v8-0001 is how it looks. Please see the v8 patch set with this change. > @@ -1629,6 +1634,20 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause, } } break; + case RS_INVAL_INACTIVE_TIMEOUT: + if (s->data.last_inactive_at > 0) + { + TimestampTz now; + + Assert(s->data.persistency == RS_PERSISTENT); + Assert(s->active_pid == 0); + + now = GetCurrentTimestamp(); + if (TimestampDifferenceExceeds(s->data.last_inactive_at, now, + inactive_replication_slot_timeout * 1000)) You might want to consider its interaction with sync slots on standby. Say, there is no activity on slots in terms of processing the changes for slots. Now, we won't perform sync of such slots on standby showing them inactive as per your new criteria where as same slots could still be valid on primary as the walsender is still active. This may be more of a theoretical point as in running system there will probably be some activity but I think this needs some thougths. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-08 17:12 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-08 17:12 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 6, 2024 at 4:49 PM Amit Kapila <[email protected]> wrote: > > You might want to consider its interaction with sync slots on standby. > Say, there is no activity on slots in terms of processing the changes > for slots. Now, we won't perform sync of such slots on standby showing > them inactive as per your new criteria where as same slots could still > be valid on primary as the walsender is still active. This may be more > of a theoretical point as in running system there will probably be > some activity but I think this needs some thougths. I believe the xmin and catalog_xmin of the sync slots on the standby keep advancing depending on the slots on the primary, no? If yes, the XID age based invalidation shouldn't be a problem. I believe there are no walsenders started for the sync slots on the standbys, right? If yes, the inactive timeout based invalidation also shouldn't be a problem. Because, the inactive timeouts for a slot are tracked only for walsenders because they are the ones that typically hold replication slots for longer durations and for real replication use. We did a similar thing in a recent commit [1]. Is my understanding right? Do you still see any problems with it? [1] commit 7c3fb505b14e86581b6a052075a294c78c91b123 Author: Amit Kapila <[email protected]> Date: Tue Nov 21 07:59:53 2023 +0530 Log messages for replication slot acquisition and release. ......... Note that these messages are emitted only for walsenders but not for backends. This is because walsenders are the ones that typically hold replication slots for longer durations, unlike backends which hold them for executing replication related functions. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-11 10:14 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Amit Kapila @ 2024-03-11 10:14 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Mar 8, 2024 at 10:42 PM Bharath Rupireddy <[email protected]> wrote: > > On Wed, Mar 6, 2024 at 4:49 PM Amit Kapila <[email protected]> wrote: > > > > You might want to consider its interaction with sync slots on standby. > > Say, there is no activity on slots in terms of processing the changes > > for slots. Now, we won't perform sync of such slots on standby showing > > them inactive as per your new criteria where as same slots could still > > be valid on primary as the walsender is still active. This may be more > > of a theoretical point as in running system there will probably be > > some activity but I think this needs some thougths. > > I believe the xmin and catalog_xmin of the sync slots on the standby > keep advancing depending on the slots on the primary, no? If yes, the > XID age based invalidation shouldn't be a problem. > > I believe there are no walsenders started for the sync slots on the > standbys, right? If yes, the inactive timeout based invalidation also > shouldn't be a problem. Because, the inactive timeouts for a slot are > tracked only for walsenders because they are the ones that typically > hold replication slots for longer durations and for real replication > use. We did a similar thing in a recent commit [1]. > > Is my understanding right? > Yes, your understanding is correct. I wanted us to consider having new parameters like 'inactive_replication_slot_timeout' to be at slot-level instead of GUC. I think this new parameter doesn't seem to be the similar as 'max_slot_wal_keep_size' which leads to truncation of WAL at global and then invalidates the appropriate slots. OTOH, the 'inactive_replication_slot_timeout' doesn't appear to have a similar global effect. The other thing we should consider is what if the checkpoint happens at a timeout greater than 'inactive_replication_slot_timeout'? Shall, we consider doing it via some other background process or do we think checkpointer is the best we can have? > Do you still see any problems with it? > Sorry, I haven't done any detailed review yet so can't say with confidence whether there is any problem or not w.r.t sync slots. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 07:54 Bertrand Drouvot <[email protected]> parent: Bharath Rupireddy <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-12 07:54 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Fri, Mar 08, 2024 at 10:42:20PM +0530, Bharath Rupireddy wrote: > On Wed, Mar 6, 2024 at 4:49 PM Amit Kapila <[email protected]> wrote: > > > > You might want to consider its interaction with sync slots on standby. > > Say, there is no activity on slots in terms of processing the changes > > for slots. Now, we won't perform sync of such slots on standby showing > > them inactive as per your new criteria where as same slots could still > > be valid on primary as the walsender is still active. This may be more > > of a theoretical point as in running system there will probably be > > some activity but I think this needs some thougths. > > I believe the xmin and catalog_xmin of the sync slots on the standby > keep advancing depending on the slots on the primary, no? If yes, the > XID age based invalidation shouldn't be a problem. > > I believe there are no walsenders started for the sync slots on the > standbys, right? If yes, the inactive timeout based invalidation also > shouldn't be a problem. Because, the inactive timeouts for a slot are > tracked only for walsenders because they are the ones that typically > hold replication slots for longer durations and for real replication > use. We did a similar thing in a recent commit [1]. > > Is my understanding right? Do you still see any problems with it? Would that make sense to "simply" discard/prevent those kind of invalidations for "synced" slot on standby? I mean, do they make sense given the fact that those slots are not usable until the standby is promoted? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 12:21 Amit Kapila <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: Amit Kapila @ 2024-03-12 12:21 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 12, 2024 at 1:24 PM Bertrand Drouvot <[email protected]> wrote: > > On Fri, Mar 08, 2024 at 10:42:20PM +0530, Bharath Rupireddy wrote: > > On Wed, Mar 6, 2024 at 4:49 PM Amit Kapila <[email protected]> wrote: > > > > > > You might want to consider its interaction with sync slots on standby. > > > Say, there is no activity on slots in terms of processing the changes > > > for slots. Now, we won't perform sync of such slots on standby showing > > > them inactive as per your new criteria where as same slots could still > > > be valid on primary as the walsender is still active. This may be more > > > of a theoretical point as in running system there will probably be > > > some activity but I think this needs some thougths. > > > > I believe the xmin and catalog_xmin of the sync slots on the standby > > keep advancing depending on the slots on the primary, no? If yes, the > > XID age based invalidation shouldn't be a problem. > > > > I believe there are no walsenders started for the sync slots on the > > standbys, right? If yes, the inactive timeout based invalidation also > > shouldn't be a problem. Because, the inactive timeouts for a slot are > > tracked only for walsenders because they are the ones that typically > > hold replication slots for longer durations and for real replication > > use. We did a similar thing in a recent commit [1]. > > > > Is my understanding right? Do you still see any problems with it? > > Would that make sense to "simply" discard/prevent those kind of invalidations > for "synced" slot on standby? I mean, do they make sense given the fact that > those slots are not usable until the standby is promoted? > AFAIR, we don't prevent similar invalidations due to 'max_slot_wal_keep_size' for sync slots, so why to prevent it for these new parameters? This will unnecessarily create inconsistency in the invalidation behavior. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 15:40 Bertrand Drouvot <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 2 replies; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-12 15:40 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Tue, Mar 12, 2024 at 05:51:43PM +0530, Amit Kapila wrote: > On Tue, Mar 12, 2024 at 1:24 PM Bertrand Drouvot > <[email protected]> wrote: > > > > On Fri, Mar 08, 2024 at 10:42:20PM +0530, Bharath Rupireddy wrote: > > > On Wed, Mar 6, 2024 at 4:49 PM Amit Kapila <[email protected]> wrote: > > > > > > > > You might want to consider its interaction with sync slots on standby. > > > > Say, there is no activity on slots in terms of processing the changes > > > > for slots. Now, we won't perform sync of such slots on standby showing > > > > them inactive as per your new criteria where as same slots could still > > > > be valid on primary as the walsender is still active. This may be more > > > > of a theoretical point as in running system there will probably be > > > > some activity but I think this needs some thougths. > > > > > > I believe the xmin and catalog_xmin of the sync slots on the standby > > > keep advancing depending on the slots on the primary, no? If yes, the > > > XID age based invalidation shouldn't be a problem. > > > > > > I believe there are no walsenders started for the sync slots on the > > > standbys, right? If yes, the inactive timeout based invalidation also > > > shouldn't be a problem. Because, the inactive timeouts for a slot are > > > tracked only for walsenders because they are the ones that typically > > > hold replication slots for longer durations and for real replication > > > use. We did a similar thing in a recent commit [1]. > > > > > > Is my understanding right? Do you still see any problems with it? > > > > Would that make sense to "simply" discard/prevent those kind of invalidations > > for "synced" slot on standby? I mean, do they make sense given the fact that > > those slots are not usable until the standby is promoted? > > > > AFAIR, we don't prevent similar invalidations due to > 'max_slot_wal_keep_size' for sync slots, Right, we'd invalidate them on the standby should the standby sync slot restart_lsn exceeds the limit. > so why to prevent it for > these new parameters? This will unnecessarily create inconsistency in > the invalidation behavior. Yeah, but I think wal removal has a direct impact on the slot usuability which is probably not the case with the new XID and Timeout ones. That's why I thought about handling them differently (but I'm also fine if that's not the case). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 15:44 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 0 replies; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-12 15:44 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 12, 2024 at 5:51 PM Amit Kapila <[email protected]> wrote: > > > Would that make sense to "simply" discard/prevent those kind of invalidations > > for "synced" slot on standby? I mean, do they make sense given the fact that > > those slots are not usable until the standby is promoted? > > AFAIR, we don't prevent similar invalidations due to > 'max_slot_wal_keep_size' for sync slots, so why to prevent it for > these new parameters? This will unnecessarily create inconsistency in > the invalidation behavior. Right. +1 to keep the behaviour consistent for all invalidations. However, an assertion that inactive_timeout isn't set for synced slots on the standby isn't a bad idea because we rely on the fact that walsenders aren't started for synced slots. Again, I think it misses the consistency in the invalidation behaviour. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 15:49 Bharath Rupireddy <[email protected]> parent: Bertrand Drouvot <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-12 15:49 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 12, 2024 at 9:11 PM Bertrand Drouvot <[email protected]> wrote: > > > AFAIR, we don't prevent similar invalidations due to > > 'max_slot_wal_keep_size' for sync slots, > > Right, we'd invalidate them on the standby should the standby sync slot restart_lsn > exceeds the limit. Right. Help me understand this a bit - is the wal_removed invalidation going to conflict with recovery on the standby? Per the discussion upthread, I'm trying to understand what invalidation reasons will exactly cause conflict with recovery? Is it just rows_removed and wal_level_insufficient invalidations? My understanding on the conflict with recovery and invalidation reason has been a bit off track. Perhaps, we need to clarify these two things in the docs for the end users as well? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-12 16:39 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-12 16:39 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 11, 2024 at 3:44 PM Amit Kapila <[email protected]> wrote: > > Yes, your understanding is correct. I wanted us to consider having new > parameters like 'inactive_replication_slot_timeout' to be at > slot-level instead of GUC. I think this new parameter doesn't seem to > be the similar as 'max_slot_wal_keep_size' which leads to truncation > of WAL at global and then invalidates the appropriate slots. OTOH, the > 'inactive_replication_slot_timeout' doesn't appear to have a similar > global effect. last_inactive_at is tracked for each slot using which slots get invalidated based on inactive_replication_slot_timeout. It's like max_slot_wal_keep_size invalidating slots based on restart_lsn. In a way, both are similar, right? > The other thing we should consider is what if the > checkpoint happens at a timeout greater than > 'inactive_replication_slot_timeout'? In such a case, the slots get invalidated upon the next checkpoint as the (current_checkpointer_timeout - last_inactive_at) will then be greater than inactive_replication_slot_timeout. > Shall, we consider doing it via > some other background process or do we think checkpointer is the best > we can have? The same problem exists if we do it with some other background process. I think the checkpointer is best because it already invalidates slots for wal_removed cause, and flushes all replication slots to disk. Moving this new invalidation functionality into some other background process such as autovacuum will not only burden that process' work but also mix up the unique functionality of that background process. Having said above, I'm open to ideas from others as I'm not so sure if there's any issue with checkpointer invalidating the slots for new reasons. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-13 04:08 Amit Kapila <[email protected]> parent: Bertrand Drouvot <[email protected]> 1 sibling, 0 replies; 32+ messages in thread From: Amit Kapila @ 2024-03-13 04:08 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 12, 2024 at 9:11 PM Bertrand Drouvot <[email protected]> wrote: > > On Tue, Mar 12, 2024 at 05:51:43PM +0530, Amit Kapila wrote: > > On Tue, Mar 12, 2024 at 1:24 PM Bertrand Drouvot > > <[email protected]> wrote: > > > so why to prevent it for > > these new parameters? This will unnecessarily create inconsistency in > > the invalidation behavior. > > Yeah, but I think wal removal has a direct impact on the slot usuability which > is probably not the case with the new XID and Timeout ones. > BTW, is XID the based parameter 'max_slot_xid_age' not have similarity with 'max_slot_wal_keep_size'? I think it will impact the rows we removed based on xid horizons. Don't we need to consider it while vacuum computing the xid horizons in ComputeXidHorizons() similar to what we do for WAL w.r.t 'max_slot_wal_keep_size'? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-13 04:24 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Amit Kapila @ 2024-03-13 04:24 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 12, 2024 at 10:10 PM Bharath Rupireddy <[email protected]> wrote: > > On Mon, Mar 11, 2024 at 3:44 PM Amit Kapila <[email protected]> wrote: > > > > Yes, your understanding is correct. I wanted us to consider having new > > parameters like 'inactive_replication_slot_timeout' to be at > > slot-level instead of GUC. I think this new parameter doesn't seem to > > be the similar as 'max_slot_wal_keep_size' which leads to truncation > > of WAL at global and then invalidates the appropriate slots. OTOH, the > > 'inactive_replication_slot_timeout' doesn't appear to have a similar > > global effect. > > last_inactive_at is tracked for each slot using which slots get > invalidated based on inactive_replication_slot_timeout. It's like > max_slot_wal_keep_size invalidating slots based on restart_lsn. In a > way, both are similar, right? > There is some similarity but 'max_slot_wal_keep_size' leads to truncation of WAL which in turn leads to invalidation of slots. Here, I am also trying to be cautious in adding a GUC unless it is required or having a slot-level parameter doesn't serve the need. Having said that, I see that there is an argument that we should follow the path of 'max_slot_wal_keep_size' GUC and there is some value to it but still I think avoiding a new GUC for inactivity in the slot would outweigh. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-13 05:43 shveta malik <[email protected]> parent: Bharath Rupireddy <[email protected]> 2 siblings, 1 reply; 32+ messages in thread From: shveta malik @ 2024-03-13 05:43 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]> On Wed, Mar 6, 2024 at 2:47 PM Bharath Rupireddy <[email protected]> wrote: > > On Wed, Mar 6, 2024 at 2:42 PM Bertrand Drouvot > <[email protected]> wrote: > > > > Hi, > > > > On Tue, Mar 05, 2024 at 01:44:43PM -0600, Nathan Bossart wrote: > > > On Wed, Mar 06, 2024 at 12:50:38AM +0530, Bharath Rupireddy wrote: > > > > On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot > > > > <[email protected]> wrote: > > > >> On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote: > > > >> > Unless I am misinterpreting some details, ISTM we could rename this column > > > >> > to invalidation_reason and use it for both logical and physical slots. I'm > > > >> > not seeing a strong need for another column. > > > >> > > > >> Yeah having two columns was more for convenience purpose. Without the "conflict" > > > >> one, a slot conflicting with recovery would be "a logical slot having a non NULL > > > >> invalidation_reason". > > > >> > > > >> I'm also fine with one column if most of you prefer that way. > > > > > > > > While we debate on the above, please find the attached v7 patch set > > > > after rebasing. > > > > > > It looks like Bertrand is okay with reusing the same column for both > > > logical and physical slots > > > > Yeah, I'm okay with one column. > > Thanks. v8-0001 is how it looks. Please see the v8 patch set with this change. JFYI, the patch does not apply to the head. There is a conflict in multiple files. thanks Shveta ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-13 07:15 shveta malik <[email protected]> parent: shveta malik <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: shveta malik @ 2024-03-13 07:15 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]> > JFYI, the patch does not apply to the head. There is a conflict in > multiple files. For review purposes, I applied v8 to the March 6 code-base. I have yet to review in detail, please find my initial thoughts: 1) I found that 'inactive_replication_slot_timeout' works only if there was any walsender ever started for that slot . The logic is under 'am_walsender' check. Is this intentional? If I create a slot and use only pg_logical_slot_get_changes or pg_replication_slot_advance on it, it never gets invalidated due to timeout. While, when I set 'max_slot_xid_age' or say 'max_slot_wal_keep_size' to a lower value, the said slot is invalidated correctly with 'xid_aged' and 'wal_removed' reasons respectively. Example: With inactive_replication_slot_timeout=1min, test1_3 is the slot for which there is no walsender and only advance and get_changes SQL functions were called; test1_4 is the one for which pg_recvlogical was run for a second. test1_3 | 785 | | reserved | | t | | test1_4 | 798 | | lost | inactive_timeout | t | 2024-03-13 11:52:41.58446+05:30 | And when inactive_replication_slot_timeout=0 and max_slot_xid_age=10 test1_3 | 785 | | lost | xid_aged | t | | test1_4 | 798 | | lost | inactive_timeout | t | 2024-03-13 11:52:41.58446+05:30 | 2) The msg for patch 3 says: -------------- a) when replication slots is lying inactive for a day or so using last_inactive_at metric, b) when a replication slot is becoming inactive too frequently using last_inactive_at metric. -------------- I think in b, you want to refer to inactive_count instead of last_inactive_at? 3) I do not see invalidation_reason updated for 2 new reasons in system-views.sgml thanks Shveta ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-13 07:21 Bertrand Drouvot <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-13 07:21 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Amit Kapila <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Tue, Mar 12, 2024 at 09:19:35PM +0530, Bharath Rupireddy wrote: > On Tue, Mar 12, 2024 at 9:11 PM Bertrand Drouvot > <[email protected]> wrote: > > > > > AFAIR, we don't prevent similar invalidations due to > > > 'max_slot_wal_keep_size' for sync slots, > > > > Right, we'd invalidate them on the standby should the standby sync slot restart_lsn > > exceeds the limit. > > Right. Help me understand this a bit - is the wal_removed invalidation > going to conflict with recovery on the standby? I don't think so, as it's not directly related to recovery. The slot will be invalided on the standby though. > Per the discussion upthread, I'm trying to understand what > invalidation reasons will exactly cause conflict with recovery? Is it > just rows_removed and wal_level_insufficient invalidations? Yes, that's the ones added in be87200efd. See the error messages on a standby: == wal removal postgres=# SELECT * FROM pg_logical_slot_get_changes('lsub4_slot', NULL, NULL, 'include-xids', '0'); ERROR: can no longer get changes from replication slot "lsub4_slot" DETAIL: This slot has been invalidated because it exceeded the maximum reserved size. == wal level postgres=# select conflict_reason from pg_replication_slots where slot_name = 'lsub5_slot';; conflict_reason ------------------------ wal_level_insufficient (1 row) postgres=# SELECT * FROM pg_logical_slot_get_changes('lsub5_slot', NULL, NULL, 'include-xids', '0'); ERROR: can no longer get changes from replication slot "lsub5_slot" DETAIL: This slot has been invalidated because it was conflicting with recovery. == rows removal postgres=# select conflict_reason from pg_replication_slots where slot_name = 'lsub6_slot';; conflict_reason ----------------- rows_removed (1 row) postgres=# SELECT * FROM pg_logical_slot_get_changes('lsub6_slot', NULL, NULL, 'include-xids', '0'); ERROR: can no longer get changes from replication slot "lsub6_slot" DETAIL: This slot has been invalidated because it was conflicting with recovery. As you can see, only wal level and rows removal are mentioning conflict with recovery. So, are we already "wrong" mentioning "wal_removed" in conflict_reason? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-15 05:14 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-15 05:14 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Mar 13, 2024 at 9:38 AM Amit Kapila <[email protected]> wrote: > > BTW, is XID the based parameter 'max_slot_xid_age' not have similarity > with 'max_slot_wal_keep_size'? I think it will impact the rows we > removed based on xid horizons. Don't we need to consider it while > vacuum computing the xid horizons in ComputeXidHorizons() similar to > what we do for WAL w.r.t 'max_slot_wal_keep_size'? I'm having a hard time understanding why we'd need something up there in ComputeXidHorizons(). Can you elaborate it a bit please? What's proposed with max_slot_xid_age is that during checkpoint we look at slot's xmin and catalog_xmin, and the current system txn id. Then, if the XID age of (xmin, catalog_xmin) and current_xid crosses max_slot_xid_age, we invalidate the slot. Let me illustrate how all this works: 1. Setup a primary and standby with hot_standby_feedback set to on on standby. For instance, check my scripts at [1]. 2. Stop the standby to make the slot inactive on the primary. Check the slot is holding xmin of 738. ./pg_ctl -D sbdata -l logfilesbdata stop postgres=# SELECT * FROM pg_replication_slots; -[ RECORD 1 ]-------+------------- slot_name | sb_repl_slot plugin | slot_type | physical datoid | database | temporary | f active | f active_pid | xmin | 738 catalog_xmin | restart_lsn | 0/3000000 confirmed_flush_lsn | wal_status | reserved safe_wal_size | two_phase | f conflict_reason | failover | f synced | f 3. Start consuming the XIDs on the primary with the following script for instance ./psql -d postgres -p 5432 DROP TABLE tab_int; CREATE TABLE tab_int (a int); do $$ begin for i in 1..268435 loop -- use an exception block so that each iteration eats an XID begin insert into tab_int values (i); exception when division_by_zero then null; end; end loop; end$$; 4. Make some dead rows in the table. update tab_int set a = a+1; delete from tab_int where a%4=0; postgres=# SELECT n_dead_tup, n_tup_ins, n_tup_upd, n_tup_del FROM pg_stat_user_tables WHERE relname = 'tab_int'; -[ RECORD 1 ]------ n_dead_tup | 335544 n_tup_ins | 268435 n_tup_upd | 268435 n_tup_del | 67109 5. Try vacuuming to delete the dead rows, observe 'tuples: 0 removed, 536870 remain, 335544 are dead but not yet removable'. The dead rows can't be removed because the inactive slot is holding an xmin, see 'removable cutoff: 738, which was 268441 XIDs old when operation ended'. postgres=# vacuum verbose tab_int; INFO: vacuuming "postgres.public.tab_int" INFO: finished vacuuming "postgres.public.tab_int": index scans: 0 pages: 0 removed, 2376 remain, 2376 scanned (100.00% of total) tuples: 0 removed, 536870 remain, 335544 are dead but not yet removable removable cutoff: 738, which was 268441 XIDs old when operation ended frozen: 0 pages from table (0.00% of total) had 0 tuples frozen index scan not needed: 0 pages from table (0.00% of total) had 0 dead item identifiers removed avg read rate: 0.000 MB/s, avg write rate: 0.000 MB/s buffer usage: 4759 hits, 0 misses, 0 dirtied WAL usage: 0 records, 0 full page images, 0 bytes system usage: CPU: user: 0.07 s, system: 0.00 s, elapsed: 0.07 s VACUUM 6. Now, repeat the above steps but with setting max_slot_xid_age = 200000 on the primary. 7. Do a checkpoint to invalidate the slot. postgres=# checkpoint; CHECKPOINT postgres=# SELECT * FROM pg_replication_slots; -[ RECORD 1 ]-------+------------- slot_name | sb_repl_slot plugin | slot_type | physical datoid | database | temporary | f active | f active_pid | xmin | 738 catalog_xmin | restart_lsn | 0/3000000 confirmed_flush_lsn | wal_status | lost safe_wal_size | two_phase | f conflicting | failover | f synced | f invalidation_reason | xid_aged 8. And, then vacuum the table, observe 'tuples: 335544 removed, 201326 remain, 0 are dead but not yet removable'. postgres=# vacuum verbose tab_int; INFO: vacuuming "postgres.public.tab_int" INFO: finished vacuuming "postgres.public.tab_int": index scans: 0 pages: 0 removed, 2376 remain, 2376 scanned (100.00% of total) tuples: 335544 removed, 201326 remain, 0 are dead but not yet removable removable cutoff: 269179, which was 0 XIDs old when operation ended new relfrozenxid: 269179, which is 268441 XIDs ahead of previous value frozen: 1189 pages from table (50.04% of total) had 201326 tuples frozen index scan not needed: 0 pages from table (0.00% of total) had 0 dead item identifiers removed avg read rate: 0.000 MB/s, avg write rate: 193.100 MB/s buffer usage: 4760 hits, 0 misses, 2381 dirtied WAL usage: 5942 records, 2378 full page images, 8343275 bytes system usage: CPU: user: 0.09 s, system: 0.00 s, elapsed: 0.09 s VACUUM [1] cd /home/ubuntu/postgres/pg17/bin ./pg_ctl -D db17 -l logfile17 stop rm -rf db17 logfile17 rm -rf /home/ubuntu/postgres/pg17/bin/archived_wal mkdir /home/ubuntu/postgres/pg17/bin/archived_wal ./initdb -D db17 echo "archive_mode = on archive_command='cp %p /home/ubuntu/postgres/pg17/bin/archived_wal/%f'" | tee -a db17/postgresql.conf ./pg_ctl -D db17 -l logfile17 start ./psql -d postgres -p 5432 -c "SELECT pg_create_physical_replication_slot('sb_repl_slot', true, false);" rm -rf sbdata logfilesbdata ./pg_basebackup -D sbdata echo "port=5433 primary_conninfo='host=localhost port=5432 dbname=postgres user=ubuntu' primary_slot_name='sb_repl_slot' restore_command='cp /home/ubuntu/postgres/pg17/bin/archived_wal/%f %p' hot_standby_feedback = on" | tee -a sbdata/postgresql.conf touch sbdata/standby.signal ./pg_ctl -D sbdata -l logfilesbdata start ./psql -d postgres -p 5433 -c "SELECT pg_is_in_recovery();" -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-16 10:24 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 2 replies; 32+ messages in thread From: Amit Kapila @ 2024-03-16 10:24 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Mar 15, 2024 at 10:45 AM Bharath Rupireddy <[email protected]> wrote: > > On Wed, Mar 13, 2024 at 9:38 AM Amit Kapila <[email protected]> wrote: > > > > BTW, is XID the based parameter 'max_slot_xid_age' not have similarity > > with 'max_slot_wal_keep_size'? I think it will impact the rows we > > removed based on xid horizons. Don't we need to consider it while > > vacuum computing the xid horizons in ComputeXidHorizons() similar to > > what we do for WAL w.r.t 'max_slot_wal_keep_size'? > > I'm having a hard time understanding why we'd need something up there > in ComputeXidHorizons(). Can you elaborate it a bit please? > > What's proposed with max_slot_xid_age is that during checkpoint we > look at slot's xmin and catalog_xmin, and the current system txn id. > Then, if the XID age of (xmin, catalog_xmin) and current_xid crosses > max_slot_xid_age, we invalidate the slot. > I can see that in your patch (in function InvalidatePossiblyObsoleteSlot()). As per my understanding, we need something similar for slot xids in ComputeXidHorizons() as we are doing WAL in KeepLogSeg(). In KeepLogSeg(), we compute the minimum LSN location required by slots and then adjust it for 'max_slot_wal_keep_size'. On similar lines, currently in ComputeXidHorizons(), we compute the minimum xid required by slots (procArray->replication_slot_xmin and procArray->replication_slot_catalog_xmin) but then don't adjust it for 'max_slot_xid_age'. I could be missing something in this but it is better to keep discussing this and try to move with another parameter 'inactive_replication_slot_timeout' which according to me can be kept at slot level instead of a GUC but OTOH we need to see the arguments on both side and then decide which makes more sense. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-17 08:33 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-17 08:33 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Mar 16, 2024 at 3:55 PM Amit Kapila <[email protected]> wrote: > > procArray->replication_slot_catalog_xmin) but then don't adjust it for > 'max_slot_xid_age'. I could be missing something in this but it is > better to keep discussing this and try to move with another parameter > 'inactive_replication_slot_timeout' which according to me can be kept > at slot level instead of a GUC but OTOH we need to see the arguments > on both side and then decide which makes more sense. Hm. Are you suggesting inactive_timeout to be a slot level parameter similar to 'failover' property added recently by c393308b69d229b664391ac583b9e07418d411b6 and 73292404370c9900a96e2bebdc7144f7010339cf? With this approach, one can set inactive_timeout while creating the slot either via pg_create_physical_replication_slot() or pg_create_logical_replication_slot() or CREATE_REPLICATION_SLOT or ALTER_REPLICATION_SLOT command, and postgres tracks the last_inactive_at for every slot based on which the slot gets invalidated. If this understanding is right, I can go ahead and work towards it. Alternatively, we can go the route of making GUC a list of key-value pairs of {slot_name, inactive_timeout}, but this kind of GUC for setting slot level parameters is going to be the first of its kind, so I'd prefer the above approach. Thoughts? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-18 03:20 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 32+ messages in thread From: Amit Kapila @ 2024-03-18 03:20 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Sun, Mar 17, 2024 at 2:03 PM Bharath Rupireddy <[email protected]> wrote: > > On Sat, Mar 16, 2024 at 3:55 PM Amit Kapila <[email protected]> wrote: > > > > procArray->replication_slot_catalog_xmin) but then don't adjust it for > > 'max_slot_xid_age'. I could be missing something in this but it is > > better to keep discussing this and try to move with another parameter > > 'inactive_replication_slot_timeout' which according to me can be kept > > at slot level instead of a GUC but OTOH we need to see the arguments > > on both side and then decide which makes more sense. > > Hm. Are you suggesting inactive_timeout to be a slot level parameter > similar to 'failover' property added recently by > c393308b69d229b664391ac583b9e07418d411b6 and > 73292404370c9900a96e2bebdc7144f7010339cf? With this approach, one can > set inactive_timeout while creating the slot either via > pg_create_physical_replication_slot() or > pg_create_logical_replication_slot() or CREATE_REPLICATION_SLOT or > ALTER_REPLICATION_SLOT command, and postgres tracks the > last_inactive_at for every slot based on which the slot gets > invalidated. If this understanding is right, I can go ahead and work > towards it. > Yeah, I have something like that in mind. You can prepare the patch but it would be good if others involved in this thread can also share their opinion. > Alternatively, we can go the route of making GUC a list of key-value > pairs of {slot_name, inactive_timeout}, but this kind of GUC for > setting slot level parameters is going to be the first of its kind, so > I'd prefer the above approach. > I would prefer a slot-level parameter in this case rather than a GUC. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-18 04:28 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 1 reply; 32+ messages in thread From: Bharath Rupireddy @ 2024-03-18 04:28 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Sat, Mar 16, 2024 at 3:55 PM Amit Kapila <[email protected]> wrote: > > > What's proposed with max_slot_xid_age is that during checkpoint we > > look at slot's xmin and catalog_xmin, and the current system txn id. > > Then, if the XID age of (xmin, catalog_xmin) and current_xid crosses > > max_slot_xid_age, we invalidate the slot. > > > > I can see that in your patch (in function > InvalidatePossiblyObsoleteSlot()). As per my understanding, we need > something similar for slot xids in ComputeXidHorizons() as we are > doing WAL in KeepLogSeg(). In KeepLogSeg(), we compute the minimum LSN > location required by slots and then adjust it for > 'max_slot_wal_keep_size'. On similar lines, currently in > ComputeXidHorizons(), we compute the minimum xid required by slots > (procArray->replication_slot_xmin and > procArray->replication_slot_catalog_xmin) but then don't adjust it for > 'max_slot_xid_age'. I could be missing something in this but it is > better to keep discussing this After invalidating slots because of max_slot_xid_age, the procArray->replication_slot_xmin and procArray->replication_slot_catalog_xmin are recomputed immediately in InvalidateObsoleteReplicationSlots->ReplicationSlotsComputeRequiredXmin->ProcArraySetReplicationSlotXmin. And, later the XID horizons in ComputeXidHorizons are computed before the vacuum on each table via GetOldestNonRemovableTransactionId. Aren't these enough? Do you want the XID horizons recomputed immediately, something like the below? /* Invalidate replication slots based on xmin or catalog_xmin age */ if (max_slot_xid_age > 0) { if (InvalidateObsoleteReplicationSlots(RS_INVAL_XID_AGE, 0, InvalidOid, InvalidTransactionId)) { ComputeXidHorizonsResult horizons; /* * Some slots have been invalidated; update the XID horizons * as a side-effect. */ ComputeXidHorizons(&horizons); } } -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-18 04:33 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Amit Kapila @ 2024-03-18 04:33 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 18, 2024 at 9:58 AM Bharath Rupireddy <[email protected]> wrote: > > On Sat, Mar 16, 2024 at 3:55 PM Amit Kapila <[email protected]> wrote: > > > > > What's proposed with max_slot_xid_age is that during checkpoint we > > > look at slot's xmin and catalog_xmin, and the current system txn id. > > > Then, if the XID age of (xmin, catalog_xmin) and current_xid crosses > > > max_slot_xid_age, we invalidate the slot. > > > > > > > I can see that in your patch (in function > > InvalidatePossiblyObsoleteSlot()). As per my understanding, we need > > something similar for slot xids in ComputeXidHorizons() as we are > > doing WAL in KeepLogSeg(). In KeepLogSeg(), we compute the minimum LSN > > location required by slots and then adjust it for > > 'max_slot_wal_keep_size'. On similar lines, currently in > > ComputeXidHorizons(), we compute the minimum xid required by slots > > (procArray->replication_slot_xmin and > > procArray->replication_slot_catalog_xmin) but then don't adjust it for > > 'max_slot_xid_age'. I could be missing something in this but it is > > better to keep discussing this > > After invalidating slots because of max_slot_xid_age, the > procArray->replication_slot_xmin and > procArray->replication_slot_catalog_xmin are recomputed immediately in > InvalidateObsoleteReplicationSlots->ReplicationSlotsComputeRequiredXmin->ProcArraySetReplicationSlotXmin. > And, later the XID horizons in ComputeXidHorizons are computed before > the vacuum on each table via GetOldestNonRemovableTransactionId. > Aren't these enough? > IIUC, this will be delayed by one cycle in the vacuum rather than doing it when the slot's xmin age is crossed and it can be invalidated. Do you want the XID horizons recomputed > immediately, something like the below? > I haven't thought of the exact logic but we can try to mimic the handling similar to WAL. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Introduce XID age and inactive timeout based replication slot invalidation @ 2024-03-18 09:32 Bertrand Drouvot <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 0 replies; 32+ messages in thread From: Bertrand Drouvot @ 2024-03-18 09:32 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Mon, Mar 18, 2024 at 08:50:56AM +0530, Amit Kapila wrote: > On Sun, Mar 17, 2024 at 2:03 PM Bharath Rupireddy > <[email protected]> wrote: > > > > On Sat, Mar 16, 2024 at 3:55 PM Amit Kapila <[email protected]> wrote: > > > > > > procArray->replication_slot_catalog_xmin) but then don't adjust it for > > > 'max_slot_xid_age'. I could be missing something in this but it is > > > better to keep discussing this and try to move with another parameter > > > 'inactive_replication_slot_timeout' which according to me can be kept > > > at slot level instead of a GUC but OTOH we need to see the arguments > > > on both side and then decide which makes more sense. > > > > Hm. Are you suggesting inactive_timeout to be a slot level parameter > > similar to 'failover' property added recently by > > c393308b69d229b664391ac583b9e07418d411b6 and > > 73292404370c9900a96e2bebdc7144f7010339cf? With this approach, one can > > set inactive_timeout while creating the slot either via > > pg_create_physical_replication_slot() or > > pg_create_logical_replication_slot() or CREATE_REPLICATION_SLOT or > > ALTER_REPLICATION_SLOT command, and postgres tracks the > > last_inactive_at for every slot based on which the slot gets > > invalidated. If this understanding is right, I can go ahead and work > > towards it. > > > > Yeah, I have something like that in mind. You can prepare the patch > but it would be good if others involved in this thread can also share > their opinion. I think it makes sense to put the inactive_timeout granularity at the slot level (as the activity could vary a lot say between one slot linked to a subcription and one linked to some plugins). As far max_slot_xid_age I've the feeling that a new GUC is good enough. > > Alternatively, we can go the route of making GUC a list of key-value > > pairs of {slot_name, inactive_timeout}, but this kind of GUC for > > setting slot level parameters is going to be the first of its kind, so > > I'd prefer the above approach. > > > > I would prefer a slot-level parameter in this case rather than a GUC. Yeah, same here. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 32+ messages in thread
end of thread, other threads:[~2024-03-18 09:32 UTC | newest] Thread overview: 32+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-04-21 17:33 [PATCH v2 3/4] Add a new MODE_SINGLE_QUERY to the core parser and use it in pg_parse_query. Julien Rouhaud <[email protected]> 2024-03-03 18:10 Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-03 21:44 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Nathan Bossart <[email protected]> 2024-03-04 01:32 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Michael Paquier <[email protected]> 2024-03-04 08:41 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-05 19:20 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-05 19:44 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Nathan Bossart <[email protected]> 2024-03-06 09:12 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-06 09:16 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-06 10:26 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-06 11:19 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-08 17:12 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-11 10:14 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-12 16:39 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-13 04:24 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-15 05:14 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-16 10:24 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-17 08:33 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-18 03:20 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-18 09:32 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-18 04:28 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-18 04:33 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-12 07:54 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-12 12:21 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-12 15:40 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-12 15:49 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-13 07:21 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bertrand Drouvot <[email protected]> 2024-03-13 04:08 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]> 2024-03-12 15:44 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Bharath Rupireddy <[email protected]> 2024-03-13 05:43 ` Re: Introduce XID age and inactive timeout based replication slot invalidation shveta malik <[email protected]> 2024-03-13 07:15 ` Re: Introduce XID age and inactive timeout based replication slot invalidation shveta malik <[email protected]> 2024-03-06 10:58 ` Re: Introduce XID age and inactive timeout based replication slot invalidation Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox