Hi all,
While analyzing the use of internal error codes in the code base, I've
some problems, and that's a mixed bag of:
- Incorrect uses, for errors that can be triggered by users with
vallid cases.
- Expected error cases, wanted by the tests like corruption cases or
just to keep some code simpler.
Here is a summary of the ones that should be fixed with proper
errcodes:
1) be-secure-openssl.c is forgetting an error codes for code comparing
the ssl_{min,max}_protocol_version range, which should be a
ERRCODE_CONFIG_FILE_ERROR.
2) 010_pg_basebackup.pl includes a test for an unmatching system ID at
its bottom, that triggers an internal error as an effect of
manifest_report_error().
3) basebackup.c, with a too long symlink or tar name, where
ERRCODE_PROGRAM_LIMIT_EXCEEDED should make sense.
4) pg_walinspect, for an invalid LSN. That would be
ERRCODE_INVALID_PARAMETER_VALUE.
5) Some paths of pg_ucol_open() are failing internally, still these
refer to parameters that can be set, so I've attached
ERRCODE_INVALID_PARAMETER_VALUE to them.
6) PerformWalRecovery(), where recovery ends before target is reached,
for a ERRCODE_CONFIG_FILE_ERROR.
7) pg_replication_slot_advance(), missing for the target LSN a
ERRCODE_INVALID_PARAMETER_VALUE.
8) Isolation test alter-table-4/s2, able to trigger a "attribute "a"
of relation "c1" does not match parent's type". Shouldn't that be a
ERRCODE_INVALID_COLUMN_DEFINITION?
Then there is a series of issues triggered by the main regression test
suite, applying three times (pg_upgrade, make check and
027_stream_regress.pl):
1) MergeConstraintsIntoExisting() under a case of relhassubclass, for
ERRCODE_INVALID_OBJECT_DEFINITION.
2) Trigger rename on a partition, see renametrig(), for
ERRCODE_FEATURE_NOT_SUPPORTED.
3) "could not find suitable unique index on materialized view", with a
plain elog() in matview.c, for ERRCODE_FEATURE_NOT_SUPPORTED
4) "role \"blah\" cannot have explicit members", for
ERRCODE_FEATURE_NOT_SUPPORTED.
5) Similar to previous one, AddRoleMems() with "role \"blah\" cannot
be a member of any role"
6) icu_validate_locale(), icu_language_tag() and make_icu_collator()
for invalid parameter inputs.
7) ATExecAlterConstraint()
There are a few fancy cases where we expect an internal error per the
state of the tests:
1) amcheck
1-1) bt_index_check_internal() in amcheck, where the code has the idea
to open an OID given in input, trigerring an elog(). That does not
strike me as a good idea, though that's perhaps acceptable. The error
is an "could not open relation with OID".
1-2) 003_check.pl has 12 cases with backtraces expected in the outcome
as these check corruption cases.
2) pg_visibility does the same thing, for two tests trigerring a
"could not open relation with OID".
3) plpython with 6 cases which are internal, not sure what to do about
these.
4) test_resowner has two cases triggered by SQL functions, which are
expected to be internal.
5) test_tidstore, with "tuple offset out of range" triggered by a SQL
call.
6) injection_points, that are aimed for tests, has six backtraces.
7) currtid_internal().. Perhaps we should try to rip out this stuff,
which is specific to ODBC. There are a lot of backtraces here.
8) satisfies_hash_partition() in test hash_part, generating a
backtrace for an InvalidOid in the main regression test suite.
All these cases are able to trigger backtraces, and while of them are
OK to keep as they are, the cases of the first and second lists ought
to be fixed, and attached is a patch do close the gap. This reduces
the number of internal errors generated by the tests from 85 to 35,
with injection points enabled.
Note that I've kept the currtid() errcodes in it, though I don't think
much of them. The rest looks sensible enough to address.
Thoughts?
--
Michael
From 8386cf2afd1496bea159100bdccb6afd93c6e1c8 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 23 Apr 2024 13:53:28 +0900
Subject: [PATCH] Assign some errcodes where missing
---
src/backend/access/transam/xlogrecovery.c | 3 ++-
src/backend/backup/basebackup.c | 6 --
src/backend/commands/matview.c| 4 +++-
src/backend/commands/tablecmds.c | 4 +++-
src/backend/commands/trigger.c| 1 +
src/backend/commands/user.c | 2 ++
src/backend/libpq/be-secure-openssl.c | 3 ++-
src/backend/optimizer/util/appendinfo.c | 12
src/backend/replication/slotfuncs.c | 3 ++-
src/backend/utils/adt/pg_locale.c | 21 +---
src/backend/utils/adt/tid.c | 24 ---
contrib/pg_walinspect/pg_walinspect.c | 3 ++-
12 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 29c5bec084..a6543b539e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1898,7 +1898,8 @@ PerformWalRecovery(void)
