Thread: Minor clarifying changes to abbreviated key abort code comments
Attached are a couple of patches that only change code comments. The first (abort abbreviation) patch is recommended for backpatch to 9.5. The second is a tiny tweak. -- Peter Geoghegan
Attachment
On Sat, Oct 31, 2015 at 3:42 PM, Peter Geoghegan <pg@heroku.com> wrote: > Attached are a couple of patches that only change code comments. The > first (abort abbreviation) patch is recommended for backpatch to 9.5. > The second is a tiny tweak. This comment doesn't make sense to me: + * (TSS_BUILDRUNS state prevents control reaching here in any + * case). Unless I'm missing something, that's not actually true. I've committed 0002. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Nov 3, 2015 at 5:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: > This comment doesn't make sense to me: > > + * (TSS_BUILDRUNS state prevents control reaching here in any > + * case). > > Unless I'm missing something, that's not actually true. It is true. consider_abort_common() only actually considers aborting when state is TSS_INITIAL (we're still doing an internal sort). The only other pertinent state here is TSS_BUILDRUNS. The point is that TSS_BUILDRUNS is a generic "point of no return" past which abbreviation cannot be aborted. That is a little arbitrary. -- Peter Geoghegan
On Tue, Nov 3, 2015 at 12:36 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Tue, Nov 3, 2015 at 5:47 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> This comment doesn't make sense to me: >> >> + * (TSS_BUILDRUNS state prevents control reaching here in any >> + * case). >> >> Unless I'm missing something, that's not actually true. > > It is true. consider_abort_common() only actually considers aborting > when state is TSS_INITIAL (we're still doing an internal sort). The > only other pertinent state here is TSS_BUILDRUNS. The point is that > TSS_BUILDRUNS is a generic "point of no return" past which > abbreviation cannot be aborted. That is a little arbitrary. OK, I see. Fixing comments in the back-branches is not always a productive use of time, and in general I might like it if you pushed for such things less frequently. But I've done it anyway in this instance. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Nov 3, 2015 at 11:15 AM, Robert Haas <robertmhaas@gmail.com> wrote: > OK, I see. Fixing comments in the back-branches is not always a > productive use of time, and in general I might like it if you pushed > for such things less frequently. But I've done it anyway in this > instance. I guess I favor doing it where the comment is actually wrong, which does apply here -- we don't *rely* on anything. We could very well abort when state is TSS_BUILDRUNS, but we elect not too, since TSS_BUILDRUNS is taken to mean that it's too late for aborting to be worth it. Thanks -- Peter Geoghegan
On Tue, Nov 3, 2015 at 2:19 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Tue, Nov 3, 2015 at 11:15 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> OK, I see. Fixing comments in the back-branches is not always a >> productive use of time, and in general I might like it if you pushed >> for such things less frequently. But I've done it anyway in this >> instance. > > I guess I favor doing it where the comment is actually wrong, which > does apply here -- we don't *rely* on anything. We could very well > abort when state is TSS_BUILDRUNS, but we elect not too, since > TSS_BUILDRUNS is taken to mean that it's too late for aborting to be > worth it. Fair point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company