Обсуждение: tuplesort.c's copytup_index() is dead code
Commit 9f03ca915 removed the only COPYTUP() call that could reach copytup_index() in practice, making copytup_index() dead code. The attached patch removes this dead code, in line with the existing copytup_datum() case, where tuplesort.c also doesn't directly support COPYTUP() (due to similar considerations around memory management). -- Peter Geoghegan
Вложения
Peter Geoghegan <pg@heroku.com> writes:
> Commit 9f03ca915 removed the only COPYTUP() call that could reach
> copytup_index() in practice, making copytup_index() dead code.
> The attached patch removes this dead code,
I think this may be premature in view of bug #14210. Even if we
don't reinstate use of this function to fix that, I'm not really
convinced we want to get rid of it; it seems likely to me that
we might want it again.
regards, tom lane
On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think this may be premature in view of bug #14210. Even if we > don't reinstate use of this function to fix that, I'm not really > convinced we want to get rid of it; it seems likely to me that > we might want it again. Oh, yes; that involves the same commit I mentioned. I'll look into #14210. FWIW, I think that that bug tells us a lot about hash index usage in the field. It took many months for someone to complain about what ought to have been a really obvious bug. Clearly, hardly anybody is using hash indexes. I broke hash index tuplesort builds in a similar way at one point, too. The slightest bit of regression test coverage would have caught either bug, I believe. I think that some minimal regression tests should be added, because evidently they are needed. -- Peter Geoghegan
Peter Geoghegan <pg@heroku.com> writes: > FWIW, I think that that bug tells us a lot about hash index usage in > the field. It took many months for someone to complain about what > ought to have been a really obvious bug. Clearly, hardly anybody is > using hash indexes. I broke hash index tuplesort builds in a similar > way at one point, too. The slightest bit of regression test coverage > would have caught either bug, I believe. We *do* have regression test coverage, but that code is set up to not kick in at any index scale that would be sane to test in the regression tests. See https://www.postgresql.org/message-id/12194.1466724741@sss.pgh.pa.us regards, tom lane
On Thu, Jun 23, 2016 at 8:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > We *do* have regression test coverage, but that code is set up to not > kick in at any index scale that would be sane to test in the regression > tests. See > https://www.postgresql.org/message-id/12194.1466724741@sss.pgh.pa.us I'm well aware of that issue. This is the same reason why we don't have any regression test coverage of external sorts. I don't think that that's good enough. -- Peter Geoghegan
On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think this may be premature in view of bug #14210. Even if we > don't reinstate use of this function to fix that, I'm not really > convinced we want to get rid of it; it seems likely to me that > we might want it again. You pushed a fix for bug #14210 that seems to not weaken the case for this at all. Where do you stand on this now? I think that leaving things as-is is confusing. Maybe the new copytup_index() comments should indicate why only a defensive stub implementation is needed in practice. I'm certainly not opposed to that. -- Peter Geoghegan
Peter Geoghegan <pg@heroku.com> writes:
> On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think this may be premature in view of bug #14210. Even if we
>> don't reinstate use of this function to fix that, I'm not really
>> convinced we want to get rid of it; it seems likely to me that
>> we might want it again.
> You pushed a fix for bug #14210 that seems to not weaken the case for
> this at all. Where do you stand on this now? I think that leaving
> things as-is is confusing.
Uh, why? It's not a large amount of code and it seems like removing
it puts a fair-size hole in the symmetry of tuplesort's capabilities.
regards, tom lane
On Fri, Jun 24, 2016 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Uh, why? It's not a large amount of code and it seems like removing > it puts a fair-size hole in the symmetry of tuplesort's capabilities. It's not a small amount of code either. Removing the code clarifies the division of labor between COPYTUP() routines in general, their callers (tuplesort_putheaptuple() and tuplesort_puttupleslot() -- which are also puttuple_common() callers), and routines that are similar to those caller routines (in that they at least call puttuple_common()) that do not call COPYTUP() (tuplesort_putdatum(), and now tuplesort_putindextuplevalues()). I believe that this has value. All the extra boilerplate code misleads. -- Peter Geoghegan
On Fri, Jun 24, 2016 at 02:26:18PM -0700, Peter Geoghegan wrote: > On Fri, Jun 24, 2016 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Uh, why? It's not a large amount of code and it seems like removing > > it puts a fair-size hole in the symmetry of tuplesort's capabilities. > > It's not a small amount of code either. > > Removing the code clarifies the division of labor between COPYTUP() > routines in general, their callers (tuplesort_putheaptuple() and > tuplesort_puttupleslot() -- which are also puttuple_common() callers), > and routines that are similar to those caller routines (in that they > at least call puttuple_common()) that do not call COPYTUP() > (tuplesort_putdatum(), and now tuplesort_putindextuplevalues()). > > I believe that this has value. All the extra boilerplate code misleads. At a minimum we can block out the code with #ifdef NOT_USED. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +