Re: Parallel Inserts in CREATE TABLE AS

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