Re: Clarification on Role Access Rights to Table Indexes

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