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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
43 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov

This is part or my bigger patch https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.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.

get-rid-of-StrRdOptions.diff (36K) Download Attachment
signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

On 2018-02-22 19:48:46 +0300, Nikolay Shaplov wrote:
> This is part or my bigger patch https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m we've decided to
> commit by smaller parts.

I've not read that thread. Is this supposed to be a first step towards
something larger?


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

I'm not sure this is a particularly strong motivator though?  Does the
patch have a goal besides saving a few bytes?


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

I have no idea what this means?


Are you aiming this for v11 or v12?


Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от 1 марта 2018 16:15:32 пользователь Andres Freund написал:

> > This is part or my bigger patch
> > https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#21464
> > 19.veIEZdk4E4@x200m we've decided to commit by smaller parts.
>
> I've not read that thread. Is this supposed to be a first step towards
> something larger?

I've started from the idea of opclass options. Same as here
https://commitfest.postgresql.org/17/1559/
And found out that current reloptions code is not flexible at all, and could
not be easily reused for other kind of options (that are not reloptions).

So I tried to change reloptions code to make universal, and then use it for
any option purposes.

In the patch https://commitfest.postgresql.org/14/992/ I created a new file
options.c, that works with options as with an abstraction. It has some option
definition structure, that has all information about how this options should
be parsed and transformed into binary representation, I called this structure
OptionsCatalog, but name can be changed. And it also have all kind of
functions that do all needed tranformation with options.

Then reloptions.c uses functions from options options.c for all options
transformations, and takes care about relation specificity of reloptions.

While doing all of these, I first of all had to adjust code that was written
around reloptions, so that there code would stay in tune with new reloptions
code. And second, I met places where current reloption related code were
imperfect, and I decided to change it too...

Since I get a really big patch as a result, it was decided to commit it in
parts.

This patch is the adjusting of reloption related code. It does not change a
great deal, but when you first commit it, the main reloptions refactoring
patch will no longer look like a big mess, it will became much more logical.

Enum options patch is more about making coder a little bit more perfect. It is
not really needed for the main patch, but it is more easy to introduce it
before, then after.
https://commitfest.postgresql.org/17/1489/

There is one more patch, that deals with toast.* options for tables with no
TOAST, but we can set it aside, it is independent enough, to deal with it
later...
https://commitfest.postgresql.org/17/1486/

The patch with reloptions tests I've written while working on this patch, were
already commited
https://commitfest.postgresql.org/15/1314/


> > 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.
> I'm not sure this is a particularly strong motivator though?  Does the
> patch have a goal besides saving a few bytes?

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.

I guess long time ago, the StdRdOptions were the way to make code uniform.
There were only one binary representation of options, and all relation were
using it. Quite uniform.

But now, some relation kinds uses it's own options binary representations.
Some still uses StdRdOptions, but only some elements from it. It is not
uniform enough for me.

If start using individual binary representation for each relation kind, then
code will be uniform and homogeneous again, and my next patch over it will be
more easy to read and understand.

> > 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.
> I have no idea what this means?
I hope I managed to explain it above... May be I am not good enough with
explaining, or with English. Or with explaining in English. If it is not clear
enough, please tell me, what points are not clear, I'll try to explain more...


> Are you aiming this for v11 or v12?
I hope to commit StdRdOptions and Enum_Options patches before v11.
Hope it does not go against any rules, since there patches does not change any
big functionality, just do some refactoring and optimization, of the code that
already exists.

As for main reloptions refactoring patch, if there is a chance to get commited
before v11 release, it would be great. If not, I hope to post it to
commitfest, right after v11 were released, so it would be possible to commit
opclass options upon it before v12 release.


--
Do code for fun.

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

On 2018-03-02 20:22:21 +0300, Nikolay Shaplov wrote:
> Since I get a really big patch as a result, it was decided to commit it in
> parts.

