Обсуждение: PATCH: Spinlock Documentation

Поиск
Список
Период
Сортировка

PATCH: Spinlock Documentation

От
Artem Luzyanin
Дата:
Hello, 
 
I am new to PostgreSQL community, but I would like to become a contributer eventually. I have read through your "Submitting Patch" guide and decided to follow "Start with submitting a patch that is small and uncontroversial to help them understand you, and to get you familiar with the overall process" suggestion. 
 
I am interested in platform-specific spinlock implementation, so I looked at s_lock.h file for possible improvement. Since it took me some time to find possible areas of improvement, I would like to submit a small patch that would facilitate the process for future contributors (including myself). Since this is my first e-mail, please let me know if I should have done something differently in order to submit a patch for the community.  
 
  Project name:
                Spinlock Documentation
  Uniquely identifiable file name:
                s_lock.h
  What the patch does:
                The patch implements addition to documentation in the mentioned above file. This addition outlines the current platform-specific implementations for an easy road map to what else could be done.
  Whether the patch is for discussion or for application:
                This patch is for application.
  Which branch the patch is against:
                This patch is against master branch.
  Whether it compiles and tests successfully:
                The changes allow for successful compilation and testing.
  Whether it contains any platform-specific items and if so, has it been tested on other platforms:
                This patch doesn’t have any platform-specific items.
  Confirm that the patch includes regression tests to check the new feature actually works as described.
                Since this is documentation improvement, regression tests are not needed.
  Include documentation on how to use the new feature, including examples:
                Since it’s documentation improvement, no documentation is needed for documentation.
  Describe the effect your patch has on performance, if any:
                No effect on performance. Unless we are talking about developer’s performance.
  Try to include a few lines about why you chose to do things particular ways:
                I have decided to include the mentioned documentation to outline the areas that need improvement. Any developer, looking for platform-specific code improvement implementation can now easily find the needed area.
 
Thank you for your time and help,
 
Artem Luzyanin

Вложения

Re: PATCH: Spinlock Documentation

От
David Fetter
Дата:
On Sun, Apr 05, 2015 at 06:50:59PM +0000, Artem Luzyanin wrote:
> Hello, 
> 
> I am new to PostgreSQLcommunity, but I would like to become a
> contributer eventually. I have readthrough your "Submitting Patch"
> guide and decided to follow "Start with submitting a patch that is
> small anduncontroversial to help them understand you, and to get you
> familiar with theoverall process" suggestion. 
> 
> I am interested inplatform-specific spinlock implementation, so I
> looked at s_lock.h file for possibleimprovement. Since it took me
> some time to find possible areas of improvement,I would like to
> submit a small patch that would facilitate the process forfuture
> contributors (including myself). Since this is my first e-mail,
> pleaselet me know if I should have done something differently in
> order to submit apatch for the community.  

One issue with this patch is that it is not localized.  If someone
goes and changes the S_LOCK implementation for one of the platforms
below, or adds a new platform, etc., without changing this comment
too, this comment becomes confusingly obsolete.

How do you plan to address this issue?

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate



Re: PATCH: Spinlock Documentation

От
Tom Lane
Дата:
David Fetter <david@fetter.org> writes:
> One issue with this patch is that it is not localized.  If someone
> goes and changes the S_LOCK implementation for one of the platforms
> below, or adds a new platform, etc., without changing this comment
> too, this comment becomes confusingly obsolete.

Indeed.  Moreover, this header comment is supposed to be an overview and
specification of the macros that need to be provided.  I think it's an
actively bad idea to clutter it with platform-by-platform details; that
will create a "can't see the forest for the trees" problem.

If we need more info here, I think a comment block before each section
of the file would make more sense.  But the patch as provided seems
like it would just be redundant if it were refactored in that form.

What would possibly be useful that's not there now is a paragraph or
two describing the overall layout of the file (eg "gcc then non gcc",
or whatever can be said at more or less that level of detail).  But
please don't stick that into the middle of the specification part.
        regards, tom lane



Re: PATCH: Spinlock Documentation

