Date: Sun, 15 May 2022 14:25:47 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Nikolay Shaplov <dhyan@nataraj.su>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
	Michael Paquier <michael@paquier.xyz>,
	Amit Langote <amitlangote09@gmail.com>
Subject: Re: [PATCH] New [relation] option engine
Message-ID: <202205151225.jbalr77h46jc@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2827066.dzGTFBD8OB@thinkpad-pgpro>
Archived-At: 
 <https://www.postgresql.org/message-id/202205151225.jbalr77h46jc%40alvherre.pgsql>
Precedence: bulk

I'm sorry if you've already said this elsewhere, but can you please
state what is the *intention* of this patchset?  If it's a pure
refactoring (but I don't think it is), then it's a net loss, because
after pgindent it summarizes as:

 58 files changed, 2714 insertions(+), 2368 deletions(-)

so we end up 500+ lines worse than the initial story.  However, I
suspect that's not the final situation, since I saw a comment somewhere
that opclass options have to be rewritten to modify this mechanism, and
I suspect that will remove a few lines.  And you maybe have a more
ambitious goal.  But what is it?

Please pgindent your code for the next submission, making sure to add
your new typedef(s) to typedefs.list so that it doesn't generate stupid
spaces.  After pgindenting you'll notice the argument lists of some
functions look bad (cf. commit c4f113e8fef9).  Please fix that too.

I notice that you kept the commentary about lock levels in the place
where they were previously defined.  This is not good.  Please move each
explanation next to the place where each option is defined.

For next time, please use "git format-patch" for submission, and write a
tentative commit message.  The committer may or may not use your
proposed commit message, but with it they will know what you're trying
to achieve.

The translatability marker for detailmsg for enums is wrong AFAICT.  You
need gettext_noop() around the strings themselves IIRC.  I think you
need to get rid of the _() call around the variable that receives that
value and use errdetail() instead of errdetail_internal(), to avoid
double-translating it; but I'm not 100% sure.  Please experiment with
"make update-po" until you get the messages in the .po file.  

You don't need braces around single-statement blocks.

Thanks

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)