Date: Sat, 8 Mar 2025 19:35:18 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ayush Vatsa <ayushvatsa1810@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Message-ID: <Z8zwVmGzXyDdkAXj@nathan>
References: <CA+Tgmob_W0iq9Kuugra3WYTO2429RMJ_+HkVukrXWOUN81QiEw@mail.gmail.com>
 <1246906.1739896202@sss.pgh.pa.us>
 <CA+TgmoZYM2az+yCWu5DBnV50N_BE9f1r8-Doy6-tZTySeb-s+A@mail.gmail.com>
 <CACX+KaNAbOzePn710EtzH9F5xiUdBC+u59=UMab=Wr8jgDKQtw@mail.gmail.com>
 <Z8dcGMMP3-D5dobY@nathan>
 <CACX+KaO4R9QDxbPSxSB0jNXFsqA6Jf=UPS+tyUvT_YvuP_grVA@mail.gmail.com>
 <Z8yxsm9ZWVkHlPbV@nathan>
 <CACX+KaP+6U9jf=GT4wpR7TvRvSMtTAhz=vP2Zr+ZdUFVZzqNsA@mail.gmail.com>
 <Z8y9RTT-vU6oVI_Y@nathan>
 <149429.1741472260@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <149429.1741472260@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Z8zwVmGzXyDdkAXj%40nathan>
Precedence: bulk

On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
> It bothers me a bit that this proposes to do something as complicated
> as pg_class_aclcheck on a table we have no lock on.  As you say, the
> lock we hold on the index would prevent DROP TABLE, but that doesn't
> mean we won't have any issues with other DDL on the table.  Still,
> taking a lock would be bad because of the deadlock hazard, and I
> think the potential for conflicts with concurrent DDL is nonzero in
> a lot of other places.  So I don't have any concrete reason to object.
> 
> ReindexIndex() faces this same problem and solves it with some
> very complex code that manages to get the table's lock first.
> But I see that it's also doing pg_class_aclcheck on a table
> it hasn't locked yet, so I don't think that adopting its approach
> would do anything useful for us here.

I noticed that amcheck's bt_index_check_internal() handles this problem,
and I think that approach could be adapted here:

	relkind = get_rel_relkind(relOid);
	if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
	{
		permOid = IndexGetRelation(relOid, true);
		if (OidIsValid(permOid))
			LockRelationOid(permOid, AccessShareLock);
		else
			fail = true;
	}
	else
		permOid = relOid;

	rel = relation_open(relOid, AccessShareLock);
	if (fail ||
		(permOid != relOid && permOid != IndexGetRelation(relOid, false)))
		ereport(ERROR,
				(errcode(ERRCODE_UNDEFINED_TABLE),
				 errmsg("could not find parent table of index \"%s\"",
						RelationGetRelationName(rel))));

	aclresult = pg_class_aclcheck(permOid, GetUserId(), ACL_SELECT);
	if (aclresult != ACLCHECK_OK)
		aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));

	if (permOid != relOid)
		UnlockRelationOid(permOid, AccessShareLock);

stats_lock_check_privileges() does something similar, but it's not as
cautious about the "heapid != IndexGetRelation(indrelid, false)" race
condition.  Maybe RangeVarCallbackForReindexIndex() should be smarter about
this, too.  That being said, this is a fair amount of complexity to handle
something that is in theory extremely rare...

-- 
nathan