Re: [PATCH] New [relation] option engine - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: [PATCH] New [relation] option engine
Date
Msg-id 202205151225.jbalr77h46jc@alvherre.pgsql
Whole thread Raw
In response to Re: [PATCH] New [relation] option engine  (Nikolay Shaplov <dhyan@nataraj.su>)
Responses Re: [PATCH] New [relation] option engine
List pgsql-hackers
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)



pgsql-hackers by date:

Previous
From: Simon Riggs
Date:
Subject: Make name optional in CREATE STATISTICS
Next
From: Julien Rouhaud
Date:
Subject: Re: make MaxBackends available in _PG_init