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

В письме от понедельник, 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.

--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing  (Russian)