I get that, but I strongly suggest not creating 10 loosely related
threads, but keeping it as a patch series in one thread. It's really
hard to follow for people not continually paying otherwise.  Having to
search the list, collect together multiple threads, trying to read them
in some chronological order... That's not a reasonably efficient way to
interact, therefore the likelihood of timely review will be reduced.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от 2 марта 2018 11:27:49 пользователь Andres Freund написал:

> > Since I get a really big patch as a result, it was decided to commit it in
> > parts.
>
> I get that, but I strongly suggest not creating 10 loosely related
> threads, but keeping it as a patch series in one thread. It's really
> hard to follow for people not continually paying otherwise.

Oups. Sorry I thought I should do other way round and create a new thread for
new patch. And tried to post a link to other threads for connectivity wherever
I can.

Will do it as you say from now on.

But I am still confused what should I do if I am commiting two patch from the
patch series in simultaneously...

--
Do code for fun.

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
Hi!

I've rebased the patch against recent master.

I've imported changes from 857f9c36 commit.

BTW this commit shows why do this patch is important: 857f9c36 adds new option
for b-tree indexes. But thanks to the StdRdOptions this option will exist for
no practical use in all heaps that has just any option set to non-default
value, and in indexes that use StdRdOptions (and also has any option set)
And there will be more. StdRdOptions is long outdated solution and it needs to
be replaced.


--
Do code for fun.

get-rid-of-StrRdOptions_1a.diff (37K) Download Attachment
signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
On Fri, Sep 14, 2018 at 09:30:25PM +0300, Nikolay Shaplov wrote:
> BTW this commit shows why do this patch is important: 857f9c36 adds new option
> for b-tree indexes. But thanks to the StdRdOptions this option will exist for
> no practical use in all heaps that has just any option set to non-default
> value, and in indexes that use StdRdOptions (and also has any option set)
> And there will be more. StdRdOptions is long outdated solution and it needs to
> be replaced.

This patch does not apply anymore, so this is moved to next CF, waiting
for author.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от 2 октября 2018 13:46:13 пользователь Michael Paquier написал:

> On Fri, Sep 14, 2018 at 09:30:25PM +0300, Nikolay Shaplov wrote:
> > BTW this commit shows why do this patch is important: 857f9c36 adds new
> > option for b-tree indexes. But thanks to the StdRdOptions this option
> > will exist for no practical use in all heaps that has just any option set
> > to non-default value, and in indexes that use StdRdOptions (and also has
> > any option set) And there will be more. StdRdOptions is long outdated
> > solution and it needs to be replaced.
>
> This patch does not apply anymore, so this is moved to next CF, waiting
> for author.
Oup...It seems to me that I've fogot to rebase it when it is needed...
Did it now



--
Do code for fun.

get-rid-of-StrRdOptions_1a.diff (37K) Download Attachment
signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On Mon, Nov 19, 2018 at 2:30 PM Nikolay Shaplov <[hidden email]> wrote:
>
> В письме от 2 октября 2018 13:46:13 пользователь Michael Paquier написал:
> > On Fri, Sep 14, 2018 at 09:30:25PM +0300, Nikolay Shaplov wrote:
> > > BTW this commit shows why do this patch is important: 857f9c36 adds new
> > > option for b-tree indexes. But thanks to the StdRdOptions this option
> > > will exist for no practical use in all heaps that has just any option set
> > > to non-default value, and in indexes that use StdRdOptions (and also has
> > > any option set) And there will be more. StdRdOptions is long outdated
> > > solution and it needs to be replaced.
> >
> > This patch does not apply anymore, so this is moved to next CF, waiting
> > for author.
> Oup...It seems to me that I've fogot to rebase it when it is needed...
> Did it now

Looks like there are some problems with this patch on windows:

src/backend/access/common/reloptions.c(1469): error C2059: syntax error : '}'

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.22359

