Content-Type: multipart/mixed; boundary="------------kn6NUTFT4hiMVeoTab2jeqct"
Message-ID: <20eea594-a05b-4c31-491b-007b6fceef28@iki.fi>
Date: Thu, 29 Jun 2023 10:06:35 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: Add more sanity checks around callers of changeDependencyFor()
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <ZJzD/rn+UbloKjB7@paquier.xyz>
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <ZJzD/rn+UbloKjB7@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/20eea594-a05b-4c31-491b-007b6fceef28%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------kn6NUTFT4hiMVeoTab2jeqct
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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)

--------------kn6NUTFT4hiMVeoTab2jeqct
Content-Type: text/x-patch; charset=UTF-8;
 name="fail-to-change-schema-dep-test.patch"
Content-Disposition: attachment;
 filename="fail-to-change-schema-dep-test.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9leHRlbnNpb25zL3NxbC90ZXN0
X2V4dGVuc2lvbnMuc3FsIGIvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X2V4dGVuc2lvbnMvc3Fs
L3Rlc3RfZXh0ZW5zaW9ucy5zcWwKaW5kZXggZjQ5NDdlN2RhNi4uYjZiOGQ5ZDMxZiAxMDA2
NDQKLS0tIGEvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X2V4dGVuc2lvbnMvc3FsL3Rlc3RfZXh0
ZW5zaW9ucy5zcWwKKysrIGIvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X2V4dGVuc2lvbnMvc3Fs
L3Rlc3RfZXh0ZW5zaW9ucy5zcWwKQEAgLTIxMCw2ICsyMTAsMTcgQEAgQUxURVIgRVhURU5T
SU9OIHRlc3RfZXh0X2NpbmUgVVBEQVRFIFRPICcxLjEnOwogCiBcZHgrIHRlc3RfZXh0X2Np
bmUKIAorCistLQorQ1JFQVRFIFNDSEVNQSB0ZXN0X2Z1bmNfZGVwMTsKK0NSRUFURSBTQ0hF
TUEgdGVzdF9mdW5jX2RlcDI7CitDUkVBVEUgRVhURU5TSU9OIHRlc3RfZXh0X3JlcV9zY2hl
bWExIFNDSEVNQSB0ZXN0X2Z1bmNfZGVwMTsKK0FMVEVSIEZVTkNUSU9OIHRlc3RfZnVuY19k
ZXAxLmRlcF9yZXExKCkgU0VUIFNDSEVNQSB0ZXN0X2Z1bmNfZGVwMjsKKworQUxURVIgRVhU
RU5TSU9OIHRlc3RfZXh0X3JlcV9zY2hlbWExIFNFVCBTQ0hFTUEgdGVzdF9mdW5jX2RlcDI7
CisKK0RST1AgRVhURU5TSU9OIHRlc3RfZXh0X3JlcV9zY2hlbWExIENBU0NBREU7CisKIC0t
CiAtLSBUZXN0IEBleHRzY2hlbWE6ZXh0bmFtZUAgc3ludGF4IGFuZCBub19yZWxvY2F0ZSBv
cHRpb24KIC0tCg==

--------------kn6NUTFT4hiMVeoTab2jeqct--