Thread: Re: [COMMITTERS] pgsql: Do a pass of code review for the ALTER TABLE
Neil Conway wrote: > Log Message: > ----------- > Do a pass of code review for the ALTER TABLE ADD INHERITS patch. Keep > the read lock we hold on the table's parent relation until commit. > Update equalfuncs.c for the new field in AlterTableCmd. Various > improvements to comments, variable names, and error reporting. > > There is room for further improvement here, but this is at least > a step in the right direction. Thanks, that is what was needed. The author obviously took the patch as far as he could, and we needed to adjust his XXX areas, rather than not apply the patch and have the code drifting. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Neil Conway wrote: > > Log Message: > > ----------- > > Do a pass of code review for the ALTER TABLE ADD INHERITS patch. Keep > > the read lock we hold on the table's parent relation until commit. > > Update equalfuncs.c for the new field in AlterTableCmd. Various > > improvements to comments, variable names, and error reporting. > > > > There is room for further improvement here, but this is at least > > a step in the right direction. > > Thanks, that is what was needed. The author obviously took the patch as > far as he could, and we needed to adjust his XXX areas, rather than not > apply the patch and have the code drifting. Hmm, is this how we should do things? I mean, should I finish the autovacuum parts of my relminxid patch, apply it, and then hope for someone to fix the mistaeks? And if we don't see any failure in the buildfarm, assume that all is well? To me this is really the easiest way, but I have a hard time convincing myself that I want to have it easy but break things in a way that nobody notices. The other day when I typoed a commit to the 8.1 branch I was all red in the face. I wonder what will happen if someone points to me or Greg as causing major breakage somewhere, just because the patch was applied in a hurry without careful review. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: >>Thanks, that is what was needed. The author obviously took the patch as >>far as he could, and we needed to adjust his XXX areas, rather than not >>apply the patch and have the code drifting. >> >> > >Hmm, is this how we should do things? I mean, should I finish the >autovacuum parts of my relminxid patch, apply it, and then hope for >someone to fix the mistaeks? And if we don't see any failure in the >buildfarm, assume that all is well? > >To me this is really the easiest way, but I have a hard time convincing >myself that I want to have it easy but break things in a way that nobody >notices. The other day when I typoed a commit to the 8.1 branch I was >all red in the face. I wonder what will happen if someone points to me >or Greg as causing major breakage somewhere, just because the patch was >applied in a hurry without careful review. > > > I am guilty of a similar recent sin that Tom caught. But, like you, I am opposed to lessening the stability in the code base, which is something we should be proud of and guard carefully. cheers andrew
Alvaro Herrera wrote: > Bruce Momjian wrote: > > Neil Conway wrote: > > > Log Message: > > > ----------- > > > Do a pass of code review for the ALTER TABLE ADD INHERITS patch. Keep > > > the read lock we hold on the table's parent relation until commit. > > > Update equalfuncs.c for the new field in AlterTableCmd. Various > > > improvements to comments, variable names, and error reporting. > > > > > > There is room for further improvement here, but this is at least > > > a step in the right direction. > > > > Thanks, that is what was needed. The author obviously took the patch as > > far as he could, and we needed to adjust his XXX areas, rather than not > > apply the patch and have the code drifting. > > Hmm, is this how we should do things? I mean, should I finish the > autovacuum parts of my relminxid patch, apply it, and then hope for > someone to fix the mistaeks? And if we don't see any failure in the > buildfarm, assume that all is well? > > To me this is really the easiest way, but I have a hard time convincing > myself that I want to have it easy but break things in a way that nobody > notices. The other day when I typoed a commit to the 8.1 branch I was > all red in the face. I wonder what will happen if someone points to me > or Greg as causing major breakage somewhere, just because the patch was > applied in a hurry without careful review. The author had been through several iterations of the patch, and I needed to clean it up to look more like our code. The XXX comments where of the variety where he was asking for confirmation on things, and we can adjust those after application. Doing it with his version of the patch would have been harder. If the issues aren't addressed, the patch is eventually reverted, as we have done in the past. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
<span style="font-family: times new roman;">I'm keeping on studying multixact.c and log management, and I hope you can helpme, as usual, in clearing my doubts.</span><br style="font-family: times new roman;" /><br style="font-family: timesnew roman;" /><span style="font-family: times new roman;">My doubts now concern MultixactID wrap-around management.</span><br style="font-family: times new roman;" /><span style="font-family: times new roman;">Afaics, it is possibleto spawn multixactids so quickly to have a wrap-around and to start overwriting the data stored in the offset slru(but analogous considerations apply to the member slru as well). This would cause corruption, if the overwritten infowas still needed, e.g., by a (very) long-running transaction. This is of course very unlikely in practice, but yet stillpossible in theory.</span><br style="font-family: times new roman;" /><br style="font-family: times new roman;" /><spanstyle="font-family: times new roman;">In GetNewMultiXactId () wrap-around of MultiXactId seems to be simply handledthis way:</span><br style="font-family: times new roman;" /><pre class="fragment" style="font-family: times new roman;">00780 <span class="comment">_/* Handle wraparound of the nextMXact counter */</span><br />00781 <span class="keywordflow">if</span>(<a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a16"target="_blank">MultiXactState</a>-><a class="code"href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/structMultiXactStateData.html#o0" target="_blank">nextMXact</a>< <a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8h.html#a1"target="_blank">FirstMultiXactId</a>)<br />00782 <a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a16" target="_blank">MultiXactState</a>-><aclass="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/structMultiXactStateData.html#o0"target="_blank">nextMXact</a> =<a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8h.html#a1" target="_blank">FirstMultiXactId</a>;</pre><brstyle="font-family: times new roman;" /><span style="font-family: times newroman;">I cannot see how this may avoid possible overwriting of still needed <span style="text-decoration: underline;">data.</span>To address such an issue shouldn't one need to check against </span><a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a17"style="font-family: times new roman;" target="_blank">OldestMemberMXactId</a><spanstyle="font-family: times new roman;">, </span><a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a18"style="font-family: times new roman;" target="_blank">OldestVisibleMXactId?</a>Or, alternatively, rely on an approach similar to the one taken to handle standardXID generation (xidWarnLimit, see GetNewTransactionId)?<span style="font-family: monospace;"><span style="text-decoration:underline;"><br /><br /></span></span>Is it me who's missing something or is it just that such a casehas been considered so unlikely not to motivate additional overheads/checks?<br /><br />Thanks in advance!<br /><br /> Paolo<p> Chiacchiera con i tuoi amici in tempo reale! <br /> http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
ops, i did forget to update the e-mail subject, sorry. I am reposting it with an appropriate one.<br /><br /><b><i></i></b>--------------------------------------------------------------------------------<blockquote class="replbq"style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px;"><span style="font-family:times new roman;">I'm keeping on studying multixact.c and log management, and I hope you can help me,as usual, in clearing my doubts.</span><br style="font-family: times new roman;" /><br style="font-family: times new roman;"/><span style="font-family: times new roman;">My doubts now concern MultixactID wrap-around management. </span><brstyle="font-family: times new roman;" /><span style="font-family: times new roman;">Afaics, it is possible to spawnmultixactids so quickly to have a wrap-around and to start overwriting the data stored in the offset slru (but analogousconsiderations apply to the member slru as well). This would cause corruption, if the overwritten info was stillneeded, e.g., by a (very) long-running transaction. This is of course very unlikely in practice, but yet still possiblein theory.</span><br style="font-family: times new roman;" /><br style="font-family: times new roman;" /><span style="font-family:times new roman;">In GetNewMultiXactId () wrap-around of MultiXactId seems to be simply handled this way:</span><brstyle="font-family: times new roman;" /><pre class="fragment" style="font-family: times new roman;">00780 <span class="comment">_/* Handle wraparound of the nextMXact counter */</span><br />00781 <span class="keywordflow">if</span>(<a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a16"target="_blank">MultiXactState</a>-><a class="code"href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/structMultiXactStateData.html#o0" target="_blank">nextMXact</a>< <a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8h.html#a1"target="_blank">FirstMultiXactId</a>)<br />00782 <a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a16" target="_blank">MultiXactState</a>-><aclass="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/structMultiXactStateData.html#o0"target="_blank">nextMXact</a> =<a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8h.html#a1" target="_blank">FirstMultiXactId</a>;</pre><brstyle="font-family: times new roman;" /><span style="font-family: times newroman;">I cannot see how this may avoid possible overwriting of still needed <span style="text-decoration: underline;">data.</span>To address such an issue shouldn't one need to check against </span><a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a17"style="font-family: times new roman;" target="_blank">OldestMemberMXactId</a><spanstyle="font-family: times new roman;">, </span><a class="code" href="http://www.mcknight.de/pgsql-doxygen/cvshead/html/multixact_8c.html#a18"style="font-family: times new roman;" target="_blank">OldestVisibleMXactId?</a>Or, alternatively, rely on an approach similar to the one taken to handle standardXID generation (xidWarnLimit, see GetNewTransactionId)?<span style="font-family: monospace;"><span style="text-decoration:underline;"><br /><br /></span></span>Is it me who's missing something or is it just that such a casehas been considered so unlikely not to motivate additional overheads/checks?<br /><br />Thanks in advance!<br /><br /> Paolo</blockquote><br /><p> Chiacchiera con i tuoi amici in tempo reale! <br /> http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com
paolo romano wrote: > ops, i did forget to update the e-mail subject, sorry. I am reposting > it with an appropriate one. > Please do NOT create a post on a new subject by using an MUA's reply mechanism, even if you replace the subject. The MUA will create an in-reply-to header which will be totally inappropriate and confuse other MUAs and achive processors that use such headers for thread construction. Only use a reply facility when you are genuinely replying on the same subject. This goes for webmail MUAs too (yahoo, squirrelmail etc.) The right way to get the address is to put it in your address book or as a last resort cut and paste it. cheers andrew
paolo romano <paolo.romano@yahoo.it> writes:
> My doubts now concern MultixactID wrap-around management.
> Afaics, it is possible to spawn multixactids so quickly to have a
> wrap-around and to start overwriting the data stored in the offset
> slru (but analogous considerations apply to the member slru as
> well).
I looked into this when the multixact code was written. There is a
theoretical risk but I think it's entirely theoretical. MXIDs are
unlikely to be consumed faster than XIDs over the long term, and also
can be recycled sooner. So you'd run up against XID wraparound (which
we do defend against) first.
regards, tom lane
Thank you for the feed-back. In fact the risk seems neglibible, but it means that at least I'm not misunderstanding whatthat code does...<br /><br /><br /><b><i>Tom Lane <tgl@sss.pgh.pa.us></i></b> ha scritto: <blockquote class="replbq"style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px;"> paolo romano writes:<br/>> My doubts now concern MultixactID wrap-around management. <br />> Afaics, it is possible to spawn multixactidsso quickly to have a<br />> wrap-around and to start overwriting the data stored in the offset<br />> slru(but analogous considerations apply to the member slru as<br />> well).<br /><br />I looked into this when the multixactcode was written. There is a<br />theoretical risk but I think it's entirely theoretical. MXIDs are<br />unlikelyto be consumed faster than XIDs over the long term, and also<br />can be recycled sooner. So you'd run up againstXID wraparound (which<br />we do defend against) first.<br /><br /> regards, tom lane<br /></blockquote><br /><p>Chiacchiera con i tuoi amici in tempo reale! <br /> http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com