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

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi,

thank you for the patch. It applies cleanly, compiles and passes check, check-world.

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.

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().
Also, this pattern is repeated in four places, maybe the branch can be moved inside a macro or static inline instead?

-       /* 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;

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?

Any thoughts on the above?

Cheers,
Georgios

The new status of this patch is: Waiting on Author