Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions

On Sun, Sep 13, 2020 at 9:34 PM Nikolay Shaplov <dhyan(at)nataraj(dot)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