On Wed, Jan 17, 2018 at 11:23 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Aleksandr Parfenov <a.parfe...@postgrespro.ru> writes: >> The new status of this patch is: Ready for Committer > > I don't feel particularly comfortable committing a patch that > was clearly labeled as a rushed draft by its author. > Peter, where do you stand on this work?
I would like to take another pass over WIP-tuplesort-memcontext-fix.patch, to be on the safe side. I'm currently up to my neck in parallel CREATE INDEX work, though, and would prefer to avoid context switching for a week or two, if possible. How time sensitive do you think this is? I think we'll end up doing this: * Committing the minimal modifications made (in both WIP patches) to tuplesort_getdatum() to both v10 and master branches. tuplesort_getdatum() must follow the example of tuplesort_gettupleslot() on these branches, since tuplesort_gettupleslot() already manages to get everything right in recent releases. (There is no known tuplesort_getdatum() crash on these versions, but this still seems necessary on general principle.) * Committing something pretty close to WIP-tuplesort-memcontext-fix.patch to 9.6. * Committing another fix to 9.5. This fix will apply the same principles as WIP-tuplesort-memcontext-fix.patch, but will be simpler mechanically, since the whole batch memory mechanism added to tuplesort.c for 9.6 isn't involved. I'm not sure whether or not we should also apply this still-to-be-written 9.5 patch to 9.4 and 9.3, since those versions don't have grouping sets, and so cannot crash. ISTM that we should leave them alone, since tuplesort has had this problem forever. Perhaps you should go ahead and commit a patch with just the changes to tuplesort_getdatum() now. This part seems low risk, and worth doing in a single, (almost) consistent pass over the back branches. This part is a trivial backpatch, and could be thought of as an independent problem. (It will be nice to get v10 and master branches completely out of the way quickly.) > In a quick look at the patches, WIP-kludge-fix.patch seems clearly > unacceptable for back-patching because it changes the signature and > behavior of ExecResetTupleTable, which external code might well be using. The signature change isn't required, and it was silly of me to add it. But I also really dislike the general approach it takes, and mostly posted it because I thought that it was a useful counterpoint. -- Peter Geoghegan