> On Fri, Mar 2, 2018 at 8:28 PM Andres Freund <[hidden email]> wrote:
>
> On 2018-03-02 20:22:21 +0300, Nikolay Shaplov wrote:
> > Since I get a really big patch as a result, it was decided to commit it in
> > parts.
>
> I get that, but I strongly suggest not creating 10 loosely related
> threads, but keeping it as a patch series in one thread. It's really
> hard to follow for people not continually paying otherwise.

Totally agree. Probably this also makes it harder to see the big picture behind
this patch - which is in turn probably preventing it from getting some more
review. I hope it doesn't sounds ridiculous, taking into account your efforts
by splitting the patch, but maybe it makes sense to gather these pieces
together (as a separate commits, of course) in one thread?

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от пятница, 30 ноября 2018 г. 23:57:21 MSK пользователь Dmitry Dolgov
написал:

> Looks like there are some problems with this patch on windows:
 
> src/backend/access/common/reloptions.c(1469): error C2059: syntax error :
> '}'
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.22359
Phew... It took me a while to get proper windows building environment... But
finally I did it...
This was some MSVC specific error, that happens when you create empty array or
something like this.  I've rewritten that function to remove this array at
all. Now MSVC successfully builds it.

I also did some codestyle improvements, and rebased the patch against the
current master.

The new patch is in attachment.

> > I get that, but I strongly suggest not creating 10 loosely related
> > threads, but keeping it as a patch series in one thread. It's really
> > hard to follow for people not continually paying otherwise.
>
> Totally agree. Probably this also makes it harder to see the big picture
> behind this patch - which is in turn probably preventing it from getting
> some more review. I hope it doesn't sounds ridiculous, taking into account
> your efforts by splitting the patch, but maybe it makes sense to gather
> these pieces together (as a separate commits, of course) in one thread?

The big picture is following. I started from the task: Add possibility to set
up opclass parameters. (Nikita Glukhov now doing it)

I found an reloptions code, that does almost same thing, but it is not flexible
at all, and I can't reuse it for opclass parameters as it is now.

So I came to decision to rewrite reloptions code, so it can be used for
reloptions opclass options and any other kind of options we may need in
future.

While rewriting the code, I found some places in the code that goes from what
seems to be a very long time, and also need refreshing.

This is one of the things.

It is not 100% necessary. Postgres will work with it as it is for ten years or  
more. But since I've touched this part of the code, I want to make this code
more consistent, and more neat.

That's what I am doing. That is what all this patches about.

I'd be happy if we move this task at last.

