On 05-01-2021 11:32, Dilip Kumar wrote:
> 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?
>
It can not and should not. I had just only seen so far c++ atomic
variables and not a (postgres-specific?) c atomic variable which
apparently requires the volatile keyword. My stupidity ;)
Cheers,
Luc
