Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions - Mailing list pgsql-hackers

On Sun, Sep 13, 2020 at 9:34 PM Nikolay Shaplov <dhyan@nataraj.su> wrote:
>
> В письме от понедельник, 20 июля 2020 г. 18:36:44 MSK пользователь Georgios
> Kokolatos написал:
>
> Hi! Sorry for really long delay, I was at my summer vacations, and then has
> urgent things to finish first. :-( Now I hope we can continue...
>
>
> > thank you for the patch. It applies cleanly, compiles and passes check,
> > check-world.
>
> Thank you for reviewing efforts.
>
> > I feel as per the discussion, this is a step to the right direction yet it
> > does not get far enough. From experience, I can confirm that dealing with
> > reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions
> > should be handled by the table AM specific code. The current patch does not
> > address the issue. Yet it does make the issue easier to address by clearing
> > up the current state.
>
> Moving reloptions to AM code is the goal I am slowly moving to. I've started
> some time ago with big patch https://commitfest.postgresql.org/14/992/ and
> have been told to split it into smaller parts. So I did, and this patch is
> last step that cleans options related things up, and then actual moving can be
> done.
>
> > If you allow me, I have a couple of comments.
> >
> > -     saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
> > -
HEAP_DEFAULT_FILLFACTOR);
> > +     if (IsToastRelation(relation))
> > +             saveFreeSpace = ToastGetTargetPageFreeSpace();
> > +     else
> > +             saveFreeSpace = HeapGetTargetPageFreeSpace(relation);
> >
> > For balance, it does make some sense for ToastGetTargetPageFreeSpace() to
> > get relation as an argument, similarly to HeapGetTargetPageFreeSpace().
>
> ToastGetTargetPageFreeSpace return a const value. I've change the code, so it
> gets relation argument, that is not used, the way you suggested. But I am not
> sure if it is good or bad idea. May be we will get some "Unused variable"
> warning on some compilers. I like consistency... But not sure we need it here.
>
> > -       /* Retrieve the parallel_workers reloption, or -1 if not set. */
> > -       rel->rel_parallel_workers = RelationGetParallelWorkers(relation,
> > -1);
>  +       /*
> > +        * Retrieve the parallel_workers for heap and mat.view relations.
> > +        * Use -1 if not set, or if we are dealing with other relation
> > kinds
>  +        */
> > +       if (relation->rd_rel->relkind == RELKIND_RELATION ||
> > +               relation->rd_rel->relkind == RELKIND_MATVIEW)
> > +               rel->rel_parallel_workers =
> > RelationGetParallelWorkers(relation, -1);
>  +       else
> > +               rel->rel_parallel_workers = -1;
> > Also, this pattern is repeated in four places, maybe the branch can be
> > moved inside a macro or static inline instead?
>
> > If the comment above is agreed upon, then it makes a bit of sense to apply
> > the same here. The expression in the branch is already asserted for in
> > macro, why not switch there and remove the responsibility from the caller?
>
> I guess here you are right, because here the logic is following: for heap
> relation take option from options, for _all_ others use -1. This can be moved
> to macro.
>
> So I changed it to
>
> /*
>  * HeapGetParallelWorkers
>  *      Returns the heap's parallel_workers reloption setting.
>  *      Note multiple eval of argument!
>  */
> #define HeapGetParallelWorkers(relation, defaultpw)                     \
>     (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||       \
>                  relation->rd_rel->relkind == RELKIND_MATVIEW),         \
>      (relation)->rd_options ?                                           \
>       ((HeapOptions *) (relation)->rd_options)->parallel_workers :      \
>             (defaultpw))
>
> /*
>  * RelationGetParallelWorkers
>  *      Returns the relation's parallel_workers reloption setting.
>  *      Note multiple eval of argument!
>  */
>
> #define RelationGetParallelWorkers(relation, defaultpw)                 \
>     (((relation)->rd_rel->relkind == RELKIND_RELATION ||                \
>       (relation)->rd_rel->relkind == RELKIND_MATVIEW) ?                 \
>       HeapGetParallelWorkers(relation, defaultpw) : defaultpw)
>
>
> But I would not like to move
>
>         if (IsToastRelation(relation))
>                 saveFreeSpace = ToastGetTargetPageFreeSpace(relation);
>         else
>                 saveFreeSpace = HeapGetTargetPageFreeSpace(relation);
>
> into macros, as there is a choice only between heap and toast. All other
> relation types are not mentioned.
>
> So we can not call it RelationGetTargetPageFreeSpace. It would be
> ToastOrHeapGetTargetPageFreeSpace actually. Better not to have such macro.
>
> Please find new version of the patch in the attachment.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh