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

Hi Nikolay,

thanks for sending a new version of the patch. I've done a basic review
today, so let me share some comments about the patch.

Firstly, there's an important question why should we actually do this.

At the beginning of this thread you mentioned memory usage - e.g. for
indexes the reduced struct is just 8B (instead of 82B). I doubt it's
worth doing for this reason - it's a tiny amount of memory (you'd need
~16k indexes to save 1MB). So memory consumption does not seem like a
pressing issue (e.g. we're probably wasting way more memory thanks to
AllocSet using the 2^N bins to allocate chunks). I see Andres already
voiced a similar opinion last year ....

You then mentioned that

  > My real motivation for this patch is to make code more uniform.
  > So the patch over it will be much clearer. And to make result
  > code more clear and uniform too.

which seems like a much better reason. The thing is - I'm not quite
convinced this patch makes the code more uniform/clearer. While that's
somewhat subjective opinion, there are cases where the code/API gets
obviously better - and this does not seem like one of those cases :-(

The one remaining possible benefit of this patch is making the code
easier to reuse / extend from other patches. In fact, that's why I'm
looking at this patch, because in the "opclass parameters" thread it was
repeatedly mentioned this patch would be useful.

So I think it'd be good to coordinate with Nikita Glukhov, and rebase
the opclass parameters patch on top of this one to verify/demonstrate
how it benefits that patch.



Now, some comments about the patch itself:


1) I see there's a bunch of functions parsing reloptions with about this
pattern:

    bytea *
    btoptions(Datum reloptions, bool validate)
    {
        static const relopt_parse_elt tab[] = { ... }
    
        options = parseRelOptions(
    
        /* if none set, we're done */
        if (numoptions == 0)
            return NULL;
    
        rdopts = allocateReloptStruct(...)
    
        fillRelOptions(rdopts, ...);
    
        pfree(options);
    
        return (bytea *) rdopts;
    }

so I wonder if the patch might define a wrapper doing all of this,
instead of copying it on a number of places.


2) The patch makes various new places aware about how reloptions for
different relkinds are parsed differently. IMHO that's a bad thing,
because it essentially violates layering / levels of abstraction.

For example, there's this change in heap_multi_insert():

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

so a code which was entirely oblivious to TOAST vs. non-TOAST relations
suddenly cares about this difference. And there's no good reason for
that, because this if might be added to the original macro, eliminating
this change entirely. And this exact same change in on various other
places.

The same thing applies to this change in get_relation_info:

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

and this vacuum_rel chunk is not particularly readable either:

    if (onerel->rd_options == NULL ||
-       ((StdRdOptions *) onerel->rd_options)->vacuum_index_cleanup)
+       (!IsToastRelation(onerel) &&
+        ((HeapRelOptions *) onerel->rd_options)->vacuum_index_cleanup) ||
+       (IsToastRelation(onerel) &&
+        ((ToastRelOptions *) onerel->rd_options)->vacuum_index_cleanup))

(To be fair, this already was looking directly at StdRdOptions, but
adding a new macro to get vacuum_index_cleanup would be a good idea
anyway).


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services