From: Nikolay Shaplov <dhyan@nataraj.su>
To: =?ISO-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Minor rework of ALTER TABLE SET RelOptions code
Date: Tue, 18 Mar 2025 16:38:02 +0300
Message-ID: <5575197.29KlJPOoH8@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <202503140835.a7rr73tvaeic@alvherre.pgsql>
References: <202503140835.a7rr73tvaeic@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart4593741.Wku2Vz74k6";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: 
 <https://www.postgresql.org/message-id/5575197.29KlJPOoH8%40thinkpad-pgpro>
Precedence: bulk

--nextPart4593741.Wku2Vz74k6
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"; protected-headers="v1"
From: Nikolay Shaplov <dhyan@nataraj.su>
To: =?ISO-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Minor rework of ALTER TABLE SET RelOptions code
Date: Tue, 18 Mar 2025 16:38:02 +0300
Message-ID: <5575197.29KlJPOoH8@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <202503140835.a7rr73tvaeic@alvherre.pgsql>
References: <202503140835.a7rr73tvaeic@alvherre.pgsql>
MIME-Version: 1.0

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=BF=D1=8F=D1=82=
=D0=BD=D0=B8=D1=86=D0=B0, 14 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2025 =D0=B3. 11=
:35:15 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=
=BB=D1=8C =C3=81lvaro Herrera=20
=D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

> > 1. `isnull` variable is actually needed in a very narrow scope, so
> > it is better to keep it in that scope, not keeping it in mind in while
> > dealing with the rest of the code.
> >=20
> > 2. Toast table RelOptions are never altered directly with ALTER command.
> > One should do ATLER to a heap relation and use toast. reloption namespa=
ce
> > to address toast's reloption. If you get `ATExecSetRelOptions` called w=
ith
> > `RELKIND_TOASTVALUE` relation in the args, something is really wrong. We
> > should throw asserts, errors and whistle as loud as we can.
=20
> These are two separate things, I would have suggested to post them as
> two separate patches. =20
Sounds reasonable.=20

> And the second one lacked tests, so I added some.
It never come into my mind that someone can try something like

ALTER TABLE pg_toast.pg_toast_2615 SET (fillfactor =3D '90')

and succeed.
Thanks for doing that.

> I have pushed both now, thanks.
Thank you for commit!

> I can't unfortunately promise to review the larger reloption refactoring
> patch for 18.
This is understandable.=20
I will try to split this patch into parts for better readability in that ti=
me.

=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

--nextPart4593741.Wku2Vz74k6
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

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

iQEzBAABCgAdFiEE+sk3ebqQKlezKOi8PMbfuIHAGpgFAmfZdzoACgkQPMbfuIHA
GpgdZgf+I5cVOpHMlTHwup5V80mX5jS9WExI9xo11r7HtBZNiGA7kdZfLHbH8TNi
P2rb1BvnVCLlNJ7GBZfBirYEv8YQytp3ihlNI4l7UxnKgCqlTvOHbye8elnLeEI+
o2IPYZDjaW0EIEiy3s4qGwXAHk5fGPIGGMpAyR+krpk6/rXFhggF4tVmyCC2cKax
p5JOdHqyTeuG3TTiJ4C28bPRE4iUXaAB8mZ5pd2vJ1pvLtmBZtDognFrGvtslCJr
JlcZaNqfSBnDeu5+IiJnmC9TJ4FAUGlEO7+SOuC73+9dATIJkmmStnNsqPh4wFQr
IOZLwQIYf5C8QVn12wMFI5nFre2/Hw==
=IQqR
-----END PGP SIGNATURE-----

--nextPart4593741.Wku2Vz74k6--