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

From: "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com>
To: 'Kyotaro HORIGUCHI' <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "dhyan(at)nataraj(dot)su" <dhyan(at)nataraj(dot)su>
Cc: "alvherre(at)2ndquadrant(dot)com" <alvherre(at)2ndquadrant(dot)com>, "9erthalion6(at)gmail(dot)com" <9erthalion6(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
Date: 2019-03-20 06:15:38
Message-ID: 71E660EB361DF14299875B198D4CE5423DF0D4DF@g01jpexmbkw25
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi,

> 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.
I think so, too.
And In current codes, RelationGetTargetPageFreeSpace users(ex. heap_page_prune_opt) don't have to think whatever target is toast or heap, but in your change, they need to use them properly.

You told us "big picture" about opclass around the beginning of this thread.
In my understanding, the purpose of this refactoring is to make reloptions more flexible to add opclass.
I understand this change may be needed for opclass, however I think that this is not the best way to refactor reloption.

How about discussing more about this specification including opclass, and finding the best way to refactor reloption?
I have not read the previous thread tightly, so you may have already started preparing.

Regards,
Aya Iwata

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2019-03-20 06:55:39 Re: Offline enabling/disabling of data checksums
Previous Message Tsunakawa, Takayuki 2019-03-20 06:01:11 RE: Libpq support to connect to standby server as priority