Re: [HACKERS] Minor clarifying changes to abbreviated key abort code comments
On Tue, Nov 3, 2015 at 2:19 PM, Peter Geoghegan wrote: > On Tue, Nov 3, 2015 at 11:15 AM, Robert Haas 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 -- 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] Minor clarifying changes to abbreviated key abort code comments
On Tue, Nov 3, 2015 at 11:15 AM, Robert Haas 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 -- 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] Minor clarifying changes to abbreviated key abort code comments
On Tue, Nov 3, 2015 at 12:36 PM, Peter Geoghegan wrote: > On Tue, Nov 3, 2015 at 5:47 AM, Robert Haas 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 -- 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] Minor clarifying changes to abbreviated key abort code comments
On Tue, Nov 3, 2015 at 5:47 AM, Robert Haas 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 -- 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] Minor clarifying changes to abbreviated key abort code comments
On Sat, Oct 31, 2015 at 3:42 PM, Peter Geoghegan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
From b49bc2ce0b83bc5e4c33c5728f885058ab52cdae Mon Sep 17 00:00:00 2001
From: Peter Geoghegan
Date: Sat, 31 Oct 2015 12:31:05 -0700
Subject: [PATCH 2/2] Correct tiny inaccuracy in strxfrm cache comment
---
src/backend/utils/adt/varlena.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 821e3e7..a89f586 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -62,7 +62,7 @@ typedef struct
char *buf2; /* 2nd string, or abbreviation strxfrm() buf */
int buflen1;
int buflen2;
- int last_len1; /* Length of last buf1 string/strxfrm() blob */
+ int last_len1; /* Length of last buf1 string/strxfrm() input */
int last_len2; /* Length of last buf2 string/strxfrm() blob */
int last_returned; /* Last comparison result (cache) */
bool cache_blob; /* Does buf2 contain strxfrm() blob, etc? */
--
1.9.1
From 72e017d9d62050a4db2ae3ddc712202b03f65d18 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan
Date: Sat, 31 Oct 2015 12:30:31 -0700
Subject: [PATCH 1/2] Clarify point on aborting abbreviation
Backpatch to 9.5, where abbreviated keys were introduced.
---
src/backend/utils/sort/tuplesort.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index d532e87..6572af8 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1294,8 +1294,10 @@ tuplesort_putindextuplevalues(Tuplesortstate *state, Relation rel,
*
* Alter datum1 representation in already-copied tuples, so as to
* ensure a consistent representation (current tuple was just
- * handled). Note that we rely on all tuples copied so far actually
- * being contained within memtuples array.
+ * 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++)
{
@@ -1373,8 +1375,10 @@ tuplesort_putdatum(Tuplesortstate *state, Datum val, bool isNull)
*
* Alter datum1 representation in already-copied tuples, so as to
* ensure a consistent representation (current tuple was just
- * handled). Note that we rely on all tuples copied so far
- * actually being contained within memtuples array.
+ * 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++)
{
@@ -3174,8 +3178,10 @@ copytup_heap(Tuplesortstate *state, SortTuple *stup, void *tup)
*
* Alter datum1 representation in already-copied tuples, so as to
* ensure a consistent representation (current tuple was just
- * handled). Note that we rely on all tuples copied so far actually
- * being contained within memtuples array.
+ * 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++)
{
@@ -3414,8 +3420,10 @@ copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup)
*
* Alter datum1 representation in already-copied tuples, so as to
* ensure a consistent representation (current tuple was just
- * handled). Note that we rely on all tuples copied so far actually
- * being contained within memtuples array.
+ * 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++)
{
@@ -3716,8 +3724,10 @@ copytup_index(Tuplesortstate *state, SortTuple *stup, void *tup)
*
* Alter datum1 representation in already-copied tuples, so as to
* ensure a consistent representation (current tuple was just
- * handled). Note that we rely on all tuples copied so far actually
- * being contained within memtuples array.
+ * 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++)
{
--
1.9.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
