On Mon, Jun 3, 2019 at 6:24 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2019-06-03 18:21:56 -0700, Ashwin Agrawal wrote: > On Mon, Jun 3, 2019 at 5:26 PM Andres Freund <andres@anarazel.de> wrote: > > > Hi, > > > > Thanks for these! > > > > On 2019-06-03 17:24:15 -0700, Ashwin Agrawal wrote: > > > /* > > > * Estimate the size of shared memory needed for a parallel scan > > of this > > > - * relation. The snapshot does not need to be accounted for. > > > + * relation. > > > */ > > > Size (*parallelscan_estimate) (Relation rel); > > > > That's not a typo? > > > > The snapshot is not passed as argument to that function hence seems weird > to refer to snapshot in the comment, as anyways callback function can't > account for it.
It's part of the parallel scan struct, and it used to be accounted for by pre tableam function...
Reads like the comment written from past context then, and doesn't have much value now. Its confusing than helping, to state not to account for snapshot and not any other field.
table_parallelscan_estimate() has snapshot argument and it accounts for it, but callback doesn't. I am not sure how a callback would explicitly use that comment and avoid accounting for snapshot if its using generic ParallelTableScanDescData. But if you feel is helpful, please feel free to keep that text.