Re: [PATCH] get rid of StdRdOptions, use individual binaryreloptions representation for each relation kind instead - Mailing list pgsql-hackers

Hello.

At Mon, 18 Mar 2019 03:03:04 +0000, "Iwata, Aya" <iwata.aya@jp.fujitsu.com> wrote in
<71E660EB361DF14299875B198D4CE5423DF05777@g01jpexmbkw25>
> This patch does not apply.  Please refer to http://commitfest.cputube.org/ and update it.
> How about separating your patch by feature or units that can be phased commit.
> For example, adding assert macro at first, refactoring StdRdOptions by the next, etc.
> 
> So I change status to "Waiting for Author".

That seems to be a good oppotunity. I have some comments.

rel.h:
-#define RelationGetToastTupleTarget(relation, defaulttarg) \
-    ((relation)->rd_options ? \
-     ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : (defaulttarg))
+#define RelationGetToastTupleTarget(relation, defaulttarg)                 \
+    (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||        \
+                 relation->rd_rel->relkind == RELKIND_MATVIEW),            \
+     ((relation)->rd_options ?                                             \
+      ((HeapRelOptions *) (relation)->rd_options)->toast_tuple_target : \
+            (defaulttarg)))

Index AMs parse reloptions by their handler
functions. HeapRelOptions in this patch are parsed in
relation_reloptions calling heap_reloptions. Maybe at least it
should be called as table_options. But I'm not sure what is the
desirable shape of relation_reloptions for the moment.

hio.c:

-    saveFreeSpace = RelationGetTargetPageFreeSpace(relation,
-                                                   HEAP_DEFAULT_FILLFACTOR);
+    if (IsToastRelation(relation))
+        saveFreeSpace = ToastGetTargetPageFreeSpace();
+    else
+        saveFreeSpace = HeapGetTargetPageFreeSpace(relation);

This locution appears four times in the patch and that's the all
where the two macros appear. And it might not be good that
RelationGetBufferForTuple identifies a heap directly since I
suppose that currently tost is a part of heap. Thus it'd be
better that HeapGetTargetPageFreeSpace handles the toast case.
Similary, it doesn't looks good that RelationGetBufferForTuple
consults HeapGetTargretPageFreeSpace() directly. Perhaps it
should be called via TableGetTargetPageFreeSpace(). I'm not sure
what is the right shape of the relationship among a relation and
a table and other kinds of relation. extract_autovac_opts
penetrates through the modularity boundary of toast/heap in a
similar way.


plancat.c:
+    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;

rel.h:
#define RelationGetParallelWorkers(relation, defaultpw)                 \
    (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||        \
                 relation->rd_rel->relkind == RELKIND_MATVIEW),            \
     ((relation)->rd_options ?                                             \
      ((HeapRelOptions *) (relation)->rd_options)->parallel_workers :     \
            (defaultpw)))

These function/macros are doing the same check twice at a call.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center