get-rid-of-StrRdOptions_2.diff (37K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
One thing I would like to revise here is to avoid unnecessary API change
-- for example the RelationHasCascadedCheckOption macro does not really
need to be renamed because it only applies to views, so there's no
possible conflict with other relation types.  We can keep the original
name and add a StaticAssert that the given relation is indeed a view.
This should keep churn somewhat low.  Of course, this doesn't work for
some options where you need a different one for different relation
kinds, such as fillfactor, but that's unavoidable.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от среда, 2 января 2019 г. 0:05:10 MSK пользователь Alvaro Herrera
написал:
> One thing I would like to revise here is to avoid unnecessary API change
> -- for example the RelationHasCascadedCheckOption macro does not really
> need to be renamed because it only applies to views, so there's no
> possible conflict with other relation types.  We can keep the original
> name and add a StaticAssert that the given relation is indeed a view.
> This should keep churn somewhat low.  Of course, this doesn't work for
> some options where you need a different one for different relation
> kinds, such as fillfactor, but that's unavoidable.

My intention was to make API names more consistent. If you are addressing View
specific option, it is good to address it via View[Something] macros or
function. Relations[Something] seems to be a bad name, since we are dealing
not with any relation in general, but with a view relation.

This is internal API, right? If we change it everywhere, then it is changed
and nothing will be broken?

May be it may lead to problems I am unable to see, but I still unable to see
them so I can't make any judgment about it.

If you insist (you have much more experience than I) I would do as you advise,
but still I do not understand.

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
On 2019-Jan-02, Nikolay Shaplov wrote:

> This is internal API, right? If we change it everywhere, then it is changed
> and nothing will be broken?

No, it's exported for extensions to use.  If we change it unnecessarily,
extension authors will hate me (not you) for breaking the compile and
requiring an #if VERSION patch.

These may not be in common usage, but I don't need any additional
reasons for people to hate me.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от четверг, 3 января 2019 г. 16:10:20 MSK пользователь Alvaro Herrera
написал:
> On 2019-Jan-02, Nikolay Shaplov wrote:
> > This is internal API, right? If we change it everywhere, then it is
> > changed and nothing will be broken?
>
> No, it's exported for extensions to use.  If we change it unnecessarily,
> extension authors will hate me (not you) for breaking the compile and
> requiring an #if VERSION patch.

Ok, that's a good reason...

Can we think about backward compatibility aliases?

#define ViewHasCheckOption(relation)                       \
  ((relation)->rd_options &&    \
  ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0)

/* Alias for backward compatibility */
#define RelationHasCheckOption(relation) ViewHasCheckOption(relation)

And keep them for as log as needed to avoid #if VERSION in thirdparty code?

Or that is not the case?


Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
On 2019-Jan-03, Nikolay Shaplov wrote:

> Can we think about backward compatibility aliases?
>
> #define ViewHasCheckOption(relation)                       \
>   ((relation)->rd_options &&    \
>   ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0)
>
> /* Alias for backward compatibility */
> #define RelationHasCheckOption(relation) ViewHasCheckOption(relation)
>
> And keep them for as log as needed to avoid #if VERSION in thirdparty code?

Well, if you do this, at some point you need to tell the extension
author to change the code.  Or they can just keep the code unchanged
forever.  I don't think it's worth the bother.

I would have liked to get a StaticAssert in the definition, but I don't
think it's possible.  A standard Assert() should be possible, though.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от четверг, 3 января 2019 г. 17:15:08 MSK пользователь Alvaro Herrera
написал:

> > Can we think about backward compatibility aliases?
.....
> > And keep them for as log as needed to avoid #if VERSION in thirdparty
> > code?
> Well, if you do this, at some point you need to tell the extension
> author to change the code.  Or they can just keep the code unchanged
> forever.  I don't think it's worth the bother.
Ok, you are the Boss ;-)

I've reverted all the macros names back to Relation* except those related to
fillfactor.

> I would have liked to get a StaticAssert in the definition, but I don't
> think it's possible.  A standard Assert() should be possible, though.

This is a really good idea. I felt uncomfortable with this (ViewOptions *)
type convertation without checking that it is really View. With Assert I fell
much more safe.

I've added AssertMacro to all options related macroses.

And so, adding asserts discovered a bug with parallel_workers options. That
are defined as Heap only option, but in get_relation_info in src/backend/
optimizer/util/plancat.c  a RelationGetParallelWorkers macros is also called
for toasts and other kinds of relations.
I've started a new thread dedicated to this issue, since it needs to be fixed
regardless to this patch.
And for now  I added here a hotfix for this issue that is good enough for now.


I also reworked some comments. BTW do you know what is this comments spoke
about:

 * RelationGetFillFactor() and RelationGetTargetPageFreeSpace() can only        
 * be applied to relations that use this format or a superset for              
 * private options data.

It is speaking about some old times when there can be some other type of
options? 'cause I do not understand what it is speaking about.
I've removed it, I hope I did not remove anything important.

