Date: Wed, 27 Nov 2024 13:34:22 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	Bruce Momjian <bruce@momjian.us>, adam@labkey.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18711: Attempting a connection with a database name longer
 than 63 characters now fails
Message-ID: <Z0d0PqBQGQQSlxB3@nathan>
References: <665338.1732306066@sss.pgh.pa.us>
 <Z0Dmw1ryKH8bEkyo@nathan>
 <Z0c9XVU6aTd8neRr@nathan>
 <1865227.1732723416@sss.pgh.pa.us>
 <Z0dKd7bvo3D2NdLt@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0dNyskuXOCM-dOz@nathan>
 <Z0daSnNI8FrrSvhW@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0db25rMTSFrK-FN@nathan>
 <Z0de/7/6wgGY074a@ip-10-97-1-34.eu-west-3.compute.internal>
 <1896106.1732731438@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="v5KPrh7Od05B3rEB"
Content-Disposition: inline
In-Reply-To: <1896106.1732731438@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Z0d0PqBQGQQSlxB3%40nathan>
Precedence: bulk


--v5KPrh7Od05B3rEB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Nov 27, 2024 at 01:17:18PM -0500, Tom Lane wrote:
> I have an idea that might salvage something from what we've just
> been doing.  The biggest single problem with the old behavior,
> AIUI, was the possibility of sending back encoding-corrupt names
> in connection failure reports.  How about something like:
> 
> * Don't truncate the names on sight (so we keep 562bee0fc1).
> 
> * Truncate the names at NAMEDATALEN-1 just before catalog lookup
> (this restores the old lookup behavior, solving the current report).
> 
> * In lookup-failure messages, be sure to report the un-truncated
> name (avoiding the bad-encoding problem).
> 
> * Copy the correctly-truncated names from the catalog to any
> persistent storage such as MyProcPort, so that those are used
> for all post-connection purposes.

One possible issue with truncating the names after-the-fact is that hooks
like ClientAuthentication_hook will have already been called with the
untruncated names.  If it's just the connection failure reports we are
worried about, then maybe we should save the "raw" names to MyProcPort
before truncating and be sure to use those in the log messages.

I attached a rough sketch of what I'm imagining.  If we aren't thrilled
about adding new fields to "struct Port" in a minor release, we could just
do a straight revert on the back-branches and add that part to v18.

-- 
nathan

--v5KPrh7Od05B3rEB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="save_untruncated_names.patch"

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 47e8c91606..d81c3eeba9 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -320,7 +320,7 @@ auth_failed(Port *port, int status, const char *logdetail)
 
 	ereport(FATAL,
 			(errcode(errcode_return),
-			 errmsg(errstr, port->user_name),
+			 errmsg(errstr, port->user_name_raw),
 			 logdetail ? errdetail_log("%s", logdetail) : 0));
 
 	/* doesn't return */
@@ -459,14 +459,14 @@ ClientAuthentication(Port *port)
 							(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 					/* translator: last %s describes encryption state */
 							 errmsg("pg_hba.conf rejects replication connection for host \"%s\", user \"%s\", %s",
-									hostinfo, port->user_name,
+									hostinfo, port->user_name_raw,
 									encryption_state)));
 				else
 					ereport(FATAL,
 							(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 					/* translator: last %s describes encryption state */
 							 errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\", %s",
-									hostinfo, port->user_name,
+									hostinfo, port->user_name_raw,
 									port->database_name,
 									encryption_state)));
 				break;
@@ -526,7 +526,7 @@ ClientAuthentication(Port *port)
 							(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 					/* translator: last %s describes encryption state */
 							 errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\", %s",
-									hostinfo, port->user_name,
+									hostinfo, port->user_name_raw,
 									encryption_state),
 							 HOSTNAME_LOOKUP_DETAIL(port)));
 				else
@@ -534,7 +534,7 @@ ClientAuthentication(Port *port)
 							(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 					/* translator: last %s describes encryption state */
 							 errmsg("no pg_hba.conf entry for host \"%s\", user \"%s\", database \"%s\", %s",
-									hostinfo, port->user_name,
+									hostinfo, port->user_name_raw,
 									port->database_name,
 									encryption_state),
 							 HOSTNAME_LOOKUP_DETAIL(port)));
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c
index 2a96c81f92..3b60559ae9 100644
--- a/src/backend/tcop/backend_startup.c
+++ b/src/backend/tcop/backend_startup.c
@@ -830,6 +830,22 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
 	if (port->database_name == NULL || port->database_name[0] == '\0')
 		port->database_name = pstrdup(port->user_name);
 
+	/*
+	 * Prior to truncating, save the raw database and user names for log
+	 * messages.
+	 */
+	port->database_name_raw = pstrdup(port->database_name);
+	port->user_name_raw = pstrdup(port->user_name);
+
+	/*
+	 * Truncate given database and user names to length of a Postgres name.
+	 * This avoids lookup failures when overlength names are given.
+	 */
+	if (strlen(port->database_name) >= NAMEDATALEN)
+		port->database_name[NAMEDATALEN - 1] = '\0';
+	if (strlen(port->user_name) >= NAMEDATALEN)
+		port->user_name[NAMEDATALEN - 1] = '\0';
+
 	if (am_walsender)
 		MyBackendType = B_WAL_SENDER;
 	else
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 3b7b2ebec0..7e3740fa0f 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -798,7 +798,8 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
 				return;
 			ereport(FATAL,
 					(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
-					 errmsg("role \"%s\" does not exist", rolename)));
+					 errmsg("role \"%s\" does not exist",
+							MyProcPort ? MyProcPort->user_name_raw : rolename)));
 		}
 	}
 	else
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 5b657a3f13..4f949a8f2d 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -1008,7 +1008,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
 		if (!HeapTupleIsValid(tuple))
 			ereport(FATAL,
 					(errcode(ERRCODE_UNDEFINED_DATABASE),
-					 errmsg("database \"%s\" does not exist", in_dbname)));
+					 errmsg("database \"%s\" does not exist",
+							MyProcPort ? MyProcPort->database_name_raw : in_dbname)));
 		dbform = (Form_pg_database) GETSTRUCT(tuple);
 		dboid = dbform->oid;
 	}
@@ -1072,7 +1073,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
 			if (in_dbname)
 				ereport(FATAL,
 						(errcode(ERRCODE_UNDEFINED_DATABASE),
-						 errmsg("database \"%s\" does not exist", in_dbname),
+						 errmsg("database \"%s\" does not exist",
+								MyProcPort ? MyProcPort->database_name_raw : in_dbname),
 						 errdetail("It seems to have just been dropped or renamed.")));
 			else
 				ereport(FATAL,
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 9109b2c334..9031cabfc7 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -232,6 +232,9 @@ typedef struct Port
 	char	   *raw_buf;
 	ssize_t		raw_buf_consumed,
 				raw_buf_remaining;
+
+	char	   *database_name_raw;
+	char	   *user_name_raw;
 } Port;
 
 /*

--v5KPrh7Od05B3rEB--