Horton

Re: Minor rework of ALTER TABLE SET RelOptions code

Timur Magomedov <t.magomedov@postgrespro.ru>

View thread
From: Timur Magomedov <t.magomedov@postgrespro.ru>
To: Nikolay Shaplov <dhyan@nataraj.su>, pgsql-hackers@lists.postgresql.org
Date: 2025-03-13T10:54:45Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove direct handling of reloptions for toast tables

  2. ATExecSetRelOptions: Reduce scope of 'isnull' variable

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