Adjust errorcode in background worker code

Started by Amit Langoteover 10 years ago5 messages

Langote_Amit_f8@lab.ntt.co.jp

over 10 years ago

1 attachment(s)

Hi,

How about the attached that adjusts errorcode for the error related to
checking the flag bgw_flags in BackgroundWorkerInitializeConnection*()
functions so that it matches the treatment in SanityCheckBackgroundWorker()?

s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g

There is already a "/* XXX is this the right errcode? */" there.

Thanks,
Amit

Attachments:

pgut_readopt-error-level.patchtext/x-diff; name=pgut_readopt-error-level.patchDownload

diff --git a/catalog.c b/catalog.c
index c9e1f2b..b6058e4 100644
--- a/catalog.c
+++ b/catalog.c
@@ -499,7 +499,7 @@ catalog_read_ini(const char *path)
 	options[i++].var = &status;
 	Assert(i == lengthof(options) - 1);
 
-	pgut_readopt(path, options, ERROR_CORRUPTED);
+	pgut_readopt(path, options, ERROR);
 
 	if (backup_mode)
 	{
diff --git a/pg_rman.c b/pg_rman.c
index 532806e..6dd4d9d 100644
--- a/pg_rman.c
+++ b/pg_rman.c
@@ -163,7 +163,7 @@ main(int argc, char *argv[])
 		}
 
 		join_path_components(path, backup_path, PG_RMAN_INI_FILE);
-		pgut_readopt(path, options, ERROR_ARGS);
+		pgut_readopt(path, options, ERROR);
 	}
 
 	/* BACKUP_PATH is always required */
diff --git a/pgut/pgut.c b/pgut/pgut.c
index ee8810c..467b278 100644
--- a/pgut/pgut.c
+++ b/pgut/pgut.c
@@ -698,7 +698,7 @@ pgut_readopt(const char *path, pgut_option options[], int elevel)
 				}
 			}
 			// Reach here if the option read from configuration file is not found in options[] 
-			if ( elevel == ERROR_ARGS && !options[j].type)
+			if (!options[j].type)
 				elog(elevel, "invalid option \"%s\"", key);
 		}
 	}

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 10 years ago

In reply to: Amit Langote (#1)

1 attachment(s)

Re: Adjust errorcode in background worker code

On 2015-06-29 AM 11:36, Amit Langote wrote:

Hi,

How about the attached that adjusts errorcode for the error related to
checking the flag bgw_flags in BackgroundWorkerInitializeConnection*()
functions so that it matches the treatment in SanityCheckBackgroundWorker()?

s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g

There is already a "/* XXX is this the right errcode? */" there.

Oops, a wrong thing got attached.

Please find correct one attached this time.

Thanks,
Amit

Attachments:

adjust-errcode-bgw-init-conn.patchtext/x-diff; name=adjust-errcode-bgw-init-conn.patchDownload

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index df8037b..d835775 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5333,10 +5333,9 @@ BackgroundWorkerInitializeConnection(char *dbname, char *username)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
 
-	/* XXX is this the right errcode? */
 	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
 		ereport(FATAL,
-				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("database connection requirement not indicated during registration")));
 
 	InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL);
@@ -5356,10 +5355,9 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid)
 {
 	BackgroundWorker *worker = MyBgworkerEntry;
 
-	/* XXX is this the right errcode? */
 	if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
 		ereport(FATAL,
-				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("database connection requirement not indicated during registration")));
 
 	InitPostgres(NULL, dboid, NULL, useroid, NULL);

Robert Haas

robertmhaas@gmail.com

about 10 years ago