get-rid-of-StrRdOptions_3.diff (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
You introduced new macros IsHeapRelation and IsViewRelation, but I don't
want to introduce such API.  Such things have been heavily contested and
I don't to have one more thing to worry about for this patch, so please
just put the relkind directly in the code.

On 2019-Jan-07, Nikolay Shaplov wrote:

> I also reworked some comments. BTW do you know what is this comments spoke
> about:
>
>  * RelationGetFillFactor() and RelationGetTargetPageFreeSpace() can only        
>  * be applied to relations that use this format or a superset for              
>  * private options data.
>
> It is speaking about some old times when there can be some other type of
> options? 'cause I do not understand what it is speaking about.
> I've removed it, I hope I did not remove anything important.

As I understand it's talking about the varlenas being StdRdOptions and
not anything else.  I think extensibility could cause some relkinds to
use different options.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

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

Nikolay Shaplov
В письме от четверг, 17 января 2019 г. 20:33:06 MSK пользователь Alvaro
Herrera написал:

> You introduced new macros IsHeapRelation and IsViewRelation, but I don't
> want to introduce such API.  Such things have been heavily contested and
> I don't to have one more thing to worry about for this patch, so please
> just put the relkind directly in the code.
Sorry.
I've been trying to avoid repeating

   (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||       \      
                relation->rd_rel->relkind == RELKIND_MATVIEW),         \  
all the time.

Fixed.

Also I make more relaxed parallel_workers behavior in get_relation_info as we
discussed in another thread.

get-rid-of-StrRdOptions_4.diff (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Iwata, Aya
Hi Nikolay,

This patch does not apply.  Please refer to http://commitfest.cputube.org/ and update it.
How about separating your patch by feature or units that can be phased commit.
For example, adding assert macro at first, refactoring StdRdOptions by the next, etc.

So I change status to "Waiting for Author".

Regards,
Aya Iwata
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
Hello.

At Mon, 18 Mar 2019 03:03:04 +0000, "Iwata, Aya" <[hidden email]> wrote in <71E660EB361DF14299875B198D4CE5423DF05777@g01jpexmbkw25>
> This patch does not apply.  Please refer to http://commitfest.cputube.org/ and update it.
> How about separating your patch by feature or units that can be phased commit.
> For example, adding assert macro at first, refactoring StdRdOptions by the next, etc.
>
> So I change status to "Waiting for Author".

That seems to be a good oppotunity. I have some comments.

rel.h:
-#define RelationGetToastTupleTarget(relation, defaulttarg) \
-    ((relation)->rd_options ? \
-     ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : (defaulttarg))
+#define RelationGetToastTupleTarget(relation, defaulttarg)                 \
+    (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||        \
+                 relation->rd_rel->relkind == RELKIND_MATVIEW),            \
+     ((relation)->rd_options ?                                             \
+      ((HeapRelOptions *) (relation)->rd_options)->toast_tuple_target : \
+            (defaulttarg)))

Index AMs parse reloptions by their handler
functions. HeapRelOptions in this patch are parsed in
relation_reloptions calling heap_reloptions. Maybe at least it
should be called as table_options. But I'm not sure what is the
desirable shape of relation_reloptions for the moment.

hio.c:

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

This locution appears four times in the patch and that's the all
where the two macros appear. And it might not be good that
RelationGetBufferForTuple identifies a heap directly since I
suppose that currently tost is a part of heap. Thus it'd be
better that HeapGetTargetPageFreeSpace handles the toast case.
Similary, it doesn't looks good that RelationGetBufferForTuple
consults HeapGetTargretPageFreeSpace() directly. Perhaps it
should be called via TableGetTargetPageFreeSpace(). I'm not sure
what is the right shape of the relationship among a relation and
a table and other kinds of relation. extract_autovac_opts
penetrates through the modularity boundary of toast/heap in a
similar way.


plancat.c:
+    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;

rel.h:
#define RelationGetParallelWorkers(relation, defaultpw)                 \
    (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||        \
                 relation->rd_rel->relkind == RELKIND_MATVIEW),            \
     ((relation)->rd_options ?                                             \
      ((HeapRelOptions *) (relation)->rd_options)->parallel_workers :     \
            (defaultpw)))

These function/macros are doing the same check twice at a call.


regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


123