On Thu, Dec 7, 2023 at 8:10 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Wed, Dec 06, 2023 at 11:32:19PM -0300, Euler Taveira wrote:
> > IIUC trace_recovery_messages was a debugging aid in the 9.0 era when the HS
> > was
> > introduced. I'm also wondering if anyone used it in the past years.
>
> FWIW, I'd be +1 for getting rid of entirely, with
+1 for removing trace_recovery_messages. Firstly, it doesn't cover all
the recovery related messages as it promises, so it's an incomplete
feature. Secondly, it needs a bit of understanding as to how it gels
with client_min_messages and log_min_messages.
> its conditional
> block in PerformWalRecovery(), as it does not bring any additional
> value now that it is possible to achieve much more with pg_waldump
> (pg_xlogdump before that) introduced a couple of years later in 9.3.
And, I agree that the functionality (description of the WAL record
being applied) of conditional trace_recovery_messages code under
WAL_DEBUG macro in PerformWalRecovery's main redo apply loop can more
easily be achieved with either pg_walinspect or pg_waldump. That's my
point as well for getting rid of WAL_DEBUG macro related code after
converting a few messages to DEBUGX level.
The comment atop trace_recovery [1] function says it should go away
eventually and seems to have served the purpose when the recovery
related code was introduced in PG 9.0.
FWIW, the attached patch is what I've left with after removing
trace_recovery_messages related code, 9 files changed, 19
insertions(+), 97 deletions(-).
[1]
* Intention is to keep this for at least the whole of the 9.0 production
* release, so we can more easily diagnose production problems in the field.
* It should go away eventually, though, because it's an ugly and
* hard-to-explain kluge.
*/
int
trace_recovery(int trace_level)
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 8b804df3959e5e4e2fdc4f147b8309bece92fba4 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 7 Dec 2023 11:49:53 +0000
Subject: [PATCH v1] Remove trace_recovery_messages related code
---
doc/src/sgml/config.sgml | 26 -----------------------
src/backend/access/transam/xlog.c | 2 +-
src/backend/access/transam/xlogrecovery.c | 4 +---
src/backend/storage/ipc/procarray.c | 20 ++++++++---------
src/backend/storage/ipc/standby.c | 11 +++++-----
src/backend/utils/cache/inval.c | 5 ++---
src/backend/utils/error/elog.c | 26 -----------------------
src/backend/utils/misc/guc_tables.c | 18 ----------------
src/include/miscadmin.h | 4 ----
9 files changed, 19 insertions(+), 97 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 94d1eb2b81..44cada2b40 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11216,32 +11216,6 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</listitem>
</varlistentry>
- <varlistentry id="guc-trace-recovery-messages" xreflabel="trace_recovery_messages">
- <term><varname>trace_recovery_messages</varname> (<type>enum</type>)
- <indexterm>
- <primary><varname>trace_recovery_messages</varname> configuration parameter</primary>
- </indexterm>
- </term>
- <listitem>
- <para>
- Enables logging of recovery-related debugging output that otherwise
- would not be logged. This parameter allows the user to override the
- normal setting of <xref linkend="guc-log-min-messages"/>, but only for
- specific messages. This is intended for use in debugging hot standby.
- Valid values are <literal>DEBUG5</literal>, <literal>DEBUG4</literal>,
- <literal>DEBUG3</literal>, <literal>DEBUG2</literal>, <literal>DEBUG1</literal>, and
- <literal>LOG</literal>. The default, <literal>LOG</literal>, does not affect
- logging decisions at all. The other values cause recovery-related
- debug messages of that priority or higher to be logged as though they
- had <literal>LOG</literal> priority; for common settings of
- <varname>log_min_messages</varname> this results in unconditionally sending
- them to the server log.
- This parameter can only be set in the <filename>postgresql.conf</filename>
- file or on the server command line.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="guc-trace-sort" xreflabel="trace_sort">
<term><varname>trace_sort</varname> (<type>boolean</type>)
<indexterm>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2d603d8dee..14342a16bf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7213,7 +7213,7 @@ RecoveryRestartPoint(const CheckPoint *checkPoint, XLogReaderState *record)
*/
if (XLogHaveInvalidPages())
{
- elog(trace_recovery(DEBUG2),
+ elog(DEBUG2,
"could not record restart point at %X/%X because there "
"are unresolved references to invalid pages",
LSN_FORMAT_ARGS(checkPoint->redo));
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c61566666a..0aed9f5ccb 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1704,9 +1704,7 @@ PerformWalRecovery(void)
LSN_FORMAT_ARGS(xlogreader->ReadRecPtr));
#ifdef WAL_DEBUG
- if (XLOG_DEBUG ||
- (record->xl_rmid == RM_XACT_ID && trace_recovery_messages <= DEBUG2) ||
- (record->xl_rmid != RM_XACT_ID && trace_recovery_messages <= DEBUG3))
+ if (XLOG_DEBUG)
{
StringInfoData buf;
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 80ab026bf5..084d3e1db2 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1109,11 +1109,11 @@ ProcArrayApplyRecoveryInfo(RunningTransactions running)
running->oldestRunningXid))
{
standbyState = STANDBY_SNAPSHOT_READY;
- elog(trace_recovery(DEBUG1),
+ elog(DEBUG1,
"recovery snapshots are now enabled");
}
else
- elog(trace_recovery(DEBUG1),
+ elog(DEBUG1,
"recovery snapshot waiting for non-overflowed snapshot or "
"until oldest active xid on standby is at least %u (now %u)",
standbySnapshotPendingXmin,
@@ -1208,7 +1208,7 @@ ProcArrayApplyRecoveryInfo(RunningTransactions running)
KnownAssignedXidsAdd(xids[i], xids[i], true);
}
- KnownAssignedXidsDisplay(trace_recovery(DEBUG3));
+ KnownAssignedXidsDisplay(DEBUG3);
}
pfree(xids);
@@ -1280,11 +1280,11 @@ ProcArrayApplyRecoveryInfo(RunningTransactions running)
Assert(FullTransactionIdIsValid(ShmemVariableCache->nextXid));
- KnownAssignedXidsDisplay(trace_recovery(DEBUG3));
+ KnownAssignedXidsDisplay(DEBUG3);
if (standbyState == STANDBY_SNAPSHOT_READY)
- elog(trace_recovery(DEBUG1), "recovery snapshots are now enabled");
+ elog(DEBUG1, "recovery snapshots are now enabled");
else
- elog(trace_recovery(DEBUG1),
+ elog(DEBUG1,
"recovery snapshot waiting for non-overflowed snapshot or "
"until oldest active xid on standby is at least %u (now %u)",
standbySnapshotPendingXmin,
@@ -4339,7 +4339,7 @@ RecordKnownAssignedTransactionIds(TransactionId xid)
Assert(TransactionIdIsValid(xid));
Assert(TransactionIdIsValid(latestObservedXid));
- elog(trace_recovery(DEBUG4), "record known xact %u latestObservedXid %u",
+ elog(DEBUG4, "record known xact %u latestObservedXid %u",
xid, latestObservedXid);
/*
@@ -4897,7 +4897,7 @@ KnownAssignedXidsRemove(TransactionId xid)
{
Assert(TransactionIdIsValid(xid));
- elog(trace_recovery(DEBUG4), "remove KnownAssignedXid %u", xid);
+ elog(DEBUG4, "remove KnownAssignedXid %u", xid);
/*
* Note: we cannot consider it an error to remove an XID that's not
@@ -4951,13 +4951,13 @@ KnownAssignedXidsRemovePreceding(TransactionId removeXid)
if (!TransactionIdIsValid(removeXid))
{
- elog(trace_recovery(DEBUG4), "removing all KnownAssignedXids");
+ elog(DEBUG4, "removing all KnownAssignedXids");
pArray->numKnownAssignedXids = 0;
pArray->headKnownAssignedXids = pArray->tailKnownAssignedXids = 0;
return;
}
- elog(trace_recovery(DEBUG4), "prune KnownAssignedXids to %u", removeXid);
+ elog(DEBUG4, "prune KnownAssignedXids to %u", removeXid);
/*
* Mark entries invalid starting at the tail. Since array is sorted, we
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index cc22d2e87c..cea47bf6e5 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -997,8 +997,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
TransactionIdDidAbort(xid))
return;
- elog(trace_recovery(DEBUG4),
- "adding recovery lock: db %u rel %u", dbOid, relOid);
+ elog(DEBUG4, "adding recovery lock: db %u rel %u", dbOid, relOid);
/* dbOid is InvalidOid when we are locking a shared relation. */
Assert(OidIsValid(relOid));
@@ -1042,7 +1041,7 @@ StandbyReleaseXidEntryLocks(RecoveryLockXidEntry *xidentry)
{
LOCKTAG locktag;
- elog(trace_recovery(DEBUG4),
+ elog(DEBUG4,
"releasing recovery lock: xid %u db %u rel %u",
entry->key.xid, entry->key.dbOid, entry->key.relOid);
/* Release the lock ... */
@@ -1109,7 +1108,7 @@ StandbyReleaseAllLocks(void)
HASH_SEQ_STATUS status;
RecoveryLockXidEntry *entry;
- elog(trace_recovery(DEBUG2), "release all standby locks");
+ elog(DEBUG2, "release all standby locks");
hash_seq_init(&status, RecoveryLockXidHash);
while ((entry = hash_seq_search(&status)))
@@ -1369,7 +1368,7 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts)
recptr = XLogInsert(RM_STANDBY_ID, XLOG_RUNNING_XACTS);
if (CurrRunningXacts->subxid_overflow)
- elog(trace_recovery(DEBUG2),
+ elog(DEBUG2,
"snapshot of %d running transactions overflowed (lsn %X/%X oldest xid %u latest complete %u next xid %u)",
CurrRunningXacts->xcnt,
LSN_FORMAT_ARGS(recptr),
@@ -1377,7 +1376,7 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts)
CurrRunningXacts->latestCompletedXid,
CurrRunningXacts->nextXid);
else
- elog(trace_recovery(DEBUG2),
+ elog(DEBUG2,
"snapshot of %d+%d running transaction ids (lsn %X/%X oldest xid %u latest complete %u next xid %u)",
CurrRunningXacts->xcnt, CurrRunningXacts->subxcnt,
LSN_FORMAT_ARGS(recptr),
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index a041d7d604..30d872dfde 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -966,13 +966,12 @@ ProcessCommittedInvalidationMessages(SharedInvalidationMessage *msgs,
if (nmsgs <= 0)
return;
- elog(trace_recovery(DEBUG4), "replaying commit with %d messages%s", nmsgs,
+ elog(DEBUG4, "replaying commit with %d messages%s", nmsgs,
(RelcacheInitFileInval ? " and relcache file invalidation" : ""));
if (RelcacheInitFileInval)
{
- elog(trace_recovery(DEBUG4), "removing relcache init files for database %u",
- dbid);
+ elog(DEBUG4, "removing relcache init files for database %u", dbid);
/*
* RelationCacheInitFilePreInvalidate, when the invalidation message
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 6aeb855e49..6ea575a53b 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3759,29 +3759,3 @@ write_stderr_signal_safe(const char *str)
nwritten += rc;
}
}
-
-
-/*
- * Adjust the level of a recovery-related message per trace_recovery_messages.
- *
- * The argument is the default log level of the message, eg, DEBUG2. (This
- * should only be applied to DEBUGn log messages, otherwise it's a no-op.)
- * If the level is >= trace_recovery_messages, we return LOG, causing the
- * message to be logged unconditionally (for most settings of
- * log_min_messages). Otherwise, we return the argument unchanged.
- * The message will then be shown based on the setting of log_min_messages.
- *
- * Intention is to keep this for at least the whole of the 9.0 production
- * release, so we can more easily diagnose production problems in the field.
- * It should go away eventually, though, because it's an ugly and
- * hard-to-explain kluge.
- */
-int
-trace_recovery(int trace_level)
-{
- if (trace_level < LOG &&
- trace_level >= trace_recovery_messages)
- return LOG;
-
- return trace_level;
-}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 6474e35ec0..f7c9882f7c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -525,7 +525,6 @@ int log_parameter_max_length_on_error = 0;
int log_temp_files = -1;
double log_statement_sample_rate = 1.0;
double log_xact_sample_rate = 0;
-int trace_recovery_messages = LOG;
char *backtrace_functions;
int temp_file_limit = -1;
@@ -4780,23 +4779,6 @@ struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
- {
- {"trace_recovery_messages", PGC_SIGHUP, DEVELOPER_OPTIONS,
- gettext_noop("Enables logging of recovery-related debugging information."),
- gettext_noop("Each level includes all the levels that follow it. The later"
- " the level, the fewer messages are sent."),
- GUC_NOT_IN_SAMPLE,
- },
- &trace_recovery_messages,
-
- /*
- * client_message_level_options allows too many values, really, but
- * it's not worth having a separate options array for this.
- */
- LOG, client_message_level_options,
- NULL, NULL, NULL
- },
-
{
{"track_functions", PGC_SUSET, STATS_CUMULATIVE,
gettext_noop("Collects function-level statistics on database activity."),
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index f0cc651435..1043a4d782 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -302,10 +302,6 @@ extern void PreventCommandIfReadOnly(const char *cmdname);
extern void PreventCommandIfParallelMode(const char *cmdname);
extern void PreventCommandDuringRecovery(const char *cmdname);
-/* in utils/misc/guc_tables.c */
-extern PGDLLIMPORT int trace_recovery_messages;
-extern int trace_recovery(int trace_level);
-
/*****************************************************************************
* pdir.h -- *
* POSTGRES directory path definitions. *
--
2.34.1
