Re: Minor rework of ALTER TABLE SET RelOptions code

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