($INBOX_DIR/description missing)help / color / mirror / Atom feed
[PATCH v11 3/4] Remove globals readOff, readLen and readSegNo 25+ messages / 5 participants [nested] [flat]
* [PATCH v11 3/4] Remove globals readOff, readLen and readSegNo @ 2019-09-10 08:28 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Kyotaro Horiguchi @ 2019-09-10 08:28 UTC (permalink / raw) The first two variables are functionally duplicate with them in XLogReaderState. Remove the globals along with readSegNo, which behaves in the similar way. --- src/backend/access/transam/xlog.c | 77 ++++++++++++++----------------- src/include/access/xlogreader.h | 1 + 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index a4e606cebf..364b1948e4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -783,14 +783,10 @@ static XLogSegNo openLogSegNo = 0; * These variables are used similarly to the ones above, but for reading * the XLOG. Note, however, that readOff generally represents the offset * of the page just read, not the seek position of the FD itself, which - * will be just past that page. readLen indicates how much of the current - * page has been read into readBuf, and readSource indicates where we got - * the currently open file from. + * will be just past that page. readSource indicates where we got the + * currently open file from. */ static int readFile = -1; -static XLogSegNo readSegNo = 0; -static uint32 readOff = 0; -static uint32 readLen = 0; static XLogSource readSource = 0; /* XLOG_FROM_* code */ /* @@ -877,10 +873,12 @@ static bool InstallXLogFileSegment(XLogSegNo *segno, char *tmppath, static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli, int source, bool notfoundOk); static int XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source); -static bool XLogPageRead(XLogReaderState *xlogreader, +static bool XLogPageRead(XLogReaderState *state, bool fetching_ckpt, int emode, bool randAccess); static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess, - bool fetching_ckpt, XLogRecPtr tliRecPtr); + bool fetching_ckpt, + XLogRecPtr tliRecPtr, + XLogSegNo readSegNo); static int emode_for_corrupt_record(int emode, XLogRecPtr RecPtr); static void XLogFileClose(void); static void PreallocXlogFiles(XLogRecPtr endptr); @@ -7509,7 +7507,8 @@ StartupXLOG(void) XLogRecPtr pageBeginPtr; pageBeginPtr = EndOfLog - (EndOfLog % XLOG_BLCKSZ); - Assert(readOff == XLogSegmentOffset(pageBeginPtr, wal_segment_size)); + Assert(XLogSegmentOffset(xlogreader->readPagePtr, wal_segment_size) == + XLogSegmentOffset(pageBeginPtr, wal_segment_size)); firstIdx = XLogRecPtrToBufIdx(EndOfLog); @@ -11529,13 +11528,14 @@ CancelBackup(void) * sleep and retry. */ static bool -XLogPageRead(XLogReaderState *xlogreader, +XLogPageRead(XLogReaderState *state, bool fetching_ckpt, int emode, bool randAccess) { - char *readBuf = xlogreader->readBuf; - XLogRecPtr targetPagePtr = xlogreader->readPagePtr; - int reqLen = xlogreader->readLen; - XLogRecPtr targetRecPtr = xlogreader->ReadRecPtr; + char *readBuf = state->readBuf; + XLogRecPtr targetPagePtr = state->readPagePtr; + int reqLen = state->readLen; + int readLen = 0; + XLogRecPtr targetRecPtr = state->ReadRecPtr; uint32 targetPageOff; XLogSegNo targetSegNo PG_USED_FOR_ASSERTS_ONLY; int r; @@ -11548,7 +11548,7 @@ XLogPageRead(XLogReaderState *xlogreader, * is not in the currently open one. */ if (readFile >= 0 && - !XLByteInSeg(targetPagePtr, readSegNo, wal_segment_size)) + !XLByteInSeg(targetPagePtr, state->readSegNo, wal_segment_size)) { /* * Request a restartpoint if we've replayed too much xlog since the @@ -11556,10 +11556,10 @@ XLogPageRead(XLogReaderState *xlogreader, */ if (bgwriterLaunched) { - if (XLogCheckpointNeeded(readSegNo)) + if (XLogCheckpointNeeded(state->readSegNo)) { (void) GetRedoRecPtr(); - if (XLogCheckpointNeeded(readSegNo)) + if (XLogCheckpointNeeded(state->readSegNo)) RequestCheckpoint(CHECKPOINT_CAUSE_XLOG); } } @@ -11569,7 +11569,7 @@ XLogPageRead(XLogReaderState *xlogreader, readSource = 0; } - XLByteToSeg(targetPagePtr, readSegNo, wal_segment_size); + XLByteToSeg(targetPagePtr, state->readSegNo, wal_segment_size); retry: /* See if we need to retrieve more data */ @@ -11578,17 +11578,14 @@ retry: receivedUpto < targetPagePtr + reqLen)) { if (!WaitForWALToBecomeAvailable(targetPagePtr + reqLen, - randAccess, - fetching_ckpt, - targetRecPtr)) + randAccess, fetching_ckpt, + targetRecPtr, state->readSegNo)) { if (readFile >= 0) close(readFile); readFile = -1; - readLen = 0; readSource = 0; - - xlogreader->readLen = -1; + state->readLen = -1; return false; } } @@ -11616,40 +11613,36 @@ retry: else readLen = XLOG_BLCKSZ; - /* Read the requested page */ - readOff = targetPageOff; - pgstat_report_wait_start(WAIT_EVENT_WAL_READ); - r = pg_pread(readFile, readBuf, XLOG_BLCKSZ, (off_t) readOff); + r = pg_pread(readFile, readBuf, XLOG_BLCKSZ, (off_t) targetPageOff); if (r != XLOG_BLCKSZ) { char fname[MAXFNAMELEN]; int save_errno = errno; pgstat_report_wait_end(); - XLogFileName(fname, curFileTLI, readSegNo, wal_segment_size); + XLogFileName(fname, curFileTLI, state->readSegNo, wal_segment_size); if (r < 0) { errno = save_errno; ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen), (errcode_for_file_access(), errmsg("could not read from log segment %s, offset %u: %m", - fname, readOff))); + fname, targetPageOff))); } else ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen), (errcode(ERRCODE_DATA_CORRUPTED), errmsg("could not read from log segment %s, offset %u: read %d of %zu", - fname, readOff, r, (Size) XLOG_BLCKSZ))); + fname, targetPageOff, r, (Size) XLOG_BLCKSZ))); goto next_record_is_invalid; } pgstat_report_wait_end(); - Assert(targetSegNo == readSegNo); - Assert(targetPageOff == readOff); + Assert(targetSegNo == state->readSegNo); Assert(reqLen <= readLen); - xlogreader->seg.ws_tli = curFileTLI; + state->seg.ws_tli = curFileTLI; /* * Check the page header immediately, so that we can retry immediately if @@ -11677,15 +11670,15 @@ retry: * Validating the page header is cheap enough that doing it twice * shouldn't be a big deal from a performance point of view. */ - if (!XLogReaderValidatePageHeader(xlogreader, targetPagePtr, readBuf)) + if (!XLogReaderValidatePageHeader(state, targetPagePtr, readBuf)) { - /* reset any error XLogReaderValidatePageHeader() might have set */ - xlogreader->errormsg_buf[0] = '\0'; + /* reset any error StateValidatePageHeader() might have set */ + state->errormsg_buf[0] = '\0'; goto next_record_is_invalid; } - Assert(xlogreader->readPagePtr == targetPagePtr); - xlogreader->readLen = readLen; + Assert(state->readPagePtr == targetPagePtr); + state->readLen = readLen; return true; next_record_is_invalid: @@ -11694,14 +11687,13 @@ next_record_is_invalid: if (readFile >= 0) close(readFile); readFile = -1; - readLen = 0; readSource = 0; /* In standby-mode, keep trying */ if (StandbyMode) goto retry; - xlogreader->readLen = -1; + state->readLen = -1; return false; } @@ -11733,7 +11725,8 @@ next_record_is_invalid: */ static bool WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess, - bool fetching_ckpt, XLogRecPtr tliRecPtr) + bool fetching_ckpt, XLogRecPtr tliRecPtr, + XLogSegNo readSegNo) { static TimestampTz last_fail_time = 0; TimestampTz now; diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 2494b2416f..f6249a5844 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -132,6 +132,7 @@ struct XLogReaderState * read by reader, which must be larger than * the request, or -1 on error */ TimeLineID readPageTLI; /* TLI for data currently in readBuf */ + XLogSegNo readSegNo; /* Segment # for data currently in readBuf */ char *readBuf; /* buffer to store data */ bool page_verified; /* is the page header on the buffer verified? */ bool record_verified;/* is the current record header verified? */ -- 2.23.0 ----Next_Part(Wed_Nov_27_12_09_23_2019_240)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v11-0004-Change-policy-of-XLog-read-buffer-allocation.patch" ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v2 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 8 files changed, 105 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 4c49393fc5..33a13fdf32 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - /* * Truncate given database and user names to length of a Postgres name. * This avoids lookup failures when overlength names are given. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71e27f8eb0..25d9008bb6 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --X1bOJ3K7DJ5YkBrT-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v1 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 7 files changed, 100 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 4c49393fc5..33a13fdf32 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - /* * Truncate given database and user names to length of a Postgres name. * This avoids lookup failures when overlength names are given. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71e27f8eb0..25d9008bb6 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --Kj7319i9nmIyA2yE-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v4 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/commands/variable.c | 15 ++++++ src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 16 ++++-- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - src/include/utils/guc_hooks.h | 1 + .../unsafe_tests/expected/guc_privs.out | 4 ++ .../modules/unsafe_tests/sql/guc_privs.sql | 3 ++ 12 files changed, 35 insertions(+), 100 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index f0f2e07655..b6a2fa2512 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -1157,6 +1157,21 @@ check_bonjour(bool *newval, void **extra, GucSource source) return true; } +bool +check_db_user_namespace(bool *newval, void **extra, GucSource source) +{ + if (*newval) + { + /* check the GUC's definition for an explanation */ + GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED); + GUC_check_errmsg("db_user_namespace is not supported"); + + return false; + } + + return true; +} + bool check_default_with_oids(bool *newval, void **extra, GucSource source) { diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 0b1de9efb2..9c8ec779f9 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - if (am_walsender) MyBackendType = B_WAL_SENDER; else diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index f8ef87d26d..94e87b7bd4 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -505,6 +505,7 @@ bool check_function_bodies = true; */ bool default_with_oids = false; bool session_auth_is_superuser; +bool Db_user_namespace = false; int log_min_error_statement = ERROR; int log_min_messages = WARNING; @@ -1534,14 +1535,21 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, + + /* + * db_user_namespace was removed in PostgreSQL 17, but we tolerate the + * parameter being set to false to avoid unnecessarily breaking older dump + * files. + */ { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL + {"db_user_namespace", PGC_SIGHUP, COMPAT_OPTIONS_PREVIOUS, + gettext_noop("db_user_namespace is no longer supported; this can only be false."), + NULL, + GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE }, &Db_user_namespace, false, - NULL, NULL, NULL + check_db_user_namespace, NULL, NULL }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 2ecb9fc086..c51d44ec15 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -49,6 +49,7 @@ extern bool check_cluster_name(char **newval, void **extra, GucSource source); extern const char *show_data_directory_mode(void); extern bool check_datestyle(char **newval, void **extra, GucSource source); extern void assign_datestyle(const char *newval, void *extra); +extern bool check_db_user_namespace(bool *newval, void **extra, GucSource source); extern bool check_default_table_access_method(char **newval, void **extra, GucSource source); extern bool check_default_tablespace(char **newval, void **extra, diff --git a/src/test/modules/unsafe_tests/expected/guc_privs.out b/src/test/modules/unsafe_tests/expected/guc_privs.out index f43a1da214..17f7a0c980 100644 --- a/src/test/modules/unsafe_tests/expected/guc_privs.out +++ b/src/test/modules/unsafe_tests/expected/guc_privs.out @@ -40,6 +40,10 @@ RESET autovacuum; -- fail, requires reload ERROR: parameter "autovacuum" cannot be changed now ALTER SYSTEM SET autovacuum = OFF; -- ok ALTER SYSTEM RESET autovacuum; -- ok +ALTER SYSTEM SET db_user_namespace = OFF; -- ok +ALTER SYSTEM SET db_user_namespace = ON; -- fail, cannot be changed +ERROR: db_user_namespace is not supported +ALTER SYSTEM RESET db_user_namespace; -- ok -- PGC_SUSET SET lc_messages = 'C'; -- ok RESET lc_messages; -- ok diff --git a/src/test/modules/unsafe_tests/sql/guc_privs.sql b/src/test/modules/unsafe_tests/sql/guc_privs.sql index 7a4fb24b9d..233ce1a5ac 100644 --- a/src/test/modules/unsafe_tests/sql/guc_privs.sql +++ b/src/test/modules/unsafe_tests/sql/guc_privs.sql @@ -31,6 +31,9 @@ SET autovacuum = OFF; -- fail, requires reload RESET autovacuum; -- fail, requires reload ALTER SYSTEM SET autovacuum = OFF; -- ok ALTER SYSTEM RESET autovacuum; -- ok +ALTER SYSTEM SET db_user_namespace = OFF; -- ok +ALTER SYSTEM SET db_user_namespace = ON; -- fail, cannot be changed +ALTER SYSTEM RESET db_user_namespace; -- ok -- PGC_SUSET SET lc_messages = 'C'; -- ok RESET lc_messages; -- ok -- 2.25.1 --jRHKVT23PllUwdXP-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v3 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 8 files changed, 105 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 0b1de9efb2..9c8ec779f9 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - if (am_walsender) MyBackendType = B_WAL_SENDER; else diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index f8ef87d26d..0c38af3f69 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --CE+1k2dSO48ffgeK-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v3 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 8 files changed, 105 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 0b1de9efb2..9c8ec779f9 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - if (am_walsender) MyBackendType = B_WAL_SENDER; else diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index f8ef87d26d..0c38af3f69 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --CE+1k2dSO48ffgeK-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v2 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/client-auth.sgml | 5 -- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 8 files changed, 105 deletions(-) diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 204d09df67..6c95f0df1e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1253,11 +1253,6 @@ omicron bryanh guest1 attacks. </para> - <para> - The <literal>md5</literal> method cannot be used with - the <xref linkend="guc-db-user-namespace"/> feature. - </para> - <para> To ease transition from the <literal>md5</literal> method to the newer SCRAM method, if <literal>md5</literal> is specified as a method diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 4c49393fc5..33a13fdf32 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - /* * Truncate given database and user names to length of a Postgres name. * This avoids lookup failures when overlength names are given. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71e27f8eb0..25d9008bb6 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --X1bOJ3K7DJ5YkBrT-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* [PATCH v1 1/1] remove db_user_namespace @ 2023-06-30 19:46 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Nathan Bossart @ 2023-06-30 19:46 UTC (permalink / raw) --- doc/src/sgml/config.sgml | 52 ------------------- src/backend/libpq/auth.c | 5 -- src/backend/libpq/hba.c | 12 ----- src/backend/postmaster/postmaster.c | 19 ------- src/backend/utils/misc/guc_tables.c | 9 ---- src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/libpq/pqcomm.h | 2 - 7 files changed, 100 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6262cb7bb2..e6cea8ddfc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1188,58 +1188,6 @@ include_dir 'conf.d' </para> </listitem> </varlistentry> - - <varlistentry id="guc-db-user-namespace" xreflabel="db_user_namespace"> - <term><varname>db_user_namespace</varname> (<type>boolean</type>) - <indexterm> - <primary><varname>db_user_namespace</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - This parameter enables per-database user names. It is off by default. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - - <para> - If this is on, you should create users as <replaceable>username@dbname</replaceable>. - When <replaceable>username</replaceable> is passed by a connecting client, - <literal>@</literal> and the database name are appended to the user - name and that database-specific user name is looked up by the - server. Note that when you create users with names containing - <literal>@</literal> within the SQL environment, you will need to - quote the user name. - </para> - - <para> - With this parameter enabled, you can still create ordinary global - users. Simply append <literal>@</literal> when specifying the user - name in the client, e.g., <literal>joe@</literal>. The <literal>@</literal> - will be stripped off before the user name is looked up by the - server. - </para> - - <para> - <varname>db_user_namespace</varname> causes the client's and - server's user name representation to differ. - Authentication checks are always done with the server's user name - so authentication methods must be configured for the - server's user name, not the client's. Because - <literal>md5</literal> uses the user name as salt on both the - client and server, <literal>md5</literal> cannot be used with - <varname>db_user_namespace</varname>. - </para> - - <note> - <para> - This feature is intended as a temporary measure until a - complete solution is found. At that time, this option will - be removed. - </para> - </note> - </listitem> - </varlistentry> </variablelist> </sect2> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index a98b934a8e..65d452f099 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -873,11 +873,6 @@ CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail) char *passwd; int result; - if (Db_user_namespace) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"))); - /* include the salt to use for computing the response */ if (!pg_strong_random(md5Salt, 4)) { diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f89f138f3c..5d4ddbb04d 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1741,19 +1741,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel) else if (strcmp(token->string, "reject") == 0) parsedline->auth_method = uaReject; else if (strcmp(token->string, "md5") == 0) - { - if (Db_user_namespace) - { - ereport(elevel, - (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled"), - errcontext("line %d of configuration file \"%s\"", - line_num, file_name))); - *err_msg = "MD5 authentication is not supported when \"db_user_namespace\" is enabled"; - return NULL; - } parsedline->auth_method = uaMD5; - } else if (strcmp(token->string, "scram-sha-256") == 0) parsedline->auth_method = uaSCRAM; else if (strcmp(token->string, "pam") == 0) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 4c49393fc5..33a13fdf32 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -236,7 +236,6 @@ int AuthenticationTimeout = 60; bool log_hostname; /* for ps display and logging */ bool Log_connections = false; -bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; @@ -2272,24 +2271,6 @@ retry1: if (port->database_name == NULL || port->database_name[0] == '\0') port->database_name = pstrdup(port->user_name); - if (Db_user_namespace) - { - /* - * If user@, it is a global user, remove '@'. We only want to do this - * if there is an '@' at the end and no earlier in the user string or - * they may fake as a local user of another database attaching to this - * database. - */ - if (strchr(port->user_name, '@') == - port->user_name + strlen(port->user_name) - 1) - *strchr(port->user_name, '@') = '\0'; - else - { - /* Append '@' and dbname */ - port->user_name = psprintf("%s@%s", port->user_name, port->database_name); - } - } - /* * Truncate given database and user names to length of a Postgres name. * This avoids lookup failures when overlength names are given. diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 71e27f8eb0..25d9008bb6 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -1534,15 +1534,6 @@ struct config_bool ConfigureNamesBool[] = false, NULL, NULL, NULL }, - { - {"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH, - gettext_noop("Enables per-database user names."), - NULL - }, - &Db_user_namespace, - false, - NULL, NULL, NULL - }, { {"default_transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the default read-only status of new transactions."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e4c0269fa3..c768af9a73 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -96,7 +96,6 @@ #authentication_timeout = 1min # 1s-600s #password_encryption = scram-sha-256 # scram-sha-256 or md5 #scram_iterations = 4096 -#db_user_namespace = off # GSSAPI using Kerberos #krb_server_keyfile = 'FILE:${sysconfdir}/krb5.keytab' diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h index c85090259d..3da00f7983 100644 --- a/src/include/libpq/pqcomm.h +++ b/src/include/libpq/pqcomm.h @@ -103,8 +103,6 @@ typedef ProtocolVersion MsgType; typedef uint32 PacketLen; -extern PGDLLIMPORT bool Db_user_namespace; - /* * In protocol 3.0 and later, the startup packet length is not fixed, but * we set an arbitrary limit on it anyway. This is just to prevent simple -- 2.25.1 --Kj7319i9nmIyA2yE-- ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-16 14:30 Anton A. Melnikov <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-09-16 14:30 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Hi! On 13.09.2024 18:20, Fujii Masao wrote: > > If I understand correctly, restartpoints_timed and restartpoints_done were > separated because a restartpoint can be skipped. restartpoints_timed counts > when a restartpoint is triggered by a timeout, whether it runs or not, > while restartpoints_done only tracks completed restartpoints. > > Similarly, I believe checkpoints should be handled the same way. > Checkpoints can also be skipped when the system is idle, but currently, > num_timed counts even the skipped ones, despite its documentation stating > it's the "Number of scheduled checkpoints that have been performed." > > Why not separate num_timed into something like checkpoints_timed and > checkpoints_done to reflect these different counters? +1 This idea seems quite tenable to me. There is a small clarification. Now if there were no skipped restartpoints then restartpoints_done will be equal to restartpoints_timed + restartpoints_req. Similar for checkpoints. So i tried to introduce num_done counter for checkpoints in the patch attached. I'm not sure should we include testing for the case when num_done is less than num_timed + num_requested to the regress tests. I haven't been able to get it in a short time yet. E.g. such a case may be obtained when an a error "checkpoints are occurring too frequently" as follows: -set checkpoint_timeout = 30 and checkpoint_warning = 40 in the postgresql.conf -start server -do periodically bulk insertions in the 1st client (e.g. insert into test values (generate_series(1,1E7));) -watch for pg_stat_checkpointer in the 2nd one: # SELECT CURRENT_TIME; select * from pg_stat_checkpointer; # \watch After some time, in the log will appear: 2024-09-16 16:38:47.888 MSK [193733] LOG: checkpoints are occurring too frequently (13 seconds apart) 2024-09-16 16:38:47.888 MSK [193733] HINT: Consider increasing the configuration parameter "max_wal_size". And num_timed + num_requested will become greater than num_done. Would be nice to find some simpler and faster way. With the best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Attachments: [text/x-patch] v1-0001-Introduce-num_done-counter-in-the-pg_stat_checkpointer.patch (10.7K, ../../[email protected]/2-v1-0001-Introduce-num_done-counter-in-the-pg_stat_checkpointer.patch) download | inline diff: From fcd0b61d1f1718dbf664cb3509aad16543d65375 Mon Sep 17 00:00:00 2001 From: "Anton A. Melnikov" <[email protected]> Date: Mon, 16 Sep 2024 16:12:07 +0300 Subject: [PATCH] Introduce num_done counter in the pg_stat_checkpointer view that reflects number of really performed checkpoints. --- doc/src/sgml/monitoring.sgml | 13 +++++- src/backend/catalog/system_views.sql | 1 + src/backend/postmaster/checkpointer.c | 2 + .../utils/activity/pgstat_checkpointer.c | 2 + src/backend/utils/adt/pgstatfuncs.c | 6 +++ src/include/catalog/pg_proc.dat | 40 +++++++++++-------- src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 1 + 8 files changed, 47 insertions(+), 19 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 933de6fe07f..dad7e236a43 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3051,7 +3051,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_timed</structfield> <type>bigint</type> </para> <para> - Number of scheduled checkpoints that have been performed + Number of scheduled checkpoints </para></entry> </row> @@ -3060,7 +3060,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of requested checkpoints that have been performed + Number of backend requested checkpoints + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>num_done</structfield> <type>bigint</type> + </para> + <para> + Number of checkpoints that have been performed </para></entry> </row> diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a18..49109dbdc86 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index eeb73c85726..06ad2f52f27 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -495,6 +495,8 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) if (do_restartpoint) PendingCheckpointerStats.restartpoints_performed++; + else + PendingCheckpointerStats.num_performed++; } else { diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c index bbfc9c7e183..4a0a2d1493a 100644 --- a/src/backend/utils/activity/pgstat_checkpointer.c +++ b/src/backend/utils/activity/pgstat_checkpointer.c @@ -49,6 +49,7 @@ pgstat_report_checkpointer(void) #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld CHECKPOINTER_ACC(num_timed); CHECKPOINTER_ACC(num_requested); + CHECKPOINTER_ACC(num_performed); CHECKPOINTER_ACC(restartpoints_timed); CHECKPOINTER_ACC(restartpoints_requested); CHECKPOINTER_ACC(restartpoints_performed); @@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void) #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld; CHECKPOINTER_COMP(num_timed); CHECKPOINTER_COMP(num_requested); + CHECKPOINTER_COMP(num_performed); CHECKPOINTER_COMP(restartpoints_timed); CHECKPOINTER_COMP(restartpoints_requested); CHECKPOINTER_COMP(restartpoints_performed); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 33c7b25560b..5624bef18e7 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS) PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested); } +Datum +pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed); +} + Datum pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS) { diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 53a081ed886..c8c1b79ff63 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5807,42 +5807,58 @@ proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{archived_count,last_archived_wal,last_archived_time,failed_count,last_failed_wal,last_failed_time,stats_reset}', prosrc => 'pg_stat_get_archiver' }, -{ oid => '2769', +{ oid => '6347', descr => 'statistics: number of timed checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_timed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_timed' }, -{ oid => '2770', +{ oid => '6348', descr => 'statistics: number of backend requested checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, -{ oid => '6327', +{ oid => '6349', + descr => 'statistics: number of checkpoints performed by the checkpointer', + proname => 'pg_stat_get_checkpointer_num_performed', + provolatile => 's', proparallel => 'r', prorettype => 'int8', + proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_num_performed' }, +{ oid => '6350', descr => 'statistics: number of timed restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_timed' }, -{ oid => '6328', +{ oid => '6351', descr => 'statistics: number of backend requested restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_requested' }, -{ oid => '6329', +{ oid => '6352', descr => 'statistics: number of backend performed restartpoints', proname => 'pg_stat_get_checkpointer_restartpoints_performed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_performed' }, -{ oid => '2771', +{ oid => '6353', descr => 'statistics: number of buffers written during checkpoints and restartpoints', proname => 'pg_stat_get_checkpointer_buffers_written', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_buffers_written' }, -{ oid => '6314', descr => 'statistics: last reset for the checkpointer', +{ oid => '6354', descr => 'statistics: last reset for the checkpointer', proname => 'pg_stat_get_checkpointer_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_stat_reset_time' }, +{ oid => '6355', + descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_write_time' }, +{ oid => '6356', + descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2772', descr => 'statistics: number of buffers written by the bgwriter for cleaning dirty buffers', proname => 'pg_stat_get_bgwriter_buf_written_clean', provolatile => 's', @@ -5857,16 +5873,6 @@ proname => 'pg_stat_get_bgwriter_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_bgwriter_stat_reset_time' }, -{ oid => '3160', - descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_write_time' }, -{ oid => '3161', - descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2859', descr => 'statistics: number of buffer allocations', proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' }, diff --git a/src/include/pgstat.h b/src/include/pgstat.h index be2c91168a1..39850d8f14f 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; PgStat_Counter num_requested; + PgStat_Counter num_performed; PgStat_Counter restartpoints_timed; PgStat_Counter restartpoints_requested; PgStat_Counter restartpoints_performed; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae9..f5434d8365c 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle pg_stat_get_bgwriter_stat_reset_time() AS stats_reset; pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, -- 2.46.0 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-17 02:47 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Fujii Masao @ 2024-09-17 02:47 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/16 23:30, Anton A. Melnikov wrote: > +1 > This idea seems quite tenable to me. > > There is a small clarification. Now if there were no skipped restartpoints then > restartpoints_done will be equal to restartpoints_timed + restartpoints_req. > Similar for checkpoints. > So i tried to introduce num_done counter for checkpoints in the patch attached. Thanks for the patch! I believe this change is targeted for v18. For v17, however, we should update the description of num_timed in the documentation. Thought? Here's a suggestion: "Number of scheduled checkpoints due to timeout. Note that checkpoints may be skipped if the server has been idle since the last one, and this value counts both completed and skipped checkpoints." Regarding the patch: if (do_restartpoint) PendingCheckpointerStats.restartpoints_performed++; + else + PendingCheckpointerStats.num_performed++; I expected the counter not to be incremented when a checkpoint is skipped, but in this code, when a checkpoint is skipped, ckpt_performed is set to true, triggering the counter increment. This seems wrong. > I'm not sure should we include testing for the case when num_done is less than > num_timed + num_requested to the regress tests. I haven't been able to get it in a short time yet. I'm not sure if that test is really necessary... Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-18 10:21 Fujii Masao <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Fujii Masao @ 2024-09-18 10:21 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/17 11:47, Fujii Masao wrote: > > > On 2024/09/16 23:30, Anton A. Melnikov wrote: >> +1 >> This idea seems quite tenable to me. >> >> There is a small clarification. Now if there were no skipped restartpoints then >> restartpoints_done will be equal to restartpoints_timed + restartpoints_req. >> Similar for checkpoints. >> So i tried to introduce num_done counter for checkpoints in the patch attached. > > Thanks for the patch! I believe this change is targeted for v18. For v17, however, > we should update the description of num_timed in the documentation. Thought? > Here's a suggestion: > > "Number of scheduled checkpoints due to timeout. Note that checkpoints may be > skipped if the server has been idle since the last one, and this value counts > both completed and skipped checkpoints." Patch attached. Unless there are any objections, I plan to commit this and back-patch it to v17. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION From 96dd9e0f923aef44ee1d73ea2bafea8d9db805bf Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Wed, 18 Sep 2024 19:03:53 +0900 Subject: [PATCH v1] docs: Improve the description of num_timed column in pg_stat_checkpointer. The previous documentation stated that num_timed reflects the number of scheduled checkpoints performed. However, checkpoints may be skipped if the server has been idle, and num_timed counts both skipped and completed checkpoints. This commit clarifies the description to make it clear that the counter includes both skipped and completed checkpoints. Back-patch to v17 where pg_stat_checkpointer was added. Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/monitoring.sgml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 933de6fe07..a2fda4677d 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3051,7 +3051,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_timed</structfield> <type>bigint</type> </para> <para> - Number of scheduled checkpoints that have been performed + Number of scheduled checkpoints due to timeout. + Note that checkpoints may be skipped if the server has been idle + since the last one, and this value counts both completed and + skipped checkpoints </para></entry> </row> -- 2.45.2 Attachments: [text/plain] v1-0001-docs-Improve-the-description-of-num_timed-column-.patch (1.5K, ../../[email protected]/2-v1-0001-docs-Improve-the-description-of-num_timed-column-.patch) download | inline diff: From 96dd9e0f923aef44ee1d73ea2bafea8d9db805bf Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Wed, 18 Sep 2024 19:03:53 +0900 Subject: [PATCH v1] docs: Improve the description of num_timed column in pg_stat_checkpointer. The previous documentation stated that num_timed reflects the number of scheduled checkpoints performed. However, checkpoints may be skipped if the server has been idle, and num_timed counts both skipped and completed checkpoints. This commit clarifies the description to make it clear that the counter includes both skipped and completed checkpoints. Back-patch to v17 where pg_stat_checkpointer was added. Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/monitoring.sgml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 933de6fe07..a2fda4677d 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3051,7 +3051,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_timed</structfield> <type>bigint</type> </para> <para> - Number of scheduled checkpoints that have been performed + Number of scheduled checkpoints due to timeout. + Note that checkpoints may be skipped if the server has been idle + since the last one, and this value counts both completed and + skipped checkpoints </para></entry> </row> -- 2.45.2 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-18 12:22 Alexander Korotkov <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Alexander Korotkov @ 2024-09-18 12:22 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Anton A. Melnikov <[email protected]>; Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On Wed, Sep 18, 2024 at 1:21 PM Fujii Masao <[email protected]> wrote: > On 2024/09/17 11:47, Fujii Masao wrote: > > > > > > On 2024/09/16 23:30, Anton A. Melnikov wrote: > >> +1 > >> This idea seems quite tenable to me. > >> > >> There is a small clarification. Now if there were no skipped restartpoints then > >> restartpoints_done will be equal to restartpoints_timed + restartpoints_req. > >> Similar for checkpoints. > >> So i tried to introduce num_done counter for checkpoints in the patch attached. > > > > Thanks for the patch! I believe this change is targeted for v18. For v17, however, > > we should update the description of num_timed in the documentation. Thought? > > Here's a suggestion: > > > > "Number of scheduled checkpoints due to timeout. Note that checkpoints may be > > skipped if the server has been idle since the last one, and this value counts > > both completed and skipped checkpoints." > > Patch attached. > Unless there are any objections, I plan to commit this and back-patch it to v17. I've checked this patch, it looks good to me. Generally, it looks like I should be in charge for this, given I've committed previous patch by Anton. Thank you for reacting here faster than me. Please, go ahead with the patch. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-18 14:35 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-09-18 14:35 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] Fujii, Alexander thanks a lot! On 17.09.2024 05:47, Fujii Masao wrote: > > Regarding the patch: > if (do_restartpoint) > PendingCheckpointerStats.restartpoints_performed++; > + else > + PendingCheckpointerStats.num_performed++; > > I expected the counter not to be incremented when a checkpoint is skipped, > but in this code, when a checkpoint is skipped, ckpt_performed is set to true, > triggering the counter increment. This seems wrong. Tried to fix it via returning bool value from the CreateCheckPoint() similarly to the CreateRestartPoint(). And slightly adjusted the patch so that it could be applied after yours. With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Attachments: [text/x-patch] v2-0002-Introduce-num_done-counter-in-the-pg_stat_checkpoint.patch (12.2K, ../../[email protected]/2-v2-0002-Introduce-num_done-counter-in-the-pg_stat_checkpoint.patch) download | inline diff: From 97595f65cb12eb2243e1b7391e1bc77bd161f41c Mon Sep 17 00:00:00 2001 From: "Anton A. Melnikov" <[email protected]> Date: Mon, 16 Sep 2024 16:12:07 +0300 Subject: [PATCH] Introduce num_done counter in the pg_stat_checkpointer view that reflects number of really performed checkpoints. --- doc/src/sgml/monitoring.sgml | 11 ++++- src/backend/access/transam/xlog.c | 6 ++- src/backend/catalog/system_views.sql | 1 + src/backend/postmaster/checkpointer.c | 5 ++- .../utils/activity/pgstat_checkpointer.c | 2 + src/backend/utils/adt/pgstatfuncs.c | 6 +++ src/include/access/xlog.h | 2 +- src/include/catalog/pg_proc.dat | 40 +++++++++++-------- src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 1 + 10 files changed, 52 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a2fda4677d7..19bf0164f1c 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3063,7 +3063,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of requested checkpoints that have been performed + Number of backend requested checkpoints + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>num_done</structfield> <type>bigint</type> + </para> + <para> + Number of checkpoints that have been performed </para></entry> </row> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 853ab06812b..ca1155567dc 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6879,7 +6879,7 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset) * both the record marking the completion of the checkpoint and the location * from which WAL replay would begin if needed. */ -void +bool CreateCheckPoint(int flags) { bool shutdown; @@ -6971,7 +6971,7 @@ CreateCheckPoint(int flags) END_CRIT_SECTION(); ereport(DEBUG1, (errmsg_internal("checkpoint skipped because system is idle"))); - return; + return false; } } @@ -7353,6 +7353,8 @@ CreateCheckPoint(int flags) CheckpointStats.ckpt_segs_added, CheckpointStats.ckpt_segs_removed, CheckpointStats.ckpt_segs_recycled); + + return true; } /* diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a18..49109dbdc86 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index eeb73c85726..ef29cb439b2 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -461,8 +461,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) */ if (!do_restartpoint) { - CreateCheckPoint(flags); - ckpt_performed = true; + ckpt_performed = CreateCheckPoint(flags); } else ckpt_performed = CreateRestartPoint(flags); @@ -495,6 +494,8 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) if (do_restartpoint) PendingCheckpointerStats.restartpoints_performed++; + else + PendingCheckpointerStats.num_performed++; } else { diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c index bbfc9c7e183..4a0a2d1493a 100644 --- a/src/backend/utils/activity/pgstat_checkpointer.c +++ b/src/backend/utils/activity/pgstat_checkpointer.c @@ -49,6 +49,7 @@ pgstat_report_checkpointer(void) #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld CHECKPOINTER_ACC(num_timed); CHECKPOINTER_ACC(num_requested); + CHECKPOINTER_ACC(num_performed); CHECKPOINTER_ACC(restartpoints_timed); CHECKPOINTER_ACC(restartpoints_requested); CHECKPOINTER_ACC(restartpoints_performed); @@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void) #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld; CHECKPOINTER_COMP(num_timed); CHECKPOINTER_COMP(num_requested); + CHECKPOINTER_COMP(num_performed); CHECKPOINTER_COMP(restartpoints_timed); CHECKPOINTER_COMP(restartpoints_requested); CHECKPOINTER_COMP(restartpoints_performed); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 9c23ac7c8c8..17b0fc02ef0 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS) PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested); } +Datum +pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed); +} + Datum pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS) { diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 083810f5b4c..36f6e4e4b4e 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -239,7 +239,7 @@ extern void LocalProcessControlFile(bool reset); extern WalLevel GetActiveWalLevelOnStandby(void); extern void StartupXLOG(void); extern void ShutdownXLOG(int code, Datum arg); -extern void CreateCheckPoint(int flags); +extern bool CreateCheckPoint(int flags); extern bool CreateRestartPoint(int flags); extern WALAvailability GetWALAvailability(XLogRecPtr targetLSN); extern void XLogPutNextOid(Oid nextOid); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 43f608d7a0a..83c96140b01 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5807,42 +5807,58 @@ proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{archived_count,last_archived_wal,last_archived_time,failed_count,last_failed_wal,last_failed_time,stats_reset}', prosrc => 'pg_stat_get_archiver' }, -{ oid => '2769', +{ oid => '6347', descr => 'statistics: number of timed checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_timed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_timed' }, -{ oid => '2770', +{ oid => '6348', descr => 'statistics: number of backend requested checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, -{ oid => '6327', +{ oid => '6349', + descr => 'statistics: number of checkpoints performed by the checkpointer', + proname => 'pg_stat_get_checkpointer_num_performed', + provolatile => 's', proparallel => 'r', prorettype => 'int8', + proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_num_performed' }, +{ oid => '6350', descr => 'statistics: number of timed restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_timed' }, -{ oid => '6328', +{ oid => '6351', descr => 'statistics: number of backend requested restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_requested' }, -{ oid => '6329', +{ oid => '6352', descr => 'statistics: number of backend performed restartpoints', proname => 'pg_stat_get_checkpointer_restartpoints_performed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_performed' }, -{ oid => '2771', +{ oid => '6353', descr => 'statistics: number of buffers written during checkpoints and restartpoints', proname => 'pg_stat_get_checkpointer_buffers_written', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_buffers_written' }, -{ oid => '6314', descr => 'statistics: last reset for the checkpointer', +{ oid => '6354', descr => 'statistics: last reset for the checkpointer', proname => 'pg_stat_get_checkpointer_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_stat_reset_time' }, +{ oid => '6355', + descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_write_time' }, +{ oid => '6356', + descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2772', descr => 'statistics: number of buffers written by the bgwriter for cleaning dirty buffers', proname => 'pg_stat_get_bgwriter_buf_written_clean', provolatile => 's', @@ -5857,16 +5873,6 @@ proname => 'pg_stat_get_bgwriter_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_bgwriter_stat_reset_time' }, -{ oid => '3160', - descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_write_time' }, -{ oid => '3161', - descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2859', descr => 'statistics: number of buffer allocations', proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' }, diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 4752dfe7197..476acd680c0 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; PgStat_Counter num_requested; + PgStat_Counter num_performed; PgStat_Counter restartpoints_timed; PgStat_Counter restartpoints_requested; PgStat_Counter restartpoints_performed; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae9..f5434d8365c 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle pg_stat_get_bgwriter_stat_reset_time() AS stats_reset; pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, -- 2.46.0 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-18 17:21 Fujii Masao <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Fujii Masao @ 2024-09-18 17:21 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Anton A. Melnikov <[email protected]>; Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/18 21:22, Alexander Korotkov wrote: >> Patch attached. >> Unless there are any objections, I plan to commit this and back-patch it to v17. > > I've checked this patch, it looks good to me. > > Generally, it looks like I should be in charge for this, given I've > committed previous patch by Anton. Thank you for reacting here faster > than me. Please, go ahead with the patch. Thanks for the review! Pushed! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-18 18:04 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Fujii Masao @ 2024-09-18 18:04 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/18 23:35, Anton A. Melnikov wrote: > Fujii, Alexander thanks a lot! > > On 17.09.2024 05:47, Fujii Masao wrote: >> >> Regarding the patch: >> if (do_restartpoint) >> PendingCheckpointerStats.restartpoints_performed++; >> + else >> + PendingCheckpointerStats.num_performed++; >> >> I expected the counter not to be incremented when a checkpoint is skipped, >> but in this code, when a checkpoint is skipped, ckpt_performed is set to true, >> triggering the counter increment. This seems wrong. > > Tried to fix it via returning bool value from the CreateCheckPoint() > similarly to the CreateRestartPoint(). > > And slightly adjusted the patch so that it could be applied after yours. Thanks for updating the patch! -void +bool CreateCheckPoint(int flags) It would be helpful to explain the new return value in the comment at the top of this function. - CreateCheckPoint(flags); - ckpt_performed = true; + ckpt_performed = CreateCheckPoint(flags); This change could result in the next scheduled checkpoint being triggered in 15 seconds if a checkpoint is skipped, which isn’t the intended behavior. -{ oid => '2769', +{ oid => '6347', I don't think that the existing functions need to be reassigned new OIDs. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-19 10:16 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-09-19 10:16 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 18.09.2024 21:04, Fujii Masao wrote: > > - CreateCheckPoint(flags); > - ckpt_performed = true; > + ckpt_performed = CreateCheckPoint(flags); > > This change could result in the next scheduled checkpoint being > triggered in 15 seconds if a checkpoint is skipped, which isn’t > the intended behavior. Thanks for pointing this out! This is really bug. Rearranged the logic a bit to save the previous behavior in the v3 attached. > -void > +bool > CreateCheckPoint(int flags) > > It would be helpful to explain the new return value in the comment > at the top of this function. Sure. Added an info about return value to the comment. > -{ oid => '2769', > +{ oid => '6347', > > I don't think that the existing functions need to be reassigned new OIDs. Ok. Left oids as is in the v3. Just added a new one for pg_stat_get_checkpointer_num_performed(). With the best regards! -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Attachments: [text/x-patch] v3-0001-Introduce-num_done-counter-in-the-pg_stat_checkpoint.patch (12.0K, ../../[email protected]/2-v3-0001-Introduce-num_done-counter-in-the-pg_stat_checkpoint.patch) download | inline diff: From 5832b1beb453b96a6ccbe72c404f2ad5373d0497 Mon Sep 17 00:00:00 2001 From: "Anton A. Melnikov" <[email protected]> Date: Thu, 19 Sep 2024 12:51:17 +0300 Subject: [PATCH] Introduce num_done counter in the pg_stat_checkpointer view that reflects number of really performed checkpoints. --- doc/src/sgml/monitoring.sgml | 11 ++++- src/backend/access/transam/xlog.c | 9 +++- src/backend/catalog/system_views.sql | 1 + src/backend/postmaster/checkpointer.c | 45 ++++++++++++------- .../utils/activity/pgstat_checkpointer.c | 2 + src/backend/utils/adt/pgstatfuncs.c | 6 +++ src/include/access/xlog.h | 2 +- src/include/catalog/pg_proc.dat | 26 ++++++----- src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 1 + 10 files changed, 74 insertions(+), 30 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a2fda4677d7..19bf0164f1c 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3063,7 +3063,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of requested checkpoints that have been performed + Number of backend requested checkpoints + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>num_done</structfield> <type>bigint</type> + </para> + <para> + Number of checkpoints that have been performed </para></entry> </row> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 853ab06812b..c9d37cba453 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6878,8 +6878,11 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset) * In this case, we only insert an XLOG_CHECKPOINT_SHUTDOWN record, and it's * both the record marking the completion of the checkpoint and the location * from which WAL replay would begin if needed. + * + * Returns true if a new checkpoint was performed or false if checkpoint + * was skipped because system is idle. */ -void +bool CreateCheckPoint(int flags) { bool shutdown; @@ -6971,7 +6974,7 @@ CreateCheckPoint(int flags) END_CRIT_SECTION(); ereport(DEBUG1, (errmsg_internal("checkpoint skipped because system is idle"))); - return; + return false; } } @@ -7353,6 +7356,8 @@ CreateCheckPoint(int flags) CheckpointStats.ckpt_segs_added, CheckpointStats.ckpt_segs_removed, CheckpointStats.ckpt_segs_recycled); + + return true; } /* diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a18..49109dbdc86 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index eeb73c85726..8d610a3f1a7 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -461,8 +461,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) */ if (!do_restartpoint) { - CreateCheckPoint(flags); - ckpt_performed = true; + ckpt_performed = CreateCheckPoint(flags); } else ckpt_performed = CreateRestartPoint(flags); @@ -484,27 +483,41 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) ConditionVariableBroadcast(&CheckpointerShmem->done_cv); - if (ckpt_performed) + if (!do_restartpoint) { /* - * Note we record the checkpoint start time not end time as - * last_checkpoint_time. This is so that time-driven - * checkpoints happen at a predictable spacing. - */ + * Note we record the checkpoint start time not end time as + * last_checkpoint_time. This is so that time-driven + * checkpoints happen at a predictable spacing. + */ last_checkpoint_time = now; - if (do_restartpoint) - PendingCheckpointerStats.restartpoints_performed++; + if(ckpt_performed) + PendingCheckpointerStats.num_performed++; + } else { - /* - * We were not able to perform the restartpoint (checkpoints - * throw an ERROR in case of error). Most likely because we - * have not received any new checkpoint WAL records since the - * last restartpoint. Try again in 15 s. - */ - last_checkpoint_time = now - CheckPointTimeout + 15; + if (ckpt_performed) + { + /* + * The same as for checkpoint. + * Please see the corresponding comment. + */ + last_checkpoint_time = now; + + PendingCheckpointerStats.restartpoints_performed++; + } + else + { + /* + * We were not able to perform the restartpoint (checkpoints + * throw an ERROR in case of error). Most likely because we + * have not received any new checkpoint WAL records since the + * last restartpoint. Try again in 15 s. + */ + last_checkpoint_time = now - CheckPointTimeout + 15; + } } ckpt_active = false; diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c index bbfc9c7e183..4a0a2d1493a 100644 --- a/src/backend/utils/activity/pgstat_checkpointer.c +++ b/src/backend/utils/activity/pgstat_checkpointer.c @@ -49,6 +49,7 @@ pgstat_report_checkpointer(void) #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld CHECKPOINTER_ACC(num_timed); CHECKPOINTER_ACC(num_requested); + CHECKPOINTER_ACC(num_performed); CHECKPOINTER_ACC(restartpoints_timed); CHECKPOINTER_ACC(restartpoints_requested); CHECKPOINTER_ACC(restartpoints_performed); @@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void) #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld; CHECKPOINTER_COMP(num_timed); CHECKPOINTER_COMP(num_requested); + CHECKPOINTER_COMP(num_performed); CHECKPOINTER_COMP(restartpoints_timed); CHECKPOINTER_COMP(restartpoints_requested); CHECKPOINTER_COMP(restartpoints_performed); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 9c23ac7c8c8..17b0fc02ef0 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS) PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested); } +Datum +pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed); +} + Datum pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS) { diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 083810f5b4c..36f6e4e4b4e 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -239,7 +239,7 @@ extern void LocalProcessControlFile(bool reset); extern WalLevel GetActiveWalLevelOnStandby(void); extern void StartupXLOG(void); extern void ShutdownXLOG(int code, Datum arg); -extern void CreateCheckPoint(int flags); +extern bool CreateCheckPoint(int flags); extern bool CreateRestartPoint(int flags); extern WALAvailability GetWALAvailability(XLogRecPtr targetLSN); extern void XLogPutNextOid(Oid nextOid); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 43f608d7a0a..baa6120e664 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5817,6 +5817,12 @@ proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, +{ oid => '2775', + descr => 'statistics: number of checkpoints performed by the checkpointer', + proname => 'pg_stat_get_checkpointer_num_performed', + provolatile => 's', proparallel => 'r', prorettype => 'int8', + proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_num_performed' }, { oid => '6327', descr => 'statistics: number of timed restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's', @@ -5843,6 +5849,16 @@ proname => 'pg_stat_get_checkpointer_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_stat_reset_time' }, + { oid => '3160', + descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_write_time' }, +{ oid => '3161', + descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', + proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', + proparallel => 'r', prorettype => 'float8', proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2772', descr => 'statistics: number of buffers written by the bgwriter for cleaning dirty buffers', proname => 'pg_stat_get_bgwriter_buf_written_clean', provolatile => 's', @@ -5857,16 +5873,6 @@ proname => 'pg_stat_get_bgwriter_stat_reset_time', provolatile => 's', proparallel => 'r', prorettype => 'timestamptz', proargtypes => '', prosrc => 'pg_stat_get_bgwriter_stat_reset_time' }, -{ oid => '3160', - descr => 'statistics: checkpoint/restartpoint time spent writing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_write_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_write_time' }, -{ oid => '3161', - descr => 'statistics: checkpoint/restartpoint time spent synchronizing buffers to disk, in milliseconds', - proname => 'pg_stat_get_checkpointer_sync_time', provolatile => 's', - proparallel => 'r', prorettype => 'float8', proargtypes => '', - prosrc => 'pg_stat_get_checkpointer_sync_time' }, { oid => '2859', descr => 'statistics: number of buffer allocations', proname => 'pg_stat_get_buf_alloc', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_buf_alloc' }, diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 4752dfe7197..476acd680c0 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; PgStat_Counter num_requested; + PgStat_Counter num_performed; PgStat_Counter restartpoints_timed; PgStat_Counter restartpoints_requested; PgStat_Counter restartpoints_performed; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae9..f5434d8365c 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle pg_stat_get_bgwriter_stat_reset_time() AS stats_reset; pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, -- 2.46.0 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-20 16:19 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Fujii Masao @ 2024-09-20 16:19 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/19 19:16, Anton A. Melnikov wrote: > > On 18.09.2024 21:04, Fujii Masao wrote: >> >> - CreateCheckPoint(flags); >> - ckpt_performed = true; >> + ckpt_performed = CreateCheckPoint(flags); >> >> This change could result in the next scheduled checkpoint being >> triggered in 15 seconds if a checkpoint is skipped, which isn’t >> the intended behavior. > > Thanks for pointing this out! This is really bug. > Rearranged the logic a bit to save the previous behavior > in the v3 attached. Thanks for updating the patch! I've attached the updated version (0001.patch). I made some cosmetic changes, including reverting the switch in the entries for pg_stat_get_checkpointer_write_time and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think that change was necessary. Could you please review the latest version? After we commit 0001.patch, how about applying 0002.patch, which updates the documentation for the pg_stat_checkpointer view to clarify what types of checkpoints and restartpoints each counter tracks? In 0002.patch, I also modified the description of num_requested from "Number of backend requested checkpoints" to remove "backend," as it can be confusing since num_requested includes requests from sources other than the backend. Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION From 859f3fecb4fb4900b6ce12f6346c5d9565fbc072 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Fri, 20 Sep 2024 11:33:07 +0900 Subject: [PATCH v4 1/2] Add num_done counter to the pg_stat_checkpointer view. Checkpoints can be skipped when the server is idle. The existing num_timed and num_requested counters in pg_stat_checkpointer track both completed and skipped checkpoints, but there was no way to count only the completed ones. This commit introduces the num_done counter, which tracks only completed checkpoints, making it easier to see how many were actually performed. Bump catalog version. Author: Anton A. Melnikov Reviewed-by: Fujii Masao Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/monitoring.sgml | 11 +++++- src/backend/access/transam/xlog.c | 9 ++++- src/backend/catalog/system_views.sql | 1 + src/backend/postmaster/checkpointer.c | 39 ++++++++++++------- .../utils/activity/pgstat_checkpointer.c | 2 + src/backend/utils/adt/pgstatfuncs.c | 6 +++ src/include/access/xlog.h | 2 +- src/include/catalog/pg_proc.dat | 6 +++ src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 1 + 10 files changed, 60 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a2fda4677d..19bf0164f1 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3063,7 +3063,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of requested checkpoints that have been performed + Number of backend requested checkpoints + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>num_done</structfield> <type>bigint</type> + </para> + <para> + Number of checkpoints that have been performed </para></entry> </row> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 853ab06812..64304d77d3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6878,8 +6878,11 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset) * In this case, we only insert an XLOG_CHECKPOINT_SHUTDOWN record, and it's * both the record marking the completion of the checkpoint and the location * from which WAL replay would begin if needed. + * + * Returns true if a new checkpoint was performed, or false if it was skipped + * because the system was idle. */ -void +bool CreateCheckPoint(int flags) { bool shutdown; @@ -6971,7 +6974,7 @@ CreateCheckPoint(int flags) END_CRIT_SECTION(); ereport(DEBUG1, (errmsg_internal("checkpoint skipped because system is idle"))); - return; + return false; } } @@ -7353,6 +7356,8 @@ CreateCheckPoint(int flags) CheckpointStats.ckpt_segs_added, CheckpointStats.ckpt_segs_removed, CheckpointStats.ckpt_segs_recycled); + + return true; } /* diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a1..49109dbdc8 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index eeb73c8572..9087e3f8db 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -460,10 +460,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) * Do the checkpoint. */ if (!do_restartpoint) - { - CreateCheckPoint(flags); - ckpt_performed = true; - } + ckpt_performed = CreateCheckPoint(flags); else ckpt_performed = CreateRestartPoint(flags); @@ -484,7 +481,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) ConditionVariableBroadcast(&CheckpointerShmem->done_cv); - if (ckpt_performed) + if (!do_restartpoint) { /* * Note we record the checkpoint start time not end time as @@ -493,18 +490,32 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) */ last_checkpoint_time = now; - if (do_restartpoint) - PendingCheckpointerStats.restartpoints_performed++; + if (ckpt_performed) + PendingCheckpointerStats.num_performed++; } else { - /* - * We were not able to perform the restartpoint (checkpoints - * throw an ERROR in case of error). Most likely because we - * have not received any new checkpoint WAL records since the - * last restartpoint. Try again in 15 s. - */ - last_checkpoint_time = now - CheckPointTimeout + 15; + if (ckpt_performed) + { + /* + * The same as for checkpoint. Please see the + * corresponding comment. + */ + last_checkpoint_time = now; + + PendingCheckpointerStats.restartpoints_performed++; + } + else + { + /* + * We were not able to perform the restartpoint + * (checkpoints throw an ERROR in case of error). Most + * likely because we have not received any new checkpoint + * WAL records since the last restartpoint. Try again in + * 15 s. + */ + last_checkpoint_time = now - CheckPointTimeout + 15; + } } ckpt_active = false; diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c index bbfc9c7e18..4a0a2d1493 100644 --- a/src/backend/utils/activity/pgstat_checkpointer.c +++ b/src/backend/utils/activity/pgstat_checkpointer.c @@ -49,6 +49,7 @@ pgstat_report_checkpointer(void) #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld CHECKPOINTER_ACC(num_timed); CHECKPOINTER_ACC(num_requested); + CHECKPOINTER_ACC(num_performed); CHECKPOINTER_ACC(restartpoints_timed); CHECKPOINTER_ACC(restartpoints_requested); CHECKPOINTER_ACC(restartpoints_performed); @@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void) #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld; CHECKPOINTER_COMP(num_timed); CHECKPOINTER_COMP(num_requested); + CHECKPOINTER_COMP(num_performed); CHECKPOINTER_COMP(restartpoints_timed); CHECKPOINTER_COMP(restartpoints_requested); CHECKPOINTER_COMP(restartpoints_performed); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 9c23ac7c8c..17b0fc02ef 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS) PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested); } +Datum +pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed); +} + Datum pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS) { diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 083810f5b4..36f6e4e4b4 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -239,7 +239,7 @@ extern void LocalProcessControlFile(bool reset); extern WalLevel GetActiveWalLevelOnStandby(void); extern void StartupXLOG(void); extern void ShutdownXLOG(int code, Datum arg); -extern void CreateCheckPoint(int flags); +extern bool CreateCheckPoint(int flags); extern bool CreateRestartPoint(int flags); extern WALAvailability GetWALAvailability(XLogRecPtr targetLSN); extern void XLogPutNextOid(Oid nextOid); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 43f608d7a0..52fca54f3a 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5817,6 +5817,12 @@ proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, +{ oid => '2775', + descr => 'statistics: number of checkpoints performed by the checkpointer', + proname => 'pg_stat_get_checkpointer_num_performed', + provolatile => 's', proparallel => 'r', prorettype => 'int8', + proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_num_performed' }, { oid => '6327', descr => 'statistics: number of timed restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 4752dfe719..476acd680c 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; PgStat_Counter num_requested; + PgStat_Counter num_performed; PgStat_Counter restartpoints_timed; PgStat_Counter restartpoints_requested; PgStat_Counter restartpoints_performed; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae..f5434d8365 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle pg_stat_get_bgwriter_stat_reset_time() AS stats_reset; pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, -- 2.45.2 From d9311aee5a0e7665eb0ea32928f728a8cd01fab5 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Sat, 21 Sep 2024 00:47:12 +0900 Subject: [PATCH v4 2/2] docs: Enhance the pg_stat_checkpointer view documentation. This commit updates the documentation for the pg_stat_checkpointer view to clarify what kind of checkpoints or restartpoints each counter tracks. This makes it easier to understand the meaning of each counter. --- doc/src/sgml/monitoring.sgml | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 19bf0164f1..3484a0490a 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3051,10 +3051,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_timed</structfield> <type>bigint</type> </para> <para> - Number of scheduled checkpoints due to timeout. - Note that checkpoints may be skipped if the server has been idle - since the last one, and this value counts both completed and - skipped checkpoints + Number of scheduled checkpoints due to timeout </para></entry> </row> @@ -3063,7 +3060,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of backend requested checkpoints + Number of requested checkpoints </para></entry> </row> @@ -3146,6 +3143,18 @@ description | Waiting for a newly initialized WAL file to reach durable storage </tgroup> </table> + <para> + Checkpoints may be skipped if the server has been idle since the last one. + <structfield>num_timed</structfield> and + <structfield>num_requested</structfield> count both completed and skipped + checkpoints, while <structfield>num_done</structfield> tracks only + the completed ones. Similarly, restartpoints may be skipped + if the last replayed checkpoint record is already the last restartpoint. + <structfield>restartpoints_timed</structfield> and + <structfield>restartpoints_req</structfield> count both completed and + skipped restartpoints, while <structfield>restartpoints_done</structfield> + tracks only the completed ones. + </para> </sect2> <sect2 id="monitoring-pg-stat-wal-view"> -- 2.45.2 Attachments: [text/plain] v4-0001-Add-num_done-counter-to-the-pg_stat_checkpointer-.patch (10.2K, ../../[email protected]/2-v4-0001-Add-num_done-counter-to-the-pg_stat_checkpointer-.patch) download | inline diff: From 859f3fecb4fb4900b6ce12f6346c5d9565fbc072 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Fri, 20 Sep 2024 11:33:07 +0900 Subject: [PATCH v4 1/2] Add num_done counter to the pg_stat_checkpointer view. Checkpoints can be skipped when the server is idle. The existing num_timed and num_requested counters in pg_stat_checkpointer track both completed and skipped checkpoints, but there was no way to count only the completed ones. This commit introduces the num_done counter, which tracks only completed checkpoints, making it easier to see how many were actually performed. Bump catalog version. Author: Anton A. Melnikov Reviewed-by: Fujii Masao Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/monitoring.sgml | 11 +++++- src/backend/access/transam/xlog.c | 9 ++++- src/backend/catalog/system_views.sql | 1 + src/backend/postmaster/checkpointer.c | 39 ++++++++++++------- .../utils/activity/pgstat_checkpointer.c | 2 + src/backend/utils/adt/pgstatfuncs.c | 6 +++ src/include/access/xlog.h | 2 +- src/include/catalog/pg_proc.dat | 6 +++ src/include/pgstat.h | 1 + src/test/regress/expected/rules.out | 1 + 10 files changed, 60 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a2fda4677d..19bf0164f1 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3063,7 +3063,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of requested checkpoints that have been performed + Number of backend requested checkpoints + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>num_done</structfield> <type>bigint</type> + </para> + <para> + Number of checkpoints that have been performed </para></entry> </row> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 853ab06812..64304d77d3 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6878,8 +6878,11 @@ update_checkpoint_display(int flags, bool restartpoint, bool reset) * In this case, we only insert an XLOG_CHECKPOINT_SHUTDOWN record, and it's * both the record marking the completion of the checkpoint and the location * from which WAL replay would begin if needed. + * + * Returns true if a new checkpoint was performed, or false if it was skipped + * because the system was idle. */ -void +bool CreateCheckPoint(int flags) { bool shutdown; @@ -6971,7 +6974,7 @@ CreateCheckPoint(int flags) END_CRIT_SECTION(); ereport(DEBUG1, (errmsg_internal("checkpoint skipped because system is idle"))); - return; + return false; } } @@ -7353,6 +7356,8 @@ CreateCheckPoint(int flags) CheckpointStats.ckpt_segs_added, CheckpointStats.ckpt_segs_removed, CheckpointStats.ckpt_segs_recycled); + + return true; } /* diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 7fd5d256a1..49109dbdc8 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1138,6 +1138,7 @@ CREATE VIEW pg_stat_checkpointer AS SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index eeb73c8572..9087e3f8db 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -460,10 +460,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) * Do the checkpoint. */ if (!do_restartpoint) - { - CreateCheckPoint(flags); - ckpt_performed = true; - } + ckpt_performed = CreateCheckPoint(flags); else ckpt_performed = CreateRestartPoint(flags); @@ -484,7 +481,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) ConditionVariableBroadcast(&CheckpointerShmem->done_cv); - if (ckpt_performed) + if (!do_restartpoint) { /* * Note we record the checkpoint start time not end time as @@ -493,18 +490,32 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) */ last_checkpoint_time = now; - if (do_restartpoint) - PendingCheckpointerStats.restartpoints_performed++; + if (ckpt_performed) + PendingCheckpointerStats.num_performed++; } else { - /* - * We were not able to perform the restartpoint (checkpoints - * throw an ERROR in case of error). Most likely because we - * have not received any new checkpoint WAL records since the - * last restartpoint. Try again in 15 s. - */ - last_checkpoint_time = now - CheckPointTimeout + 15; + if (ckpt_performed) + { + /* + * The same as for checkpoint. Please see the + * corresponding comment. + */ + last_checkpoint_time = now; + + PendingCheckpointerStats.restartpoints_performed++; + } + else + { + /* + * We were not able to perform the restartpoint + * (checkpoints throw an ERROR in case of error). Most + * likely because we have not received any new checkpoint + * WAL records since the last restartpoint. Try again in + * 15 s. + */ + last_checkpoint_time = now - CheckPointTimeout + 15; + } } ckpt_active = false; diff --git a/src/backend/utils/activity/pgstat_checkpointer.c b/src/backend/utils/activity/pgstat_checkpointer.c index bbfc9c7e18..4a0a2d1493 100644 --- a/src/backend/utils/activity/pgstat_checkpointer.c +++ b/src/backend/utils/activity/pgstat_checkpointer.c @@ -49,6 +49,7 @@ pgstat_report_checkpointer(void) #define CHECKPOINTER_ACC(fld) stats_shmem->stats.fld += PendingCheckpointerStats.fld CHECKPOINTER_ACC(num_timed); CHECKPOINTER_ACC(num_requested); + CHECKPOINTER_ACC(num_performed); CHECKPOINTER_ACC(restartpoints_timed); CHECKPOINTER_ACC(restartpoints_requested); CHECKPOINTER_ACC(restartpoints_performed); @@ -127,6 +128,7 @@ pgstat_checkpointer_snapshot_cb(void) #define CHECKPOINTER_COMP(fld) pgStatLocal.snapshot.checkpointer.fld -= reset.fld; CHECKPOINTER_COMP(num_timed); CHECKPOINTER_COMP(num_requested); + CHECKPOINTER_COMP(num_performed); CHECKPOINTER_COMP(restartpoints_timed); CHECKPOINTER_COMP(restartpoints_requested); CHECKPOINTER_COMP(restartpoints_performed); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 9c23ac7c8c..17b0fc02ef 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1191,6 +1191,12 @@ pg_stat_get_checkpointer_num_requested(PG_FUNCTION_ARGS) PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_requested); } +Datum +pg_stat_get_checkpointer_num_performed(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_fetch_stat_checkpointer()->num_performed); +} + Datum pg_stat_get_checkpointer_restartpoints_timed(PG_FUNCTION_ARGS) { diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 083810f5b4..36f6e4e4b4 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -239,7 +239,7 @@ extern void LocalProcessControlFile(bool reset); extern WalLevel GetActiveWalLevelOnStandby(void); extern void StartupXLOG(void); extern void ShutdownXLOG(int code, Datum arg); -extern void CreateCheckPoint(int flags); +extern bool CreateCheckPoint(int flags); extern bool CreateRestartPoint(int flags); extern WALAvailability GetWALAvailability(XLogRecPtr targetLSN); extern void XLogPutNextOid(Oid nextOid); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 43f608d7a0..52fca54f3a 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5817,6 +5817,12 @@ proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, +{ oid => '2775', + descr => 'statistics: number of checkpoints performed by the checkpointer', + proname => 'pg_stat_get_checkpointer_num_performed', + provolatile => 's', proparallel => 'r', prorettype => 'int8', + proargtypes => '', + prosrc => 'pg_stat_get_checkpointer_num_performed' }, { oid => '6327', descr => 'statistics: number of timed restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_timed', provolatile => 's', diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 4752dfe719..476acd680c 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -294,6 +294,7 @@ typedef struct PgStat_CheckpointerStats { PgStat_Counter num_timed; PgStat_Counter num_requested; + PgStat_Counter num_performed; PgStat_Counter restartpoints_timed; PgStat_Counter restartpoints_requested; PgStat_Counter restartpoints_performed; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index a1626f3fae..f5434d8365 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1824,6 +1824,7 @@ pg_stat_bgwriter| SELECT pg_stat_get_bgwriter_buf_written_clean() AS buffers_cle pg_stat_get_bgwriter_stat_reset_time() AS stats_reset; pg_stat_checkpointer| SELECT pg_stat_get_checkpointer_num_timed() AS num_timed, pg_stat_get_checkpointer_num_requested() AS num_requested, + pg_stat_get_checkpointer_num_performed() AS num_done, pg_stat_get_checkpointer_restartpoints_timed() AS restartpoints_timed, pg_stat_get_checkpointer_restartpoints_requested() AS restartpoints_req, pg_stat_get_checkpointer_restartpoints_performed() AS restartpoints_done, -- 2.45.2 [text/plain] v4-0002-docs-Enhance-the-pg_stat_checkpointer-view-docume.patch (2.4K, ../../[email protected]/3-v4-0002-docs-Enhance-the-pg_stat_checkpointer-view-docume.patch) download | inline diff: From d9311aee5a0e7665eb0ea32928f728a8cd01fab5 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Sat, 21 Sep 2024 00:47:12 +0900 Subject: [PATCH v4 2/2] docs: Enhance the pg_stat_checkpointer view documentation. This commit updates the documentation for the pg_stat_checkpointer view to clarify what kind of checkpoints or restartpoints each counter tracks. This makes it easier to understand the meaning of each counter. --- doc/src/sgml/monitoring.sgml | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 19bf0164f1..3484a0490a 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3051,10 +3051,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_timed</structfield> <type>bigint</type> </para> <para> - Number of scheduled checkpoints due to timeout. - Note that checkpoints may be skipped if the server has been idle - since the last one, and this value counts both completed and - skipped checkpoints + Number of scheduled checkpoints due to timeout </para></entry> </row> @@ -3063,7 +3060,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage <structfield>num_requested</structfield> <type>bigint</type> </para> <para> - Number of backend requested checkpoints + Number of requested checkpoints </para></entry> </row> @@ -3146,6 +3143,18 @@ description | Waiting for a newly initialized WAL file to reach durable storage </tgroup> </table> + <para> + Checkpoints may be skipped if the server has been idle since the last one. + <structfield>num_timed</structfield> and + <structfield>num_requested</structfield> count both completed and skipped + checkpoints, while <structfield>num_done</structfield> tracks only + the completed ones. Similarly, restartpoints may be skipped + if the last replayed checkpoint record is already the last restartpoint. + <structfield>restartpoints_timed</structfield> and + <structfield>restartpoints_req</structfield> count both completed and + skipped restartpoints, while <structfield>restartpoints_done</structfield> + tracks only the completed ones. + </para> </sect2> <sect2 id="monitoring-pg-stat-wal-view"> -- 2.45.2 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-22 04:55 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-09-22 04:55 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 20.09.2024 19:19, Fujii Masao wrote: > I've attached the updated version (0001.patch). I made some cosmetic changes, > including reverting the switch in the entries for pg_stat_get_checkpointer_write_time > and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think > that change was necessary. Could you please review the latest version? Thanks for corrections! All looks good for me. As for switching in the pg_proc.dat entries the idea was to put them in order so that the pg_stat_get_checkpointer* functions were grouped together. I don't know if this is the common and accepted practice. Simply i like it better this way. Sure, if you think it's unnecessary, let it stay as is with minimal diff. > After we commit 0001.patch, how about applying 0002.patch, which updates > the documentation for the pg_stat_checkpointer view to clarify what types > of checkpoints and restartpoints each counter tracks? I liked that the short definitions of the counters are now separated from the description of its work features which are combined into one paragraph. It seems to me that is much more logical and easier to understand. In addition, checkpoints may be skipped due to "checkpoints are occurring too frequently" error. Not sure, but maybe add this information to the new description? > In 0002.patch, I also modified the description of num_requested from > "Number of backend requested checkpoints" to remove "backend," as it can > be confusing since num_requested includes requests from sources other than > the backend. Thought? Agreed. E.g. from xlog. Then maybe changed it also in the function descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested() and pg_stat_get_checkpointer_restartpoints_requested(). Also checked v4 with the travis patch-tester. All is ok. With the best wishes! -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-30 03:26 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 2 replies; 25+ messages in thread From: Fujii Masao @ 2024-09-30 03:26 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/22 13:55, Anton A. Melnikov wrote: > On 20.09.2024 19:19, Fujii Masao wrote: >> I've attached the updated version (0001.patch). I made some cosmetic changes, >> including reverting the switch in the entries for pg_stat_get_checkpointer_write_time >> and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think >> that change was necessary. Could you please review the latest version? > > Thanks for corrections! > All looks good for me. Thanks for the review! I've pushed the 0001 patch. > As for switching in the pg_proc.dat entries the idea was to put them in order > so that the pg_stat_get_checkpointer* functions were grouped together. > I don't know if this is the common and accepted practice. Simply i like it better this way. > Sure, if you think it's unnecessary, let it stay as is with minimal diff. I understand your point, but I didn't made that change to keep the diff minimal, which should make future back-patching easier. >> After we commit 0001.patch, how about applying 0002.patch, which updates >> the documentation for the pg_stat_checkpointer view to clarify what types >> of checkpoints and restartpoints each counter tracks? > > I liked that the short definitions of the counters are now separated from > the description of its work features which are combined into one paragraph. > It seems to me that is much more logical and easier to understand. Thanks for the review! > In addition, checkpoints may be skipped due to "checkpoints are occurring > too frequently" error. Not sure, but maybe add this information to > the new description? From what I can see in the code, that error message doesn’t seem to indicate the checkpoint is being skipped. In fact, checkpoints are still happening actually when that message appears. Am I misunderstanding something? >> In 0002.patch, I also modified the description of num_requested from >> "Number of backend requested checkpoints" to remove "backend," as it can >> be confusing since num_requested includes requests from sources other than >> the backend. Thought? > > Agreed. E.g. from xlog. Then maybe changed it also in the function > descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested() > and pg_stat_get_checkpointer_restartpoints_requested(). Yes, good catch! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-30 07:00 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-09-30 07:00 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 30.09.2024 06:26, Fujii Masao wrote: > Thanks for the review! I've pushed the 0001 patch. Thanks a lot! >> As for switching in the pg_proc.dat entries the idea was to put them in order >> so that the pg_stat_get_checkpointer* functions were grouped together. >> I don't know if this is the common and accepted practice. Simply i like it better this way. >> Sure, if you think it's unnecessary, let it stay as is with minimal diff. > > I understand your point, but I didn't made that change to keep the diff minimal, > which should make future back-patching easier. Agreed. Its quite reasonable. I've not take into account the backporting possibility at all. This is of course wrong. >> In addition, checkpoints may be skipped due to "checkpoints are occurring >> too frequently" error. Not sure, but maybe add this information to >> the new description? > > From what I can see in the code, that error message doesn’t seem to indicate > the checkpoint is being skipped. In fact, checkpoints are still happening > actually when that message appears. Am I misunderstanding something? No, you are right! This is my oversight. I didn't notice that elevel is just a log not a error. Thanks! With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-09-30 17:09 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Fujii Masao @ 2024-09-30 17:09 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/30 16:00, Anton A. Melnikov wrote: > > On 30.09.2024 06:26, Fujii Masao wrote: >> Thanks for the review! I've pushed the 0001 patch. > > Thanks a lot! > >>> As for switching in the pg_proc.dat entries the idea was to put them in order >>> so that the pg_stat_get_checkpointer* functions were grouped together. >>> I don't know if this is the common and accepted practice. Simply i like it better this way. >>> Sure, if you think it's unnecessary, let it stay as is with minimal diff. >> >> I understand your point, but I didn't made that change to keep the diff minimal, >> which should make future back-patching easier. > > Agreed. Its quite reasonable. I've not take into account the backporting > possibility at all. This is of course wrong. > >>> In addition, checkpoints may be skipped due to "checkpoints are occurring >>> too frequently" error. Not sure, but maybe add this information to >>> the new description? >> >> From what I can see in the code, that error message doesn’t seem to indicate >> the checkpoint is being skipped. In fact, checkpoints are still happening >> actually when that message appears. Am I misunderstanding something? > > No, you are right! This is my oversight. I didn't notice that elevel is just a log > not a error. Thanks! Ok, so I pushed 0002.patch. Thanks for the review! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-10-08 12:42 Fujii Masao <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 25+ messages in thread From: Fujii Masao @ 2024-10-08 12:42 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/09/30 12:26, Fujii Masao wrote: >>> In 0002.patch, I also modified the description of num_requested from >>> "Number of backend requested checkpoints" to remove "backend," as it can >>> be confusing since num_requested includes requests from sources other than >>> the backend. Thought? >> >> Agreed. E.g. from xlog. Then maybe changed it also in the function >> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested() >> and pg_stat_get_checkpointer_restartpoints_requested(). > > Yes, good catch! Patch attached. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION From 5c935f5263fc4d516ceaf8d46c2a06daf035f1f7 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 8 Oct 2024 21:12:48 +0900 Subject: [PATCH v1] Improve descriptions of some pg_stat_checkpoints functions in pg_proc.dat. Previously, the descriptions of pg_stat_get_checkpointer_num_requested(), pg_stat_get_checkpointer_restartpoints_requested(), and pg_stat_get_checkpointer_restartpoints_performed() in pg_proc.dat referred to "backend". This was misleading because these functions report the number of checkpoints or restartpoints requested or performed by other than backends as well. This commit removes "backend" from these descriptions to avoid confusion. --- src/include/catalog/pg_proc.dat | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 77f54a79e6..2ba144a7aa 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5816,7 +5816,7 @@ proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_timed' }, { oid => '2770', - descr => 'statistics: number of backend requested checkpoints started by the checkpointer', + descr => 'statistics: number of requested checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, @@ -5831,13 +5831,13 @@ proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_timed' }, { oid => '6328', - descr => 'statistics: number of backend requested restartpoints started by the checkpointer', + descr => 'statistics: number of requested restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_requested' }, { oid => '6329', - descr => 'statistics: number of backend performed restartpoints', + descr => 'statistics: number of restartpoints performed by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_performed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', -- 2.46.2 Attachments: [text/plain] v1-0001-Improve-descriptions-of-some-pg_stat_checkpoints-.patch (2.4K, ../../[email protected]/2-v1-0001-Improve-descriptions-of-some-pg_stat_checkpoints-.patch) download | inline diff: From 5c935f5263fc4d516ceaf8d46c2a06daf035f1f7 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 8 Oct 2024 21:12:48 +0900 Subject: [PATCH v1] Improve descriptions of some pg_stat_checkpoints functions in pg_proc.dat. Previously, the descriptions of pg_stat_get_checkpointer_num_requested(), pg_stat_get_checkpointer_restartpoints_requested(), and pg_stat_get_checkpointer_restartpoints_performed() in pg_proc.dat referred to "backend". This was misleading because these functions report the number of checkpoints or restartpoints requested or performed by other than backends as well. This commit removes "backend" from these descriptions to avoid confusion. --- src/include/catalog/pg_proc.dat | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 77f54a79e6..2ba144a7aa 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -5816,7 +5816,7 @@ proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_timed' }, { oid => '2770', - descr => 'statistics: number of backend requested checkpoints started by the checkpointer', + descr => 'statistics: number of requested checkpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_num_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_num_requested' }, @@ -5831,13 +5831,13 @@ proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_timed' }, { oid => '6328', - descr => 'statistics: number of backend requested restartpoints started by the checkpointer', + descr => 'statistics: number of requested restartpoints started by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_requested', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', prosrc => 'pg_stat_get_checkpointer_restartpoints_requested' }, { oid => '6329', - descr => 'statistics: number of backend performed restartpoints', + descr => 'statistics: number of restartpoints performed by the checkpointer', proname => 'pg_stat_get_checkpointer_restartpoints_performed', provolatile => 's', proparallel => 'r', prorettype => 'int8', proargtypes => '', -- 2.46.2 ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-10-08 14:16 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Anton A. Melnikov @ 2024-10-08 14:16 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 08.10.2024 15:42, Fujii Masao wrote: > On 2024/09/30 12:26, Fujii Masao wrote: >>>> In 0002.patch, I also modified the description of num_requested from >>>> "Number of backend requested checkpoints" to remove "backend," as it can >>>> be confusing since num_requested includes requests from sources other than >>>> the backend. Thought? >>> >>> Agreed. E.g. from xlog. Then maybe changed it also in the function >>> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested() >>> and pg_stat_get_checkpointer_restartpoints_requested(). >> >> Yes, good catch! > > Patch attached. Looked at the patch. Just in case, checked that neither “backend completed” nor “backend requested” were found anywhere else. All is ok for me. Thanks a lot! With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-10-10 15:14 Fujii Masao <[email protected]> parent: Anton A. Melnikov <[email protected]> 0 siblings, 1 reply; 25+ messages in thread From: Fujii Masao @ 2024-10-10 15:14 UTC (permalink / raw) To: Anton A. Melnikov <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 2024/10/08 23:16, Anton A. Melnikov wrote: > > On 08.10.2024 15:42, Fujii Masao wrote: >> On 2024/09/30 12:26, Fujii Masao wrote: >>>>> In 0002.patch, I also modified the description of num_requested from >>>>> "Number of backend requested checkpoints" to remove "backend," as it can >>>>> be confusing since num_requested includes requests from sources other than >>>>> the backend. Thought? >>>> >>>> Agreed. E.g. from xlog. Then maybe changed it also in the function >>>> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested() >>>> and pg_stat_get_checkpointer_restartpoints_requested(). >>> >>> Yes, good catch! >> >> Patch attached. > > Looked at the patch. Just in case, checked that neither > “backend completed” nor “backend requested” were found anywhere else. > All is ok for me. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. @ 2024-10-10 15:37 Anton A. Melnikov <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 0 replies; 25+ messages in thread From: Anton A. Melnikov @ 2024-10-10 15:37 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: Magnus Hagander <[email protected]>; Anton A. Melnikov <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected] On 10.10.2024 18:14, Fujii Masao wrote: > Thanks for the review! Pushed. Thanks a lot! With the best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company ^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2024-10-10 15:37 UTC | newest] Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-09-10 08:28 [PATCH v11 3/4] Remove globals readOff, readLen and readSegNo Kyotaro Horiguchi <[email protected]> 2023-06-30 19:46 [PATCH v1 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v4 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v3 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v2 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v1 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v2 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2023-06-30 19:46 [PATCH v3 1/1] remove db_user_namespace Nathan Bossart <[email protected]> 2024-09-16 14:30 Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-09-17 02:47 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-18 10:21 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-18 12:22 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Alexander Korotkov <[email protected]> 2024-09-18 17:21 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-18 14:35 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-09-18 18:04 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-19 10:16 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-09-20 16:19 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-22 04:55 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-09-30 03:26 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-09-30 07:00 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-09-30 17:09 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-10-08 12:42 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-10-08 14:16 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[email protected]> 2024-10-10 15:14 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Fujii Masao <[email protected]> 2024-10-10 15:37 ` Re: May be BUG. Periodic burst growth of the checkpoint_req counter on replica. Anton A. Melnikov <[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