Hello Tom,
05.05.2020 21:59, Tom Lane wrote:
>> Could you please let me know if the fix is incorrect (or not elaborated
>> enough to be included in the upcoming releases) or this issue is false
>> positive?
> I took a look at this. I see the hazard, but I'm not sure that
> I like your proposed quick-fix. Essentially, the problem is that
> gistBuildCallback is supposing that the tuple it creates will survive
> the execution of its subroutines, which in fact it won't always.
> But that means that at some point the tuple pointer it's passed down to
> those subroutines becomes a dangling pointer. What guarantee would we
> have that the subroutines don't access the tuple after that point?
Thanks for your review!
Yes, I'm not excited by this fix too, so I'll try to find another not so
quick way to fix it.
> Or, if we do just hack it as you suggest, there had better be a couple
> of fat comments in gistBufferingBuildInsert warning about the hazards.
>
> I was somewhat dismayed to realize from looking at the code coverage
> report that we have exactly zero test coverage of the gist buffering
> build code paths today. That's just awful; how did the feature get
> committed with no test coverage? Your proposed test additions raise the
> coverage in gistbuild.c and gistbuildbuffers.c to something respectable,
> at least. But it looks like there are still some potentially-delicate
> paths that aren't tested, notably the "have to stop buffering" business.
> Can we do better at reasonable cost, or are those paths just never
> reached without huge data volume? (Could we make them more reachable
> by turning down maintenance_work_mem or some other setting?)
Please look at the improved test that makes the code coverage for
gistbuildbuffers.c almost 100%.
Best regards,
Alexander