Peter Geoghegan <pg@heroku.com> writes:
> I should have already specifically pointed out that the original
> discussion on what became 0002-* is here:
> postgr.es/m/7256.1476711733@sss.pgh.pa.us
> As I said already, the general idea seems uncontroversial.
I looked at the 0002 patch, and while the code is probably OK, I am
dissatisfied with this API spec:
+ * If copy is TRUE, the slot receives a copied tuple that will stay valid
+ * regardless of future manipulations of the tuplesort's state. Memory is
+ * owned by the caller. If copy is FALSE, the slot may just receive a pointer
+ * to a tuple held within the tuplesort. The latter is more efficient, but
+ * the slot contents may be corrupted if there is another call here before
+ * previous slot contents are used.
What does "here" mean? If that means specifically "another call of
tuplesort_gettupleslot", say so. If "here" refers to the whole module,
it would be better to say something like "the slot contents may be
invalidated by any subsequent manipulation of the tuplesort's state".
In any case it'd be a good idea to delineate safe usage patterns, perhaps
"copy=FALSE is recommended only when the next tuplesort manipulation will
be another tuplesort_gettupleslot fetch into the same slot."
There are several other uses of "call here", both in this patch and
pre-existing in tuplesort.c, that I find equally vague and unsatisfactory.
Let's try to improve that.
regards, tom lane
