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
