Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 15 May 2024 10:14:09 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Avoid orphaned objects dependencies, take 3
Message-ID: <ZkQMYVxbOxFD0519@paquier.xyz>
References: <a0e850b9-31c6-86cd-351d-6b8a7a1d6bf7@gmail.com>
 <ZidAHRJyMzPZ5JtJ@ip-10-97-1-34.eu-west-3.compute.internal>
 <Ziff3vGRKv1LUjY7@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZijE6UBCo02U2TXJ@ip-10-97-1-34.eu-west-3.compute.internal>
 <62310067-6dc4-97ab-6724-7d3602b5056d@gmail.com>
 <ZinjZmaMEEkOSfZ0@ip-10-97-1-34.eu-west-3.compute.internal>
 <403075e1-4d60-09e2-a06a-749cd9d88f66@gmail.com>
 <ZioEJafHC95wjfMk@ip-10-97-1-34.eu-west-3.compute.internal>
 <d815344e-a6a2-617a-d35f-012e5e89f29f@gmail.com>
 <Zjy/o3t/26exwj5Z@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4r2nQsEFhQdabiBd"
Content-Disposition: inline
In-Reply-To: <Zjy/o3t/26exwj5Z@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: <https://www.postgresql.org/message-id/ZkQMYVxbOxFD0519%40paquier.xyz>
Precedence: bulk


--4r2nQsEFhQdabiBd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, May 09, 2024 at 12:20:51PM +0000, Bertrand Drouvot wrote:
> Oh I see, your test updates an existing dependency. v4 took care about br=
and new=20
> dependencies creation (recordMultipleDependencies()) but forgot to take c=
are
> about changing an existing dependency (which is done in another code path:
> changeDependencyFor()).
>=20
> Please find attached v5 that adds:
>=20
> - a call to the new depLockAndCheckObject() function in changeDependencyF=
or().
> - a test when altering an existing dependency.
>=20
> With v5 applied, I don't see the issue anymore.

+            if (object_description)
+                ereport(ERROR, errmsg("%s does not exist", object_descript=
ion));
+            else
+                ereport(ERROR, errmsg("a dependent object does not ex

This generates an internal error code.  Is that intended?

--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_lock=
s.spec=20

This introduces a module with only one single spec.  I could get
behind an extra module if splitting the tests into more specs makes
sense or if there is a restriction on installcheck.  However, for=20
one spec file filed with a bunch of objects, and note that I'm OK to
let this single spec grow more for this range of tests, it seems to me
that this would be better under src/test/isolation/.

+		if (use_dirty_snapshot)
+		{
+			InitDirtySnapshot(DirtySnapshot);
+			snapshot =3D &DirtySnapshot;
+		}
+		else
+			snapshot =3D NULL;

I'm wondering if Robert has a comment about that.  It looks backwards
in a world where we try to encourage MVCC snapshots for catalog
scans (aka 568d4138c646), especially for the part related to
dependency.c and ObjectAddresses.
--
Michael

--4r2nQsEFhQdabiBd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmZEDGEACgkQnvQgOdby
QH19URAAkn7ldXAbhI2N0VAC1Xy04k+C1OSBMXKTKcJFkFv26V/qqRbPIZEuF5gb
R5BiHqbZAPitj69W+ikWlW9aJZUt93+nBq0PIX5a1N9rZy4BiQZzL26Afg1ktXwt
CfbVmvyvcNyKBD8gxtISGcTxFf1wF0XXGS1Hug9DMtJLUMNkGdtKvb/QDHkVw0Op
cM9MdF5MF3+yseRnvq3x+ZjOzzbD791OYr8w0GdqNXvI4qR1bWvTVUgfh2h+7P8R
nCVMfYZ/zUV0E4WKPmDcfXDMPvw1NY62AyjOeN88OfcRixMwLfxnctmnOTgJBcfI
uX0GnnfmHCkLxExe/bXoPegQIeTcUOlK7cph1Y8VQ20d+iWLfFiY31cC4Lgr82P0
46uD8NFoRmUmq6Wt0k9NLC9kjGfHSN6iQKyLOtXlBcLOxFAMzBMloi+BetNCijGJ
rnMyyaA9dgQommEjbdZaZHjFMFL8MhBmc/HHoZye0pG2AipKQOHTWmkJc87LEkjy
veEEn9scxmutF5Dj1cbLehOFFqUzs0IxBIYt9kQFNOctpz6qOyFtlB1fvNzAEepX
VWYyiZVMht3elfZpSCF7f5uh62HRGdZz6OCns0EG6jFxMLYwDoVelRB9gVakBM1l
hnrixUAc7eDpb5pTnBolKJB95+SROj8t0kfLEdxod9S+4nbe9dE=
=mtpW
-----END PGP SIGNATURE-----

--4r2nQsEFhQdabiBd--