Re: Re: Reusing abbreviated keys during second pass of ordered [set] aggregates

On Thu, Dec 17, 2015 at 11:43 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Wed, Dec 16, 2015 at 12:04 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>>> What kind of state is that? Can't we define this in terms of what it
>>> is rather than how it gets that way?
>>
>> It's zeroed.
>>
>> I guess we can update everything, including existing comments, to reflect that.

Thanks, this looks much easier to understand from here.

> Attached revision updates both the main commit (the optimization), and
> the backpatch commit (updated the contract).

- /* abbreviation is possible here only for by-reference types */
+ /*
+ * Abbreviation is possible here only for by-reference types.
Note that a
+ * pass-by-value representation for abbreviated values is forbidden, but
+ * that's a distinct, generic restriction imposed by the SortSupport
+ * contract.

I think that you have not written what you meant to write here. I
think what you mean is "Note that a pass-by-REFERENCE representation
for abbreviated values is forbidden...".

+ /*
+ * If we produced only one initial run (quite likely if the total data
+ * volume is between 1X and 2X workMem), we can just use that
tape as the
+ * finished output, rather than doing a useless merge. (This obvious
+ * optimization is not in Knuth's algorithm.)
+ */
+ if (state->currentRun == 1)
+ {
+ state->result_tape = state->tp_tapenum[state->destTape];
+ /* must freeze and rewind the finished output tape */
+ LogicalTapeFreeze(state->tapeset, state->result_tape);
+ state->status = TSS_SORTEDONTAPE;
+ return;
+ }

I don't understand the point of moving this code. If there's some
reason to do this after rewinding the tapes rather than beforehand, I
think we should articulate that reason in the comment block.

The last hunk in your 0001 patch properly belongs in 0002.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company