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 |
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 |