public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Michael Paquier <[email protected]>
To: Postgres hackers <[email protected]>
Subject: Re: Add more sanity checks around callers of changeDependencyFor()
Date: Thu, 29 Jun 2023 10:06:35 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZJzD/[email protected]>
References: <ZJzD/[email protected]>

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)


Attachments:

  [text/x-patch] fail-to-change-schema-dep-test.patch (766B, ../[email protected]/2-fail-to-change-schema-dep-test.patch)
  download | inline diff:
diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql
index f4947e7da6..b6b8d9d31f 100644
--- a/src/test/modules/test_extensions/sql/test_extensions.sql
+++ b/src/test/modules/test_extensions/sql/test_extensions.sql
@@ -210,6 +210,17 @@ ALTER EXTENSION test_ext_cine UPDATE TO '1.1';
 
 \dx+ test_ext_cine
 
+
+--
+CREATE SCHEMA test_func_dep1;
+CREATE SCHEMA test_func_dep2;
+CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1;
+ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2;
+
+ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep2;
+
+DROP EXTENSION test_ext_req_schema1 CASCADE;
+
 --
 -- Test @extschema:extname@ syntax and no_relocate option
 --

view thread (3+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Add more sanity checks around callers of changeDependencyFor()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox