public inbox for [email protected]
help / color / mirror / Atom feedRe: [HACKERS] psql casts aspersions on server reliability
3+ messages / 2 participants
[nested] [flat]
* Re: [HACKERS] psql casts aspersions on server reliability
@ 2023-11-24 15:19 Bruce Momjian <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Bruce Momjian @ 2023-11-24 15:19 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, Nov 24, 2023 at 04:06:22AM +0100, Laurenz Albe wrote:
> On Thu, 2023-11-23 at 11:12 -0500, Bruce Momjian wrote:
> > On Wed, Nov 22, 2023 at 10:25:14PM -0500, Bruce Momjian wrote:
> > > Yes, you are correct. Here is a patch that implements the FATAL test,
> > > though I am not sure I have the logic correct or backwards, and I don't
> > > know how to test this. Thanks.
> >
> > I developed the attached patch which seems to work better. In testing
> > kill -3 on a backend or calling elog(FATAL) in the server for a
> > session, libpq's 'res' is NULL, meaning we don't have any status to
> > check for PGRES_FATAL_ERROR. It is very possible that libpq just isn't
> > stuctured to have the PGRES_FATAL_ERROR at the point where we issue this
> > message, and this is not worth improving.
> >
> > test=> select pg_sleep(100);
> > --> FATAL: FATAL called
> >
> > server closed the connection unexpectedly
> > --> This probably means the server terminated null
> > before or while processing the request.
> > The connection to the server was lost. Attempting reset: Succeeded.
>
> I don't thing "terminated null" is a meaningful message.
Yes, this is just a debug build so we can see the values of 'res'.
Sorry for the confusion. This attached patch has the elog() added so
you can reproduce what I saw.
I am actually unclear which exits should be labled as "abnormal".
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
[text/x-diff] exit.diff (4.9K, ../../[email protected]/2-exit.diff)
download | inline diff:
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 5d78d6dc06..890fe3bd4a 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -372,6 +372,8 @@ pg_sleep(PG_FUNCTION_ARGS)
float8 secs = PG_GETARG_FLOAT8(0);
float8 endtime;
+ elog(FATAL, "FATAL from pg_sleep()");
+
/*
* We sleep using WaitLatch, to ensure that we'll wake up promptly if an
* important signal (such as SIGALRM or SIGINT) arrives. Because
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 660cdec93c..64faad19df 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -749,8 +749,11 @@ retry4:
*/
definitelyEOF:
libpq_append_conn_error(conn, "server closed the connection unexpectedly\n"
- "\tThis probably means the server terminated abnormally\n"
- "\tbefore or while processing the request.");
+ "\tThis probably means the server terminated%s\n"
+ "\tbefore or while processing the request.",
+ (conn->result == NULL) ? " null" :
+ (conn->result->resultStatus == PGRES_FATAL_ERROR) ?
+ "" : " abnormally");
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 5613c56b14..03914b97fc 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -2158,6 +2158,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
if (pqGetErrorNotice3(conn, true))
continue;
status = PGRES_FATAL_ERROR;
+ fprintf(stderr, "Got 'E'\n");
break;
case 'A': /* notify message */
/* handle notify and go back to processing return values */
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index f1192d28f2..f4c7f51b0a 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -206,8 +206,11 @@ rloop:
if (result_errno == EPIPE ||
result_errno == ECONNRESET)
libpq_append_conn_error(conn, "server closed the connection unexpectedly\n"
- "\tThis probably means the server terminated abnormally\n"
- "\tbefore or while processing the request.");
+ "\tThis probably means the server terminated%s\n"
+ "\tbefore or while processing the request.",
+ (conn->result == NULL) ? " null" :
+ (conn->result->resultStatus == PGRES_FATAL_ERROR) ?
+ "" : " abnormally");
else
libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
SOCK_STRERROR(result_errno,
@@ -306,8 +309,11 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
result_errno = SOCK_ERRNO;
if (result_errno == EPIPE || result_errno == ECONNRESET)
libpq_append_conn_error(conn, "server closed the connection unexpectedly\n"
- "\tThis probably means the server terminated abnormally\n"
- "\tbefore or while processing the request.");
+ "\tThis probably means the server terminated%s\n"
+ "\tbefore or while processing the request.",
+ (conn->result == NULL) ? " null" :
+ (conn->result->resultStatus == PGRES_FATAL_ERROR) ?
+ "" : " abnormally");
else
libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
SOCK_STRERROR(result_errno,
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index bd72a87bbb..be93c2c0f9 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -233,8 +233,11 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
case EPIPE:
case ECONNRESET:
libpq_append_conn_error(conn, "server closed the connection unexpectedly\n"
- "\tThis probably means the server terminated abnormally\n"
- "\tbefore or while processing the request.");
+ "\tThis probably means the server terminated%s\n"
+ "\tbefore or while processing the request.",
+ (conn->result == NULL) ? " null" :
+ (conn->result->resultStatus == PGRES_FATAL_ERROR) ?
+ "" : " abnormally");
break;
default:
@@ -395,8 +398,12 @@ retry_masked:
/* (strdup failure is OK, we'll cope later) */
snprintf(msgbuf, sizeof(msgbuf),
libpq_gettext("server closed the connection unexpectedly\n"
- "\tThis probably means the server terminated abnormally\n"
- "\tbefore or while processing the request."));
+ "\tThis probably means the server terminated%s\n"
+ "\tbefore or while processing the request."),
+ (conn->result == NULL) ? " null" :
+ (conn->result->resultStatus == PGRES_FATAL_ERROR) ?
+ "" : " abnormally");
+
/* keep newline out of translated string */
strlcat(msgbuf, "\n", sizeof(msgbuf));
conn->write_err_msg = strdup(msgbuf);
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [HACKERS] psql casts aspersions on server reliability
@ 2023-12-08 02:59 Bruce Momjian <[email protected]>
parent: Bruce Momjian <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Bruce Momjian @ 2023-12-08 02:59 UTC (permalink / raw)
To: Laurenz Albe <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, Nov 24, 2023 at 10:19:29AM -0500, Bruce Momjian wrote:
> On Fri, Nov 24, 2023 at 04:06:22AM +0100, Laurenz Albe wrote:
> > On Thu, 2023-11-23 at 11:12 -0500, Bruce Momjian wrote:
> > > On Wed, Nov 22, 2023 at 10:25:14PM -0500, Bruce Momjian wrote:
> > > > Yes, you are correct. Here is a patch that implements the FATAL test,
> > > > though I am not sure I have the logic correct or backwards, and I don't
> > > > know how to test this. Thanks.
> > >
> > > I developed the attached patch which seems to work better. In testing
> > > kill -3 on a backend or calling elog(FATAL) in the server for a
> > > session, libpq's 'res' is NULL, meaning we don't have any status to
> > > check for PGRES_FATAL_ERROR. It is very possible that libpq just isn't
> > > structured to have the PGRES_FATAL_ERROR at the point where we issue this
> > > message, and this is not worth improving.
> > >
> > > test=> select pg_sleep(100);
> > > --> FATAL: FATAL called
> > >
> > > server closed the connection unexpectedly
> > > --> This probably means the server terminated null
> > > before or while processing the request.
> > > The connection to the server was lost. Attempting reset: Succeeded.
> >
> > I don't thing "terminated null" is a meaningful message.
>
> Yes, this is just a debug build so we can see the values of 'res'.
> Sorry for the confusion. This attached patch has the elog() added so
> you can reproduce what I saw.
>
> I am actually unclear which exits should be labeled as "abnormal".
There are five call sites which issue this message, so I looked at
adding "abnormally" just at the call sites where it made sense, but I
couldn't find a pattern. I don't plan to pursue this further.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH v2 1/2] Do not lock tables in get_tables_to_repack().
@ 2026-06-16 06:49 ChangAo Chen <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: ChangAo Chen @ 2026-06-16 06:49 UTC (permalink / raw)
When doing a whole database repack, we build a list of repackable
tables and take a lock on them to prevent concurrent drops. But
concurrent drops can always happen after we build the list because
we process each table in a separate transaction. The
ConditionalLockRelationOid() also makes the default behavior like
SKIP_LOCKED, which is unexpected.
To remove the locks, we need to make repack_is_permitted_for_relation()
handles concurrent drops correctly: it should not report an error
when failing to search the syscache in pg_class_aclcheck(). Use
pg_class_aclcheck_ext() instead to detect a concurrent drop. Also
check the return value of get_rel_name().
While at it, replace relation_close() with table_close() to match
the table_open().
---
src/backend/commands/repack.c | 67 ++++++++++-------------------------
1 file changed, 19 insertions(+), 48 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index faa07d1a118..2879c8af574 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -2169,22 +2169,9 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
index = (Form_pg_index) GETSTRUCT(tuple);
- /*
- * Try to obtain a light lock on the index's table, to ensure it
- * doesn't go away while we collect the list. If we cannot, just
- * disregard it. Be sure to release this if we ultimately decide
- * not to process the table!
- */
- if (!ConditionalLockRelationOid(index->indrelid, AccessShareLock))
- continue;
-
- /* Verify that the table still exists; skip if not */
classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid));
if (!HeapTupleIsValid(classtup))
- {
- UnlockRelationOid(index->indrelid, AccessShareLock);
continue;
- }
classForm = (Form_pg_class) GETSTRUCT(classtup);
/* Skip temp relations belonging to other sessions */
@@ -2192,7 +2179,6 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
!isTempOrTempToastNamespace(classForm->relnamespace))
{
ReleaseSysCache(classtup);
- UnlockRelationOid(index->indrelid, AccessShareLock);
continue;
}
@@ -2201,10 +2187,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
/* noisily skip rels which the user can't process */
if (!repack_is_permitted_for_relation(cmd, index->indrelid,
GetUserId()))
- {
- UnlockRelationOid(index->indrelid, AccessShareLock);
continue;
- }
/* Use a permanent memory context for the result list */
oldcxt = MemoryContextSwitchTo(permcxt);
@@ -2228,45 +2211,20 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
class = (Form_pg_class) GETSTRUCT(tuple);
- /*
- * Try to obtain a light lock on the table, to ensure it doesn't
- * go away while we collect the list. If we cannot, just
- * disregard the table. Be sure to release this if we ultimately
- * decide not to process the table!
- */
- if (!ConditionalLockRelationOid(class->oid, AccessShareLock))
- continue;
-
- /* Verify that the table still exists */
- if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(class->oid)))
- {
- UnlockRelationOid(class->oid, AccessShareLock);
- continue;
- }
-
/* Can only process plain tables and matviews */
if (class->relkind != RELKIND_RELATION &&
class->relkind != RELKIND_MATVIEW)
- {
- UnlockRelationOid(class->oid, AccessShareLock);
continue;
- }
/* Skip temp relations belonging to other sessions */
if (class->relpersistence == RELPERSISTENCE_TEMP &&
!isTempOrTempToastNamespace(class->relnamespace))
- {
- UnlockRelationOid(class->oid, AccessShareLock);
continue;
- }
/* noisily skip rels which the user can't process */
if (!repack_is_permitted_for_relation(cmd, class->oid,
GetUserId()))
- {
- UnlockRelationOid(class->oid, AccessShareLock);
continue;
- }
/* Use a permanent memory context for the result list */
oldcxt = MemoryContextSwitchTo(permcxt);
@@ -2279,7 +2237,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt)
}
table_endscan(scan);
- relation_close(catalog, AccessShareLock);
+ table_close(catalog, AccessShareLock);
return rtcs;
}
@@ -2357,15 +2315,28 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid,
static bool
repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid)
{
+ bool is_missing = false;
+
Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK);
- if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK)
+ if (pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing) == ACLCHECK_OK)
return true;
- ereport(WARNING,
- errmsg("permission denied to execute %s on \"%s\", skipping it",
- RepackCommandAsString(cmd),
- get_rel_name(relid)));
+ /* Report a warning if the relation still exists. */
+ if (!is_missing)
+ {
+ char *relname;
+
+ relname = get_rel_name(relid);
+ if (relname != NULL)
+ {
+ ereport(WARNING,
+ errmsg("permission denied to execute %s on \"%s\", skipping it",
+ RepackCommandAsString(cmd), relname));
+
+ pfree(relname);
+ }
+ }
return false;
}
--
2.47.3
--op6mnexl7cn72cto
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment; filename=v2-0002-fixups.patch
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-06-16 06:49 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-24 15:19 Re: [HACKERS] psql casts aspersions on server reliability Bruce Momjian <[email protected]>
2023-12-08 02:59 ` Bruce Momjian <[email protected]>
2026-06-16 06:49 [PATCH v2 1/2] Do not lock tables in get_tables_to_repack(). ChangAo Chen <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox