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

From: Georgios Kokolatos <gkokolatos(at)protonmail(dot)com>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
Subject: Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
Date: 2020-07-20 15:36:44
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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


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,
+ 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?


The new status of this patch is: Waiting on Author

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-07-20 15:51:19 Re: Binary support for pgoutput plugin
Previous Message Lawrence Jones 2020-07-20 15:21:55 Postgres-native method to identify if a tuple is frozen