On Tue, Jan 5, 2021 at 12:43 PM Luc Vlaming <l...@swarm64.com> wrote: > > On 04-01-2021 14:32, Bharath Rupireddy wrote: > > On Mon, Jan 4, 2021 at 4:22 PM Luc Vlaming <l...@swarm64.com > > <mailto:l...@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
