On Tue, Jan 5, 2021 at 12:43 PM Luc Vlaming <luc@swarm64.com> wrote:
>
> On 04-01-2021 14:32, Bharath Rupireddy wrote:
> > On Mon, Jan 4, 2021 at 4:22 PM Luc Vlaming <luc@swarm64.com
> > <mailto:luc@swarm64.com>> wrote:
> > > Sorry it took so long to get back to reviewing this.
> >
> > Thanks for the comments.
> >
> > > wrt v18-0001....patch:
> > >
> > > + /*
> > > + * If the worker is for parallel insert in CTAS, then
> > use the proper
> > > + * dest receiver.
> > > + */
> > > + intoclause = (IntoClause *) stringToNode(intoclausestr);
> > > + receiver = CreateIntoRelDestReceiver(intoclause);
> > > + ((DR_intorel *)receiver)->is_parallel_worker = true;
> > > + ((DR_intorel *)receiver)->object_id = fpes->objectid;
> > > I would move this into a function called e.g.
> > > GetCTASParallelWorkerReceiver so that the details wrt CTAS can be put in
> > > createas.c.
> > > I would then also split up intorel_startup into intorel_leader_startup
> > > and intorel_worker_startup, and in GetCTASParallelWorkerReceiver set
> > > self->pub.rStartup to intorel_worker_startup.
> >
> > My intention was to not add any new APIs to the dest receiver. I simply
> > made the changes in intorel_startup, in which for workers it just does
> > the minimalistic work and exit from it. In the leader most of the table
> > creation and sanity check is kept untouched. Please have a look at the
> > v19 patch posted upthread [1].
> >
>
> Looks much better, really nicely abstracted away in the v20 patch.
>
> > > + volatile pg_atomic_uint64 *processed;
> > > why is it volatile?
> >
> > Intention is to always read from the actual memory location. I referred
> > it from the way pg_atomic_fetch_add_u64_impl,
> > pg_atomic_compare_exchange_u64_impl, pg_atomic_init_u64_impl and their
> > u32 counterparts use pass the parameter as volatile pg_atomic_uint64 *ptr.
But in your case, I do not understand the intention that where do you
think that the compiler can optimize it and read the old value?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
