On 05-01-2021 11:32, Dilip Kumar wrote:
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?
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