От
Artem Luzyanin
Дата:
<div style="color:#000; background-color:#fff; font-family:HelveticaNeue, Helvetica Neue, Helvetica, Arial, Lucida
Grande,sans-serif;font-size:12px"><div dir="ltr" id="yui_3_16_0_1_1428270899481_6893">Hello,</div><div dir="ltr"
id="yui_3_16_0_1_1428270899481_6892"><br/></div><div dir="ltr" id="yui_3_16_0_1_1428270899481_6891">Thank you very much
foryour feedback! I will work on the changes as soon as I can. </div><div dir="ltr"
id="yui_3_16_0_1_1428270899481_6890"><br/></div><div dir="ltr"
id="yui_3_16_0_1_1428270899481_6889">Respectfully,</div><divdir="ltr" id="yui_3_16_0_1_1428270899481_6889"><br
/></div><divdir="ltr" id="yui_3_16_0_1_1428270899481_6889">Artem Luzyanin</div><br /><div class="qtdSeparateBR"><br
/><br/></div><div class="yahoo_quoted" style="display: block;"><div style="font-family: HelveticaNeue, Helvetica Neue,
Helvetica,Arial, Lucida Grande, sans-serif; font-size: 12px;"><div style="font-family: HelveticaNeue, Helvetica Neue,
Helvetica,Arial, Lucida Grande, sans-serif; font-size: 16px;"><div dir="ltr"><font face="Arial" size="2"> On Sunday,
April5, 2015 5:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:<br /></font></div><br /><br /><div
class="y_msg_container">DavidFetter <<a href="mailto:david@fetter.org" shape="rect"
ymailto="mailto:david@fetter.org">david@fetter.org</a>>writes:<div class="yqt5487667748" id="yqtfd17392"><br
clear="none"/>> One issue with this patch is that it is not localized.  If someone<br clear="none" />> goes and
changesthe S_LOCK implementation for one of the platforms<br clear="none" />> below, or adds a new platform, etc.,
withoutchanging this comment<br clear="none" />> too, this comment becomes confusingly obsolete.</div><br
clear="none"/><br clear="none" />Indeed.  Moreover, this header comment is supposed to be an overview and<br
clear="none"/>specification of the macros that need to be provided.  I think it's an<br clear="none" />actively bad
ideato clutter it with platform-by-platform details; that<br clear="none" />will create a "can't see the forest for the
trees"problem.<br clear="none" /><br clear="none" />If we need more info here, I think a comment block before each
section<brclear="none" />of the file would make more sense.  But the patch as provided seems<br clear="none" />like it
wouldjust be redundant if it were refactored in that form.<br clear="none" /><br clear="none" />What would possibly be
usefulthat's not there now is a paragraph or<br clear="none" />two describing the overall layout of the file (eg "gcc
thennon gcc",<br clear="none" />or whatever can be said at more or less that level of detail).  But<br clear="none"
/>pleasedon't stick that into the middle of the specification part.<br clear="none" /><br clear="none" />           
regards,tom lane<div class="yqt5487667748" id="yqtfd95033"><br clear="none" /></div><br /><br
/></div></div></div></div></div>

Re: PATCH: Spinlock Documentation

От
Artem Luzyanin
Дата:
Hello,

Thank you again for your feedback. I have improved the patch with your suggestions. Please let me know what you think and if I can do anything else.

Current CommitFest link for this patch is: https://commitfest.postgresql.org/5/208/

Respectfully,

Artem Luzyanin



On Sunday, April 5, 2015 5:59 PM, Artem Luzyanin <lisyonok85@yahoo.ca> wrote:


Hello,

Thank you very much for your feedback! I will work on the changes as soon as I can. 

Respectfully,

Artem Luzyanin



On Sunday, April 5, 2015 5:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:


David Fetter <david@fetter.org> writes:

> One issue with this patch is that it is not localized.  If someone
> goes and changes the S_LOCK implementation for one of the platforms
> below, or adds a new platform, etc., without changing this comment
> too, this comment becomes confusingly obsolete.


Indeed.  Moreover, this header comment is supposed to be an overview and
specification of the macros that need to be provided.  I think it's an
actively bad idea to clutter it with platform-by-platform details; that
will create a "can't see the forest for the trees" problem.

If we need more info here, I think a comment block before each section
of the file would make more sense.  But the patch as provided seems
like it would just be redundant if it were refactored in that form.

What would possibly be useful that's not there now is a paragraph or
two describing the overall layout of the file (eg "gcc then non gcc",
or whatever can be said at more or less that level of detail).  But
please don't stick that into the middle of the specification part.

            regards, tom lane





Вложения

Re: PATCH: Spinlock Documentation

От
Robert Haas
Дата:
On Sat, Apr 11, 2015 at 12:03 PM, Artem Luzyanin <lisyonok85@yahoo.ca> wrote:
> Thank you again for your feedback. I have improved the patch with your
> suggestions. Please let me know what you think and if I can do anything
> else.
>
> Current CommitFest link for this patch is:
> https://commitfest.postgresql.org/5/208/

Some review comments:

- The first hunk in s_lock.h touches only whitespace.  Changing the
space to a tab on the "Usually" line would make sense for consistency,
but adding a trailing space to the "override them" line does not.

- As Tom basically said before, I think the "File layout" block
comment will just get out of date and be a maintenance annoyance to
future updaters of this file.   It's not really that hard to see the
structure of the file just by going through it, so I don't think this
is worthwhile.

- Similarly, adding all of the "Currently implemented" lines looks
useless to me.  Why can't somebody see that from just reading the code
itself?

Overall, I'm not seeing much point to this patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company