While working with my New Options Engine patch https://commitfest.postgresql.org/patch/4688/ I found out that I can detach a small portion of it as a separate patch. It has own value, even if big patch is never committed, and it would make smoother further committing of big patch if we ever get to it.
Patch description is following:
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.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
>From 5618baa7a4efd075d402444aedc31437c7652afa Mon Sep 17 00:00:00 2001
From: Nikolay Shaplov <dh...@nataraj.su>
Date: Fri, 7 Mar 2025 19:44:14 +0300
Subject: [PATCH v[1] 1/1] Minor rework of ALTER TABLE SET RelOptions code
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
signature.asc
Description: This is a digitally signed message part.
