Re: [PATCH] Do not use StdRdOptions in Access Methods

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Dent John <denty(at)qqdd(dot)eu>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>
Subject: Re: [PATCH] Do not use StdRdOptions in Access Methods
Date: 2019-10-11 10:54:25
Message-ID: 5996824.IEPH8pLxVD@x200m
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

В письме от четверг, 10 октября 2019 г. 17:17:30 MSK пользователь Amit Langote
написал:

> I read comments that Tomas left at:
> https://www.postgresql.org/message-id/20190727173841.7ypzo4xuzizvijge%40deve
> lopment
>
> I'd like to join Michael in reiterating one point from Tomas' review.
> I think the patch can go further in trying to make the code in this
> area more maintainable.
>
> For example, even without this patch, the following stanza is repeated
> in many places:
>
> options = parseRelOptions(reloptions, validate, foo_relopt_kind,
> &numoptions);
> rdopts = allocateReloptStruct(sizeof(FooOptions), options, numoptions);
> fillRelOptions((void *) rdopts, sizeof(FooOptions), options, numoptions,
> validate, foo_relopt_tab, lengthof(foo_relopt_tab)); return (bytea *)
> rdopts;
>
> and this patch adds few more instances as it's adding more Options structs.
>
> I think it wouldn't be hard to encapsulate the above stanza in a new
> public function in reloptions.c and call it from the various places
> that now have the above code.
The code of reloptions is very historical and illogical. I also noticed that
these lines are repeated several time. And may be it would be better to put
them into reloptions.c. But could anybody clearly explain what are they doing?
Just to give function a proper name. I understand what they are doing, but I
am unable to give short and clear explanation.

I am planning to rewrite this part completely. So we have none of this lines
repeated. I had a proposal you can see it here https://
commitfest.postgresql.org/15/992/ but people on the list told be that patch is
too complex and I should commit it part by part.

So I am doing it now. I am almost done. But to provide clear and logical patch
that introduces my concept, I need StdRdOption to be divided into separated
structures. At least for AM. And I need at to be done as simply as possible
because the rest of the code is going to be rewritten anyway.

That is why I want to follow the steps: make the code uniform, and then
improve it. I have improvement in the pocket, but I need a uniform code before
revealing it.

If you think it is absolutely necessary to put these line into one function, I
will do it. It will not make code more clear, I guess. I See no benefits, but
I can do it, but I would avoid doing it, if possible. At least at this step.

--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nikolay Shaplov 2019-10-11 10:58:41 Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
Previous Message Christoph Berg 2019-10-11 10:48:14 Re: Collation versioning