Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Bruce Momjian]

On Fri, Jun 24, 2016 at 02:26:18PM -0700, Peter Geoghegan wrote:
> On Fri, Jun 24, 2016 at 2:18 PM, Tom Lane  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  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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Fri, Jun 24, 2016 at 2:18 PM, Tom Lane  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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  writes:
> On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 8:35 PM, Tom Lane  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.1466724...@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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  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.1466724...@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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  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

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  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

[HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

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
From 8b2c0d3b1844caaf144971f488201e806c7fcc5e Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Thu, 23 Jun 2016 18:51:37 -0700
Subject: [PATCH] Remove dead COPYTUP routine in tuplesort.c

Commit 9f03ca915 made copytup_index() dead code.  Remove its body, and
leave only a defensive stub implementation.
---
 src/backend/utils/sort/tuplesort.c | 63 ++
 1 file changed, 2 insertions(+), 61 deletions(-)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 7878660..26eeab6 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -4492,67 +4492,8 @@ comparetup_index_hash(const SortTuple *a, const SortTuple *b,
 static void
 copytup_index(Tuplesortstate *state, SortTuple *stup, void *tup)
 {
-	IndexTuple	tuple = (IndexTuple) tup;
-	unsigned int tuplen = IndexTupleSize(tuple);
-	IndexTuple	newtuple;
-	Datum		original;
-
-	/* copy the tuple into sort storage */
-	newtuple = (IndexTuple) MemoryContextAlloc(state->tuplecontext, tuplen);
-	memcpy(newtuple, tuple, tuplen);
-	USEMEM(state, GetMemoryChunkSpace(newtuple));
-	stup->tuple = (void *) newtuple;
-	/* set up first-column key value */
-	original = index_getattr(newtuple,
-			 1,
-			 RelationGetDescr(state->indexRel),
-			 >isnull1);
-
-	if (!state->sortKeys->abbrev_converter || stup->isnull1)
-	{
-		/*
-		 * Store ordinary Datum representation, or NULL value.  If there is a
-		 * converter it won't expect NULL values, and cost model is not
-		 * required to account for NULL, so in that case we avoid calling
-		 * converter and just set datum1 to zeroed representation (to be
-		 * consistent, and to support cheap inequality tests for NULL
-		 * abbreviated keys).
-		 */
-		stup->datum1 = original;
-	}
-	else if (!consider_abort_common(state))
-	{
-		/* Store abbreviated key representation */
-		stup->datum1 = state->sortKeys->abbrev_converter(original,
-		 state->sortKeys);
-	}
-	else
-	{
-		/* Abort abbreviation */
-		int			i;
-
-		stup->datum1 = original;
-
-		/*
-		 * Set state to be consistent with never trying abbreviation.
-		 *
-		 * Alter datum1 representation in already-copied tuples, so as to
-		 * ensure a consistent representation (current tuple was just
-		 * handled).  It does not matter if some dumped tuples are already
-		 * sorted on tape, since serialized tuples lack abbreviated keys
-		 * (TSS_BUILDRUNS state prevents control reaching here in any case).
-		 */
-		for (i = 0; i < state->memtupcount; i++)
-		{
-			SortTuple  *mtup = >memtuples[i];
-
-			tuple = (IndexTuple) mtup->tuple;
-			mtup->datum1 = index_getattr(tuple,
-		 1,
-		 RelationGetDescr(state->indexRel),
-		 >isnull1);
-		}
-	}
+	/* Not currently needed */
+	elog(ERROR, "copytup_index() should not be called");
 }
 
 static void
-- 
2.7.4


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers