[PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Subject: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
Date: 2018-02-22 16:48:46
Message-ID: 2083183.Rn7qOxG4Ov@x200m
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


This is part or my bigger patch https://www.postgresql.org/message-id/flat/2146419(dot)veIEZdk4E4(at)x200m#2146419(dot)veIEZdk4E4@x200m we've decided to
commit by smaller parts.

Now in postgres an StdRdOptions structure is used as binary represenations of
reloptions for heap, toast, and some indexes. It has a number of fields, and
only heap relation uses them all. Toast relations uses only autovacuum
options, and indexes uses only fillfactor option.

So for example if you set custom fillfactor value for some index, then it will
lead to allocating 84 bytes of memory (sizeof StdRdOptions on i386) and only 8
bytes will be actually used (varlena header and fillfactor value). 74 bytes is
not much, but allocating them for each index for no particular reason is bad
idea, as I think.

Moreover when I wrote my big reloption refactoring patch, I came to "one
reloption kind - one binary representation" philosophy. It allows to write
code with more logic in it.

This patch replaces StdRdOptions with HeapRelOptions, ToastRelOptions,
BTRelOptions, HashRelOptions, SpGistRelOptions and PartitionedRelOptions

one for each relation kind that were using StdRdOptions before.

The second thing I've done, I've renamed Relation* macroses from
src/include/utils/rel.h, that were working with reloptions. I've renamed them
into Heap*, Toast* and View* (depend on what relation options they were
actually using)

I did it because there names were misleading. For example
RelationHasCheckOption can be called only for View relation, and will give
wrong result for other relation types. It just takes binary representation of
reloptions, cast is to (ViewOptions *) and then returns some value from it.
Naming it as ViewHasCheckOption would better reflect what it actually do, and
strictly specify that it is applicable only to View relations.

Possible flaws:

I replaced

saveFreeSpace = RelationGetTargetPageFreeSpace(state->rs_new_rel,
HEAP_DEFAULT_FILLFACTOR);

with

if (IsToastRelation(state->rs_new_rel))
saveFreeSpace = ToastGetTargetPageFreeSpace();
else
saveFreeSpace = HeapGetTargetPageFreeSpace(state->rs_new_rel);

wherever I met it (and other cases like that), but I am not sure if in some
cases that part of code is used for heap only or not. So may be this "ifs" is
not needed and should be removed, and only Heap-case should be left. But I am
not that much familiar with postgres internals to see it for sure... I need
advice of more experienced developers here.

--
Do code for fun.

Attachment Content-Type Size
get-rid-of-StrRdOptions.diff text/x-patch 35.7 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2018-02-22 16:53:15 Re: non-bulk inserts and tuple routing
Previous Message Robert Haas 2018-02-22 16:10:57 Re: non-bulk inserts and tuple routing