pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedMinor rework of ALTER TABLE SET RelOptions code
4+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2025-03-14 1548c3a304 Remove direct handling of reloptions for toast tables
2025-03-13 c7fc8808a9 ATExecSetRelOptions: Reduce scope of 'isnull' variable
* Minor rework of ALTER TABLE SET RelOptions code
@ 2025-03-07 17:02 Nikolay Shaplov <dhyan@nataraj.su>
0 siblings, 2 replies; 4+ messages in thread
From: Nikolay Shaplov @ 2025-03-07 17:02 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
Attachments:
v-1-0001-Minor-rework-of-ALTER-TABLE-SET-RelOptions-code.patch (text/x-patch, 3.4K) download
From 5618baa7a4efd075d402444aedc31437c7652afa Mon Sep 17 00:00:00 2001
From: Nikolay Shaplov <dhyan@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
* Re: Minor rework of ALTER TABLE SET RelOptions code
@ 2025-03-13 10:54 Timur Magomedov <t.magomedov@postgrespro.ru>
parent: Nikolay Shaplov <dhyan@nataraj.su>
1 sibling, 0 replies; 4+ messages in thread
From: Timur Magomedov @ 2025-03-13 10:54 UTC (permalink / raw)
To: Nikolay Shaplov <dhyan@nataraj.su>; pgsql-hackers@lists.postgresql.org
On Fri, 2025-03-07 at 20:02 +0300, Nikolay Shaplov wrote:
> 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:
Thanks for the patch!
> 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.
Since we already have datum=0 logic there, there is indeed no need to
have isnull variable on same scope.
>
> 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.
Seems reasonable to me. Toast tables are handled later by
"heap_reloptions(RELKIND_TOASTVALUE, ..." in same ATExecSetRelOptions()
function code.
LGTM
-------
Regards,
Timur Magomedov
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Minor rework of ALTER TABLE SET RelOptions code
@ 2025-03-14 08:35 Álvaro Herrera <alvherre@alvh.no-ip.org>
parent: Nikolay Shaplov <dhyan@nataraj.su>
1 sibling, 1 reply; 4+ messages in thread
From: Álvaro Herrera @ 2025-03-14 08:35 UTC (permalink / raw)
To: Nikolay Shaplov <dhyan@nataraj.su>; +Cc: pgsql-hackers@lists.postgresql.org
On 2025-Mar-07, Nikolay Shaplov wrote:
> 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.
These are two separate things, I would have suggested to post them as
two separate patches. And the second one lacked tests, so I added some.
I have pushed both now, thanks.
I can't unfortunately promise to review the larger reloption refactoring
patch for 18.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming" (A. Stepanov)
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Minor rework of ALTER TABLE SET RelOptions code
@ 2025-03-18 13:38 Nikolay Shaplov <dhyan@nataraj.su>
parent: Álvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 4+ messages in thread
From: Nikolay Shaplov @ 2025-03-18 13:38 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-hackers@lists.postgresql.org
В письме от пятница, 14 марта 2025 г. 11:35:15 MSK пользователь Álvaro Herrera
написал:
> > 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.
> These are two separate things, I would have suggested to post them as
> two separate patches.
Sounds reasonable.
> 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 = '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.
I will try to split this patch into parts for better readability in that time.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
signature.asc (application/pgp-signature, 487B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-03-18 13:38 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-03-07 17:02 Minor rework of ALTER TABLE SET RelOptions code Nikolay Shaplov <dhyan@nataraj.su>
2025-03-13 10:54 ` Timur Magomedov <t.magomedov@postgrespro.ru>
2025-03-14 08:35 ` Álvaro Herrera <alvherre@alvh.no-ip.org>
2025-03-18 13:38 ` Nikolay Shaplov <dhyan@nataraj.su>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox