Date: Fri, 22 Nov 2024 14:01:41 -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: <Z0DjJbJV4ZWkLuQd@nathan>
References: <Zz9B3KQGXFCGVPXy@momjian.us>
 <Zz9FRrwJRlyGBFPN@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zz9IHPBf-z8MsLdw@momjian.us>
 <Zz9OTyWAvATeeHev@nathan>
 <Zz9jfOkVmlYcYHSy@momjian.us>
 <Zz9pOi3pGF-DnJTp@nathan>
 <Zz92cka2VlBDsat3@momjian.us>
 <Zz94l3LHsML0V9eu@nathan>
 <Z0A2o+dS0MqDE2Fs@ip-10-97-1-34.eu-west-3.compute.internal>
 <659706.1732303427@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="C/iOis7Qr7jPeu5S"
Content-Disposition: inline
In-Reply-To: <659706.1732303427@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Z0DjJbJV4ZWkLuQd%40nathan>
Precedence: bulk


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

On Fri, Nov 22, 2024 at 02:23:47PM -0500, Tom Lane wrote:
> After more thought I don't really like the idea of failing if there
> are multiple matches.  It means that we might fail in cases where
> pre-v17 worked fine (because NAMEDATALEN-1 was accidentally the
> right truncation point).  ISTM the entire point of this patch is to
> restore the pre-v17 behavior as much as possible, so that seems like
> the wrong outcome.

That's fine with me.
 
> So that means we could do something like the attached.  (There's
> room for argument about which error messages in InitPostgres
> should use in_dbname versus the truncated name, but I chose to
> use in_dbname for the two "does not exist" reports.)

Looks reasonable to me.  I had been thinking of ways to simplify this code
based on Bruce's feedback, and I came up with the attached.  If nothing
else, this at least helps minimize the amount of code that is only reached
in extremely rare cases, but it's probably not quite as performant as v4
for the usual path, so v4 may still be the way to go.

I think we'll also want to do something about MyProcPort->database_name
(and the user name, of course).  In a previous patch, I tried to update it
when a match was found, but maybe it'd be better to postpone setting those
values at all until after we've found the database tuple.  AFAICT they are
only used for the process title before this code is reached.

-- 
nathan

--C/iOis7Qr7jPeu5S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v5-attempt-multibyte-aware-truncation.patch"

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 5b657a3f13..bfbc41aceb 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -68,6 +68,7 @@
 #include "utils/timeout.h"
 
 static HeapTuple GetDatabaseTuple(const char *dbname);
+static HeapTuple GetDatabaseTupleInternal(Relation relation, const char *dbname);
 static HeapTuple GetDatabaseTupleByOid(Oid dboid);
 static void PerformAuthentication(Port *port);
 static void CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections);
@@ -97,12 +98,72 @@ static void process_settings(Oid databaseid, Oid roleid);
  * descriptors for both pg_database and its indexes from the shared relcache
  * cache file, and so we can do an indexscan.  criticalSharedRelcachesBuilt
  * tells whether we got the cached descriptors.
+ *
+ * This function also deals with the trickier-than-it-sounds issue of
+ * truncating an overlength incoming database name.  We do not know which
+ * encoding the name is in, so we can't apply pg_encoding_mbcliplen() as
+ * CREATE DATABASE would have done.  We use the heuristic of truncating one
+ * byte at a time until we find a match in pg_database, relying on the
+ * assumption that the name as stored in pg_database is valid according to
+ * some server-side encoding.  This lets us constrain the search using the
+ * fact that all bytes of a multibyte character have their high bit set.
  */
 static HeapTuple
 GetDatabaseTuple(const char *dbname)
 {
 	HeapTuple	tuple;
 	Relation	relation;
+	char		tname[NAMEDATALEN];
+
+	relation = table_open(DatabaseRelationId, AccessShareLock);
+
+	for (int i = NAMEDATALEN - 1;
+		 i >= NAMEDATALEN - MAX_MULTIBYTE_CHAR_LEN;
+		 i--)
+	{
+		strncpy(tname, dbname, sizeof(tname));
+		tname[i] = '\0';
+
+		/*
+		 * Take the first match.  It's theoretically possible that we could
+		 * find more than one match if we continued to try shorter truncations
+		 * (which is what makes this a heuristic).  This is unlikely though.
+		 * We considered failing if there are multiple candidate matches, but
+		 * that cure seems worse than the disease: it might reject cases that
+		 * worked fine before v17.
+		 */
+		tuple = GetDatabaseTupleInternal(relation, tname);
+		if (HeapTupleIsValid(tuple))
+			break;
+
+		/*
+		 * On the first iteration, check if we actually truncated anything.
+		 * If not, then we don't need to bother with the successive-truncation
+		 * heuristic.
+		 */
+		if (i == NAMEDATALEN - 1 && strlen(dbname) < NAMEDATALEN)
+			break;
+
+		/*
+		 * If the bytes at the truncation point are ASCII characters, we know
+		 * that they cannot both be part of the same character, so the
+		 * original truncation by CREATE/ALTER DATABASE would have gone no
+		 * further.
+		 */
+		if (!IS_HIGHBIT_SET(dbname[i]) || !IS_HIGHBIT_SET(dbname[i - 1]))
+			break;
+	}
+
+	table_close(relation, AccessShareLock);
+
+	return tuple;
+}
+
+/* One tuple fetch for GetDatabaseTuple */
+static HeapTuple
+GetDatabaseTupleInternal(Relation relation, const char *dbname)
+{
+	HeapTuple	tuple;
 	SysScanDesc scan;
 	ScanKeyData key[1];
 
@@ -115,11 +176,10 @@ GetDatabaseTuple(const char *dbname)
 				CStringGetDatum(dbname));
 
 	/*
-	 * Open pg_database and fetch a tuple.  Force heap scan if we haven't yet
-	 * built the critical shared relcache entries (i.e., we're starting up
-	 * without a shared relcache cache file).
+	 * Try to fetch the tuple.  Force heap scan if we haven't yet built the
+	 * critical shared relcache entries (i.e., we're starting up without a
+	 * shared relcache cache file).
 	 */
-	relation = table_open(DatabaseRelationId, AccessShareLock);
 	scan = systable_beginscan(relation, DatabaseNameIndexId,
 							  criticalSharedRelcachesBuilt,
 							  NULL,
@@ -133,7 +193,6 @@ GetDatabaseTuple(const char *dbname)
 
 	/* all done */
 	systable_endscan(scan);
-	table_close(relation, AccessShareLock);
 
 	return tuple;
 }
@@ -1011,6 +1070,8 @@ InitPostgres(const char *in_dbname, Oid dboid,
 					 errmsg("database \"%s\" does not exist", in_dbname)));
 		dbform = (Form_pg_database) GETSTRUCT(tuple);
 		dboid = dbform->oid;
+		/* Save the real (possibly truncated) database name */
+		strlcpy(dbname, NameStr(dbform->datname), sizeof(dbname));
 	}
 	else if (!OidIsValid(dboid))
 	{
@@ -1066,8 +1127,9 @@ InitPostgres(const char *in_dbname, Oid dboid,
 		if (HeapTupleIsValid(tuple))
 			datform = (Form_pg_database) GETSTRUCT(tuple);
 
+		/* Note comparison here must be against the truncated DB name */
 		if (!HeapTupleIsValid(tuple) ||
-			(in_dbname && namestrcmp(&datform->datname, in_dbname)))
+			(in_dbname && namestrcmp(&datform->datname, dbname) != 0))
 		{
 			if (in_dbname)
 				ereport(FATAL,

--C/iOis7Qr7jPeu5S--