Thread

[PATCH v[1] 1/1] Minor rework of ALTER TABLE SET RelOptions code

Nikolay Shaplov <dhyan@nataraj.su> — 2025-03-07T16:44:14Z

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.

2. Toast table RelOptions are never altered directly with ALTER command.
One should do ATLER to a heap relation and use toast. reloption namespace
to address toast's reloption. If you get `ATExecSetRelOptions` called with
`RELKIND_TOASTVALUE` relation in the args, something is really wrong. We
should throw asserts, errors and whistle as loud as we can.
---
 src/backend/commands/tablecmds.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 59156a1c1f..a0f78f036b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15804,7 +15804,6 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 	HeapTuple	tuple;
 	HeapTuple	newtuple;
 	Datum		datum;
-	bool		isnull;
 	Datum		newOptions;
 	Datum		repl_val[Natts_pg_class];
 	bool		repl_null[Natts_pg_class];
@@ -15829,25 +15828,25 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		 * there were none before.
 		 */
 		datum = (Datum) 0;
-		isnull = true;
 	}
 	else
 	{
+		bool		isnull;
 		/* Get the old reloptions */
 		datum = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_reloptions,
 								&isnull);
+		if (isnull)
+			datum = (Datum) 0;
 	}
 
 	/* Generate new proposed reloptions (text array) */
-	newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
-									 defList, NULL, validnsps, false,
+	newOptions = transformRelOptions(datum, defList, NULL, validnsps, false,
 									 operation == AT_ResetRelOptions);
 
 	/* Validate */
 	switch (rel->rd_rel->relkind)
 	{
 		case RELKIND_RELATION:
-		case RELKIND_TOASTVALUE:
 		case RELKIND_MATVIEW:
 			(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
 			break;
@@ -15861,6 +15860,12 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		case RELKIND_PARTITIONED_INDEX:
 			(void) index_reloptions(rel->rd_indam->amoptions, newOptions, true);
 			break;
+		case RELKIND_TOASTVALUE:
+			/* Should never get here */
+			/* TOAST options are never altered directly */
+			Assert(0);
+			/* FALLTHRU */
+			/* If we get here in prod. error is the best option */
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
@@ -15950,18 +15955,19 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 			 * pretend there were none before.
 			 */
 			datum = (Datum) 0;
-			isnull = true;
 		}
 		else
 		{
+			bool		isnull;
 			/* Get the old reloptions */
 			datum = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_reloptions,
 									&isnull);
+			if (isnull)
+				datum = (Datum) 0;
 		}
 
-		newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
-										 defList, "toast", validnsps, false,
-										 operation == AT_ResetRelOptions);
+		newOptions = transformRelOptions(datum, defList, "toast", validnsps,
+										false, operation == AT_ResetRelOptions);
 
 		(void) heap_reloptions(RELKIND_TOASTVALUE, newOptions, true);
 
-- 
2.39.2


--nextPart2215029.irdbgypaU6--

--nextPart13713915.uLZWGnKmhe
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+sk3ebqQKlezKOi8PMbfuIHAGpgFAmfLJpAACgkQPMbfuIHA
GpjXJwgAn1piaU1+C9j8iF670iXa6P9fJ/SpV3THh6jcglTDXwUpELwr5IV7xdDf
TN8llIT2YSrjmLwyhZ6BJ+Me3/PL+QS2mOuknbxWkT/dLIERFSnf9pv/prQCU6ws
DZJu2+XnQVJoR0nKyRKmd2dcIb7zdHRt3+acKRU5iU6EclQrlgPC0KgjFcaBuHuv
CVqEivMjOmoV3cYCV0Ntj4Lae42fRIGEbUDRpZ6y2HVArFOgPJgRCTxkxvC4eGo+
J5c+lzlpRhJwDq/WOYjuxof+HvJUmiHsfZlVQQqeUoq84JfLHmGUUP2BG5PP0dPo
ffy9gkF8aWrdnb3TwBEdxspjmUUnMA==
=51hh
-----END PGP SIGNATURE-----

--nextPart13713915.uLZWGnKmhe--