Re: Add more sanity checks around callers of changeDependencyFor()

On 29/06/2023 02:36, Michael Paquier wrote:
> Hi all,
> 
> While working on a different patch, I have noted three code paths that
> call changeDependencyFor() but don't check that they do not return
> errors.  In all the three cases (support function, extension/schema
> and object/schema), it seems to me that only one dependency update is
> expected.

Makes sense.

>     /* update dependencies to point to the new schema */

Suggest: "update dependency ..." in singular, as there should be only one.

>     if (changeDependencyFor(ExtensionRelationId, extensionOid,
>                             NamespaceRelationId, oldNspOid, nspOid) != 1)
>         elog(ERROR, "failed to change schema dependency for extension %s",
>              NameStr(extForm->extname));

The error messages like "failed to change schema dependency for 
extension" don't conform to the usual error message style. "could not 
change schema dependency for extension" would be more conformant. I see 
that you copy-pasted that from existing messages, and we have a bunch of 
other "failed to" messages in the repository too, so I'm OK with leaving 
it as it is for now. Or maybe change the wording of all the 
changeDependencyFor() callers now, and consider all the other "failed 
to" messages separately later.

If changeDependencyFor() returns >= 2, the message is a bit misleading. 
That's what the existing callers did too, so maybe that's fine.

I can hit the above error with the attached test case. That seems wrong, 
although I don't know if it means that the check is wrong or it exposed 
a long-standing bug.

-- 
Heikki Linnakangas
Neon (https://neon.tech)