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
Attachments:
0001-Remove-dead-COPYTUP-routine-in-tuplesort.c.patchtext/x-patch; charset=US-ASCII; name=0001-Remove-dead-COPYTUP-routine-in-tuplesort.c.patchDownload+2-62
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
/messages/by-id/12194.1466724741@sss.pgh.pa.us
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
/messages/by-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 +--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers