Hi,
On Fri, 23 Feb 2024 at 15:34, Daniel Gustafsson <dan...@yesql.se> wrote:
>
> > On 23 Feb 2024, at 13:09, Nazir Bilal Yavuz <byavu...@gmail.com> wrote:
>
> > Does errmsg_internal() need to be used all the time when turning elogs
> > into ereports? errmsg_internal()'s comment says that "This should be
> > used for "can't happen" cases that are probably not worth spending
> > translation effort on.". Is it enough to check if the error message
> > has a translation, and then decide the use of errmsg_internal() or
> > errmsg()?
>
> If it's an elog then it won't have a translation as none are included in the
> translation set. If the errmsg is generic enough to be translated anyways via
> another (un)related ereport call then we of course use that, but ideally not
> create new ones.
Thanks for the explanation.
All of your feedback is addressed in v2.
--
Regards,
Nazir Bilal Yavuz
Microsoft
From fa45a69731da1488560b2749023e9b9573231c2b Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Fri, 23 Feb 2024 16:49:10 +0300
Subject: [PATCH v2 1/2] (xlog.c) Add missing error codes to PANIC/FATAL error
reports
---
src/backend/access/transam/xlog.c | 77 +++++++++++++++++++++----------
1 file changed, 52 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c1162d55bff..d295cef9606 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1354,7 +1354,9 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
}
if (CurrPos != EndPos)
- elog(PANIC, "space reserved for WAL record does not match what was written");
+ ereport(PANIC,
+ errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("space reserved for WAL record does not match what was written"));
}
/*
@@ -4040,7 +4042,8 @@ ValidateXLOGDirectoryStructure(void)
if (stat(XLOGDIR, &stat_buf) != 0 ||
!S_ISDIR(stat_buf.st_mode))
ereport(FATAL,
- (errmsg("required WAL directory \"%s\" does not exist",
+ (errcode_for_file_access(),
+ errmsg("required WAL directory \"%s\" does not exist",
XLOGDIR)));
/* Check for archive_status */
@@ -4050,7 +4053,8 @@ ValidateXLOGDirectoryStructure(void)
/* Check for weird cases where it exists but isn't a directory */
if (!S_ISDIR(stat_buf.st_mode))
ereport(FATAL,
- (errmsg("required WAL directory \"%s\" does not exist",
+ (errcode_for_file_access(),
+ errmsg("required WAL directory \"%s\" does not exist",
path)));
}
else
@@ -4059,7 +4063,8 @@ ValidateXLOGDirectoryStructure(void)
(errmsg("creating missing WAL directory \"%s\"", path)));
if (MakePGDirectory(path) < 0)
ereport(FATAL,
- (errmsg("could not create missing directory \"%s\": %m",
+ (errcode_for_file_access(),
+ errmsg("could not create missing directory \"%s\": %m",
path)));
}
@@ -4296,7 +4301,8 @@ ReadControlFile(void)
if (ControlFile->pg_control_version != PG_CONTROL_VERSION && ControlFile->pg_control_version % 65536 == 0 && ControlFile->pg_control_version / 65536 != 0)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with PG_CONTROL_VERSION %d (0x%08x),"
" but the server was compiled with PG_CONTROL_VERSION %d (0x%08x).",
ControlFile->pg_control_version, ControlFile->pg_control_version,
@@ -4305,7 +4311,8 @@ ReadControlFile(void)
if (ControlFile->pg_control_version != PG_CONTROL_VERSION)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with PG_CONTROL_VERSION %d,"
" but the server was compiled with PG_CONTROL_VERSION %d.",
ControlFile->pg_control_version, PG_CONTROL_VERSION),
@@ -4320,7 +4327,8 @@ ReadControlFile(void)
if (!EQ_CRC32C(crc, ControlFile->crc))
ereport(FATAL,
- (errmsg("incorrect checksum in control file")));
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("incorrect checksum in control file")));
/*
* Do compatibility checking immediately. If the database isn't
@@ -4329,68 +4337,78 @@ ReadControlFile(void)
*/
if (ControlFile->catalog_version_no != CATALOG_VERSION_NO)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with CATALOG_VERSION_NO %d,"
" but the server was compiled with CATALOG_VERSION_NO %d.",
ControlFile->catalog_version_no, CATALOG_VERSION_NO),
errhint("It looks like you need to initdb.")));
if (ControlFile->maxAlign != MAXIMUM_ALIGNOF)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with MAXALIGN %d,"
" but the server was compiled with MAXALIGN %d.",
ControlFile->maxAlign, MAXIMUM_ALIGNOF),
errhint("It looks like you need to initdb.")));
if (ControlFile->floatFormat != FLOATFORMAT_VALUE)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster appears to use a different floating-point number format than the server executable."),
errhint("It looks like you need to initdb.")));
if (ControlFile->blcksz != BLCKSZ)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with BLCKSZ %d,"
" but the server was compiled with BLCKSZ %d.",
ControlFile->blcksz, BLCKSZ),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->relseg_size != RELSEG_SIZE)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with RELSEG_SIZE %d,"
" but the server was compiled with RELSEG_SIZE %d.",
ControlFile->relseg_size, RELSEG_SIZE),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->xlog_blcksz != XLOG_BLCKSZ)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with XLOG_BLCKSZ %d,"
" but the server was compiled with XLOG_BLCKSZ %d.",
ControlFile->xlog_blcksz, XLOG_BLCKSZ),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->nameDataLen != NAMEDATALEN)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with NAMEDATALEN %d,"
" but the server was compiled with NAMEDATALEN %d.",
ControlFile->nameDataLen, NAMEDATALEN),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->indexMaxKeys != INDEX_MAX_KEYS)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with INDEX_MAX_KEYS %d,"
" but the server was compiled with INDEX_MAX_KEYS %d.",
ControlFile->indexMaxKeys, INDEX_MAX_KEYS),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->toast_max_chunk_size != TOAST_MAX_CHUNK_SIZE)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with TOAST_MAX_CHUNK_SIZE %d,"
" but the server was compiled with TOAST_MAX_CHUNK_SIZE %d.",
ControlFile->toast_max_chunk_size, (int) TOAST_MAX_CHUNK_SIZE),
errhint("It looks like you need to recompile or initdb.")));
if (ControlFile->loblksize != LOBLKSIZE)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with LOBLKSIZE %d,"
" but the server was compiled with LOBLKSIZE %d.",
ControlFile->loblksize, (int) LOBLKSIZE),
@@ -4399,14 +4417,16 @@ ReadControlFile(void)
#ifdef USE_FLOAT8_BYVAL
if (ControlFile->float8ByVal != true)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized without USE_FLOAT8_BYVAL"
" but the server was compiled with USE_FLOAT8_BYVAL."),
errhint("It looks like you need to recompile or initdb.")));
#else
if (ControlFile->float8ByVal != false)
ereport(FATAL,
- (errmsg("database files are incompatible with server"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("database files are incompatible with server"),
errdetail("The database cluster was initialized with USE_FLOAT8_BYVAL"
" but the server was compiled without USE_FLOAT8_BYVAL."),
errhint("It looks like you need to recompile or initdb.")));
@@ -5282,7 +5302,8 @@ CheckRequiredParameterValues(void)
if (ArchiveRecoveryRequested && ControlFile->wal_level == WAL_LEVEL_MINIMAL)
{
ereport(FATAL,
- (errmsg("WAL was generated with wal_level=minimal, cannot continue recovering"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("WAL was generated with wal_level=minimal, cannot continue recovering"),
errdetail("This happens if you temporarily set wal_level=minimal on the server."),
errhint("Use a backup taken after setting wal_level to higher than minimal.")));
}
@@ -5348,7 +5369,8 @@ StartupXLOG(void)
*/
if (!XRecOffIsValid(ControlFile->checkPoint))
ereport(FATAL,
- (errmsg("control file contains invalid checkpoint location")));
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("control file contains invalid checkpoint location")));
switch (ControlFile->state)
{
@@ -5399,7 +5421,8 @@ StartupXLOG(void)
default:
ereport(FATAL,
- (errmsg("control file contains invalid database cluster state")));
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("control file contains invalid database cluster state")));
}
/* This is just to allow attaching to startup process with a debugger */
@@ -5783,11 +5806,13 @@ StartupXLOG(void)
{
if (!XLogRecPtrIsInvalid(ControlFile->backupStartPoint) || ControlFile->backupEndRequired)
ereport(FATAL,
- (errmsg("WAL ends before end of online backup"),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("WAL ends before end of online backup"),
errhint("All WAL generated while online backup was taken must be available at recovery.")));
else
ereport(FATAL,
- (errmsg("WAL ends before consistent recovery point")));
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("WAL ends before consistent recovery point")));
}
}
@@ -8564,7 +8589,9 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
Assert(false);
break;
default:
- elog(PANIC, "unrecognized wal_sync_method: %d", wal_sync_method);
+ ereport(PANIC,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg_internal("unrecognized wal_sync_method: %d", wal_sync_method));
break;
}
--
2.43.0
From df027460544a308766c2cc30459c82e7ba933c9d Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <byavu...@gmail.com>
Date: Fri, 23 Feb 2024 17:11:34 +0300
Subject: [PATCH v2 2/2] (relcache.c) Add missing error codes to PANIC/FATAL
error reports
---
src/backend/utils/cache/relcache.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 50acae45298..f503bca0fce 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4215,8 +4215,10 @@ RelationCacheInitializePhase3(void)
htup = SearchSysCache1(RELOID,
ObjectIdGetDatum(RelationGetRelid(relation)));
if (!HeapTupleIsValid(htup))
- elog(FATAL, "cache lookup failed for relation %u",
- RelationGetRelid(relation));
+ ereport(FATAL,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg_internal("cache lookup failed for relation %u",
+ RelationGetRelid(relation)));
relp = (Form_pg_class) GETSTRUCT(htup);
/*
@@ -4349,7 +4351,9 @@ load_critical_index(Oid indexoid, Oid heapoid)
LockRelationOid(indexoid, AccessShareLock);
ird = RelationBuildDesc(indexoid, true);
if (ird == NULL)
- elog(PANIC, "could not open critical system index %u", indexoid);
+ ereport(PANIC,
+ errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg_internal("could not open critical system index %u", indexoid));
ird->rd_isnailed = true;
ird->rd_refcnt = 1;
UnlockRelationOid(indexoid, AccessShareLock);
@@ -6518,7 +6522,9 @@ write_relcache_init_file(bool shared)
*/
magic = RELCACHE_INIT_FILEMAGIC;
if (fwrite(&magic, 1, sizeof(magic), fp) != sizeof(magic))
- elog(FATAL, "could not write init file");
+ ereport(FATAL,
+ errcode_for_file_access(),
+ errmsg_internal("could not write init file: %m"));
/*
* Write all the appropriate reldescs (in no particular order).
@@ -6619,7 +6625,9 @@ write_relcache_init_file(bool shared)
}
if (FreeFile(fp))
- elog(FATAL, "could not write init file");
+ ereport(FATAL,
+ errcode_for_file_access(),
+ errmsg_internal("could not write init file: %m"));
/*
* Now we have to check whether the data we've so painstakingly
@@ -6669,9 +6677,13 @@ static void
write_item(const void *data, Size len, FILE *fp)
{
if (fwrite(&len, 1, sizeof(len), fp) != sizeof(len))
- elog(FATAL, "could not write init file");
+ ereport(FATAL,
+ errcode_for_file_access(),
+ errmsg_internal("could not write init file: %m"));
if (len > 0 && fwrite(data, 1, len, fp) != len)
- elog(FATAL, "could not write init file");
+ ereport(FATAL,
+ errcode_for_file_access(),
+ errmsg_internal("could not write init file: %m"));
}
/*
--
2.43.0
