On Sun, Sep 13, 2020 at 9:34 PM Nikolay Shaplov <dh...@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
