On 2022-Jun-29, Simon Riggs wrote:
> > - if (strcmp(objectName, get_database_name(objectOid)) != 0)
> > + if (objectName && strcmp(objectName, get_database_name(objectOid)) != 0)
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > errmsg("can only reindex the currently open database")));
> > if (!pg_database_ownercheck(objectOid, GetUserId()))
> > aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
> > - objectName);
> > + get_database_name(objectOid));
> >
> > This could call get_database_name() just once.
>
> It could, but I couldn't see any benefit in changing that for the code
> under discussion.
>
> If calling get_database_name() multiple times is an issue, I've added
> a cache for that - another patch attached, if you think its worth it.
TBH I doubt that this is an issue: since we're throwing an error anyway,
the memory would be released, and error cases are not considered worth
of performance optimization anyway.
Putting that thought aside, if we were to think that this is an issue, I
don't think the cache as implemented here is a good idea, because then
caller is responsible for tracking whether to free or not the return
value.
I think that Michaël's idea could be implemented more easily by having a
local variable that receives the return value from get_database_name.
But I think the coding as Simon had it was all right.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes" (Germán Poo)