In reply to: Amit Langote (#2)

Re: Adjust errorcode in background worker code

On Sun, Jun 28, 2015 at 10:43 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015-06-29 AM 11:36, Amit Langote wrote:

Hi,

How about the attached that adjusts errorcode for the error related to
checking the flag bgw_flags in BackgroundWorkerInitializeConnection*()
functions so that it matches the treatment in SanityCheckBackgroundWorker()?

s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g

There is already a "/* XXX is this the right errcode? */" there.

Oops, a wrong thing got attached.

Please find correct one attached this time.

Well, I'm just catching up on some old email and saw this thread. I
like the idea of trying to use the best possible error code, but I'm
not so sure this is an improvement. One problem is that
ERRCODE_INVALID_PARAMETER_VALUE is that we use it, uh, a lot:

[rhaas pgsql]$ git grep ERRCODE_ | sed 's/.*ERRCODE_/ERRCODE_/;
s/[^A-Z0-9_].*//;' | sort | uniq -c | sort -n -r | head
540 ERRCODE_FEATURE_NOT_SUPPORTED
442 ERRCODE_INVALID_PARAMETER_VALUE
380 ERRCODE_SYNTAX_ERROR
194 ERRCODE_WRONG_OBJECT_TYPE
194 ERRCODE_UNDEFINED_OBJECT
181 ERRCODE_DATATYPE_MISMATCH
180 ERRCODE_INSUFFICIENT_PRIVILEGE
150 ERRCODE_INVALID_TEXT_REPRESENTATION
137 ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
123 ERRCODE_PROGRAM_LIMIT_EXCEEDED

I wonder if we need to think about inventing some new error codes. I
can sort of understand that "feature not supported" is something that
can come in a large number of different contexts and mean pretty much
the same all the time, but I'm betting that things like "invalid
parameter value" and "invalid text representation" and "object not in
prerequisite state" cover an amazing breadth of errors that may not
actually be that similar to each other.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Craig Ringer

craig@2ndquadrant.com

about 10 years ago

In reply to: Robert Haas (#3)

Re: Adjust errorcode in background worker code

On 7 November 2015 at 02:55, Robert Haas <robertmhaas@gmail.com> wrote:

I wonder if we need to think about inventing some new error codes. I
can sort of understand that "feature not supported" is something that
can come in a large number of different contexts and mean pretty much
the same all the time, but I'm betting that things like "invalid
parameter value" and "invalid text representation" and "object not in
prerequisite state" cover an amazing breadth of errors that may not
actually be that similar to each other.

That depends on what client applications are going to do about the error.

If the app's only choice is likely to be "huh, that's weird, I'll barf
and tell the user about it" then there's little point changing
anything. The app just spits out a different error and nobody gains
anything.

If there are error sites where an application could take specific,
useful action in response to an error then they could well benefit
from being changed. ERRCODE_T_R_DEADLOCK_DETECTED for example is
extremely useful to applications.

So the first step should be auditing error sites of interest to see
whether the client app could possibly do anything constructive about
the error. Then see if there's an existing category it'd fit better,
or if we have a group of errors that would go well together in a new
category where the application action for them is similar.

Having too many highly specific errors is at least as bad as too few
broad categories. You land up with huge case statements and error
number lists - which you inevitably miss something from. They're
annoying to write, so they start getting re-used and cargo culted in
increasingly outdated forms. Next thing you know you've invented Java
exception handling ;)

Personally the only PostgreSQL errors I've ever cared about handling
specifically have been:

* deadlock detected, serialization failure for tx abort and retry loop
* connection-level errors, for reconnect and retry handling, graceful
reporting of DB shutdown, etc
* constraint violations, where useful info can be extracted to tell
the user what exactly was wrong with their input
* access permission errors, to drive more informative client-side UI

The great majority of sever-side errors go into the "huh, something's
broken that shouldn't have, tell the user to tell the admin" box.

I'm nuts about handling errors in my code compared to most of what
I've seen. The majority of apps I see aren't even prepared to cope
with a deadlock without data loss, tending to barf an error into the
logs and/or to the user, forget what they were doing, and hope someone
comes to rescue them or re-enter the info.

Do you think anyone has *ever* written code to trap
ERRCODE_AMBIGUOUS_FUNCTION or ERRCODE_INVALID_ARGUMENT_FOR_LOG? Not
counting simple translation layers that map every exception ... I
doubt it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

about 10 years ago

In reply to: Robert Haas (#3)

Re: Adjust errorcode in background worker code

On 2015/11/07 3:55, Robert Haas wrote:

On Sun, Jun 28, 2015 at 10:43 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2015-06-29 AM 11:36, Amit Langote wrote:

Hi,

How about the attached that adjusts errorcode for the error related to
checking the flag bgw_flags in BackgroundWorkerInitializeConnection*()
functions so that it matches the treatment in SanityCheckBackgroundWorker()?

s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g

There is already a "/* XXX is this the right errcode? */" there.

Oops, a wrong thing got attached.

Please find correct one attached this time.

Well, I'm just catching up on some old email and saw this thread. I
like the idea of trying to use the best possible error code, but I'm
not so sure this is an improvement. One problem is that
ERRCODE_INVALID_PARAMETER_VALUE is that we use it, uh, a lot:

[rhaas pgsql]$ git grep ERRCODE_ | sed 's/.*ERRCODE_/ERRCODE_/;
s/[^A-Z0-9_].*//;' | sort | uniq -c | sort -n -r | head
540 ERRCODE_FEATURE_NOT_SUPPORTED
442 ERRCODE_INVALID_PARAMETER_VALUE
380 ERRCODE_SYNTAX_ERROR
194 ERRCODE_WRONG_OBJECT_TYPE
194 ERRCODE_UNDEFINED_OBJECT
181 ERRCODE_DATATYPE_MISMATCH
180 ERRCODE_INSUFFICIENT_PRIVILEGE
150 ERRCODE_INVALID_TEXT_REPRESENTATION
137 ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
123 ERRCODE_PROGRAM_LIMIT_EXCEEDED

I wonder if we need to think about inventing some new error codes. I
can sort of understand that "feature not supported" is something that
can come in a large number of different contexts and mean pretty much
the same all the time, but I'm betting that things like "invalid
parameter value" and "invalid text representation" and "object not in
prerequisite state" cover an amazing breadth of errors that may not
actually be that similar to each other.

Now that I see it, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE may be a
better choice there.

In general, I tend to agree with the observations expressed by Craig
down-thread. I'd think this particular error code (or more to the point,
the error site) does not concern a client app itself but rather suggests a
bug in a loadable module implementation which the client app has no way to
fix itself. In general, it seems we should only ever report error codes
that a client app can do something about. But I guess that's a whole
different area and vast project to investigate about. For a rather
contrived example related to OP, it would have made sense to send an error
code if there was a parameter like worker_spi.can_connect_db which the app
set to false and then later did something with it that required a database
connection.

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers