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