From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dilip Kumar <dilipbalaut@gmail.com>
cc: maximilian.chrzan@here.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18959: Name collisions of expression indexes during parallel
 Index creations on a pratitioned table.
In-reply-to: 
 <CAFiTN-tVuS7pWbSZtCh5kAsGihen5ESpirS811KNFPCgLBm1WQ@mail.gmail.com>
References: <18959-f63b53b864bb1417@postgresql.org>
 <CAFiTN-vB9UHF0yJxFynKuFiLkw094z7KLyDw4EAfUeaj1pZ00g@mail.gmail.com>
 <1081051.1750260111@sss.pgh.pa.us> <1138732.1750265195@sss.pgh.pa.us>
 <CAFiTN-tVuS7pWbSZtCh5kAsGihen5ESpirS811KNFPCgLBm1WQ@mail.gmail.com>
Comments: In-reply-to Dilip Kumar <dilipbalaut@gmail.com>
	message dated "Thu, 19 Jun 2025 21:34:22 +0530"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1456981.1750352190.0@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Thu, 19 Jun 2025 12:57:46 -0400
Message-ID: <1457083.1750352266@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1457083.1750352266%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <1456981.1750352190.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit

Dilip Kumar <dilipbalaut@gmail.com> writes:
> On Wed, Jun 18, 2025 at 10:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I experimented with the attached, which borrows a couple of ideas
>> from 3db61db48 to produce names like "parent_index_2" when cloning
>> indexes.  While it should help with the immediate problem, I'm not
>> sure if this is acceptable, because there are a *lot* of ensuing
>> changes in the regression tests, many more than 3db61db48 caused.

> I haven't reviewed the patch itself, but I like the idea.  We're now
> consistently using the parent index name for partitioned indexes,
> whether they're named or unnamed indexes. That looks like a great
> improvement.  And I think including the partition number of each level
> in the index name significantly enhances its clarity, especially
> within a multi-level partition hierarchy.

A different approach that we could take --- possibly alongside doing
the above --- is to try to remove the race condition between two
sessions choosing the same index name.  It doesn't look practical
to close the race window completely, but it's quite simple to make
it a whole lot shorter.  If we check for a conflicting relation
name using SnapshotDirty instead of only looking for committed
pg_class rows, then the window is little more than the time needed
to insert the index's pg_class row, rather than being the whole
time needed to build the index.  (The fact that the OP is working
with terabyte-sized tables is what's making this so bad for him.)

In the attached draft I only bothered to change the initial
probe for a conflicting pg_class entry.  We could go further and
apply the same idea in ConstraintNameExists(), but I'm not sure
it's worth the trouble.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="wip-shorten-index-name-choice-race-condition.patch";
	charset="us-ascii"
Content-ID: <1456981.1750352190.2@sss.pgh.pa.us>
Content-Description: wip-shorten-index-name-choice-race-condition.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/index=
cmds.c
index c3ec2076a52..f2898fee5fc 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2592,7 +2592,9 @@ makeObjectName(const char *name1, const char *name2,=
 const char *label)
  * constraint names.)
  *
  * Note: it is theoretically possible to get a collision anyway, if someo=
ne
- * else chooses the same name concurrently.  This is fairly unlikely to b=
e
+ * else chooses the same name concurrently.  We shorten the race conditio=
n
+ * window by checking for conflicting relations using SnapshotDirty, but
+ * that doesn't close the window entirely.  This is fairly unlikely to be
  * a problem in practice, especially if one is holding an exclusive lock =
on
  * the relation identified by name1.  However, if choosing multiple names
  * within a single command, you'd better create the new object and do
@@ -2608,15 +2610,45 @@ ChooseRelationName(const char *name1, const char *=
name2,
 	int			pass =3D 0;
 	char	   *relname =3D NULL;
 	char		modlabel[NAMEDATALEN];
+	SnapshotData SnapshotDirty;
+	Relation	pgclassrel;
+
+	/* prepare to search pg_class with a dirty snapshot */
+	InitDirtySnapshot(SnapshotDirty);
+	pgclassrel =3D table_open(RelationRelationId, AccessShareLock);
 =

 	/* try the unmodified label first */
 	strlcpy(modlabel, label, sizeof(modlabel));
 =

 	for (;;)
 	{
+		ScanKeyData key[2];
+		SysScanDesc scan;
+		bool		collides;
+
 		relname =3D makeObjectName(name1, name2, modlabel);
 =

-		if (!OidIsValid(get_relname_relid(relname, namespaceid)))
+		/* is there any conflicting relation name? */
+		ScanKeyInit(&key[0],
+					Anum_pg_class_relname,
+					BTEqualStrategyNumber, F_NAMEEQ,
+					CStringGetDatum(relname));
+		ScanKeyInit(&key[1],
+					Anum_pg_class_relnamespace,
+					BTEqualStrategyNumber, F_OIDEQ,
+					ObjectIdGetDatum(namespaceid));
+
+		scan =3D systable_beginscan(pgclassrel, ClassNameNspIndexId,
+								  true /* indexOK */ ,
+								  &SnapshotDirty,
+								  2, key);
+
+		collides =3D HeapTupleIsValid(systable_getnext(scan));
+
+		systable_endscan(scan);
+
+		/* break out of loop if no conflict */
+		if (!collides)
 		{
 			if (!isconstraint ||
 				!ConstraintNameExists(relname, namespaceid))
@@ -2628,6 +2660,8 @@ ChooseRelationName(const char *name1, const char *na=
me2,
 		snprintf(modlabel, sizeof(modlabel), "%s%d", label, ++pass);
 	}
 =

+	table_close(pgclassrel, AccessShareLock);
+
 	return relname;
 }
 =


------- =_aaaaaaaaaa0--