colflow: fix emitting of flow stats and simplify the creator a bit

[yuzefovich]

We introduced isGatewayNode boolean on the flow creator a couple of
months ago which is used to decide whether the last outbox should emit
the flow level memory and disk stats. However, we forget to properly
initialize it in the constructor, and this commit fixes that issue. The
impact though is very limited - we would just emit some redundant stats,
and because they are used for maximum, they would get ignored.

Additionally, this commit simplifies the flow creator by removing
materializerAdded field because it is essentially a duplicate of
isGatewayNode.

Release note: None

[cockroach-teamcity]

This change is 

[asubiotto]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/colflow/vectorized_flow.go, line 461 at r1 (raw file):

	// node. This field doesn't have to be accessed atomically because all
	// writes to it happen before the outboxes' goroutines are started.
	numOutboxes int32

I think it still needs to be accessed atomically because they are different goroutines. Using an atomic value will ensure we flush the CPU cache which will make the updated value available to any following goroutines running on other cores.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

---

pkg/sql/colflow/vectorized_flow.go, line 461 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think it still needs to be accessed atomically because they are different goroutines. Using an atomic value will ensure we flush the CPU cache which will make the updated value available to any following goroutines running on other cores.

Ok, reverted. I tried searching for documentation of this scenario and didn't find any, do you have some links maybe?

[RaduBerinde]

---

pkg/sql/colflow/vectorized_flow.go, line 461 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Ok, reverted. I tried searching for documentation of this scenario and didn't find any, do you have some links maybe?

https://golang.org/ref/mem
http://www.cs.cmu.edu/~410-f10/doc/Intel_Reordering_318147.pdf

In general, you must either use locks (or other synchronization primitives) or atomic stuff. Note that atomic.Store is not a simple store, it uses XCHG which is a "locked" instruction (which flushes the store buffer and ensures a total ordering among all cores w.r.t other locked instructions). (code in $GOPATH/src/runtime/internal/atomic/asm_amd64.s).

[asubiotto]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

[yuzefovich]

TFTRs!

bors r+

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/colflow/vectorized_flow.go, line 461 at r1 (raw file):

Previously, RaduBerinde wrote…

https://golang.org/ref/mem
http://www.cs.cmu.edu/~410-f10/doc/Intel_Reordering_318147.pdf

In general, you must either use locks (or other synchronization primitives) or atomic stuff. Note that atomic.Store is not a simple store, it uses XCHG which is a "locked" instruction (which flushes the store buffer and ensures a total ordering among all cores w.r.t other locked instructions). (code in $GOPATH/src/runtime/internal/atomic/asm_amd64.s).

Interesting, thanks for the links. My reading is that whenever in doubt, just use the sync things and don't try to be smart :)

[craig]

Build succeeded:

• GitHub CI (Cockroach)