Changing this value from "PROFILE_RESET" to "PROFILE_REQUEST" could cause
deadlock: "collector" processed "PROFILE_RESET" query while client backend
waits data from the "collector_mq" shared memory message queue.

Not deadlock but infinite waiting of requested profile backend process caused by lostPROFILE_REQUEST request (ifreset of request variable happened after setting ofPROFILE_REQUEST value).

In general, your proposed solution (moving of request value saving under collector lock) resolves your problem. But we might end up to lostPROFILE_RESET request whenPROFILE_RESET and successive some another signal arrive before collector acquires its lock. And root cause I see in asynchronous nature ofPROFILE_RESET request: client backend just send signal (set latch) andrelease queue lock allowing other clients to perform other requests.

You might rewrite patch to include some feedback mechanics for backend performingPROFILE_RESET request, e.g., via waiting around collector's message queue, leaving the initial state of codebase (before your patch) unchanged. Or you might stick out for your solution - I figure losing ofPROFILE_RESET signal as non-critical and anyway I plan to rewrite a whole mechanics of IPC communication soon. But in this case the moving of request variable reading under collector lock is enough, other changes that incorporates this variable setting on backend's side under lock looks redundant.

Making PROFILE_RESET request processing synchronous was not the goal of these patches. But this synchronization could be easily added.
There were two problems:

1. deadlock (stuck, enter inconsistent state with denial of service, etc.) while processing script below

psql -c "CREATE EXTENSION pg_wait_sampling"

for j in {1..100}; do
echo "iteration $j"

for i in {1..10}; do
echo "
SELECT * FROM pg_wait_sampling_get_profile();
SELECT pg_wait_sampling_reset_profile();
" | psql >psql$i.log &
done
wait

done

2. Coredumps while parallel processing of two "queries" regression tests with "aqo" module loaded.
   aqo+pgws.txt

1. deadlock (stuck, enter inconsistent state with denial of service, etc.) while processing script below

psql -c "CREATE EXTENSION pg_wait_sampling"
for j in {1..100}; do
echo "iteration $j"
for i in {1..10}; do
echo "
SELECT * FROM pg_wait_sampling_get_profile();
SELECT pg_wait_sampling_reset_profile();
" | psql >psql$i.log &
done
wait
done

Yes, deadlock in your presented above scenario occurs when successive queriespg_wait_sampling_get_profile() try to acquire queue lock and get stuck due to another backend process that waits on message queue already holds this lock and collector that had lostPROFILE_REQUEST signal waits for new signals.

As I have mentioned, makingpg_wait_sampling_reset_profile() synchronous resolves this and more race condition problems. But you might leave your solution (reorder collector lock acquiring andrequest variable saving on collector side) - the losing ofPROFILE_RESET signals is not critical on current stage.

Coredumps while parallel processing of two "queries" regression tests with "aqo" module loaded.
aqo+pgws.txt

I cannot find coredump(

@rzharkov could you make necessary changes - removeLockRelease() moving inreceive_array() andpg_wait_sampling_reset_profile() functions? This moving is redundant as it doesn't protectcollector_hdr->request shared variable but just is used to wait for collector completes its communication work.

Of course, I'll do it. The task is stuck, because my reviewer is on vacation :)

could you make necessary changes - remove LockRelease() moving in receive_array() and pg_wait_sampling_reset_profile() functions? This moving is redundant as it doesn't protect collector_hdr->request shared variable but just is used to wait for collector completes its communication work.

No, it's not redundant. Consider the following interleaving:

1. S1 executed pg_wait_sampling_reset_profile and setcollector_hdr->request toPROFILE_RESET.
2. S2 is executing receive_array. It acquires the queue lock, acquires and immediately releases the collector lock...
3. Now collector acquires the collector lock and readsPROFILE_RESET from S1.
4. S2 continues and setscollector_hdr->request toPROFILE_REQUEST.
5. collector continues and overwritescollector_hdr->request withNO_REQUEST andPROFILE_REQUEST is lost.

The following test script hangs sooner or later ifcollector_hdr->request is not protected with the lock in receive_array:

for i in `seq 1000`; doecho $ifor j in `seq 10`; dopsql <<EOF >/dev/null &select * from pg_wait_sampling_profile;select pg_wait_sampling_reset_profile();EOFdonewaitdone

For pg_wait_sampling_reset_profile it doesn't matter, but it's better to keep both functions in sync.

+extern const LOCKTAG   queueTag;+extern const LOCKTAG   collectorTag;...-extern void init_lock_tag(LOCKTAG *tag, uint32 lock);

Instead of init_lock_tag that clashes with the like named symbol in aqo, we now have two other unprefixed external symbols that may clash with other modules. I think we should prefix all external symbols with "pgws_" to be safe. And it's better to do this in a separate PR. (Roman, I'm sorry for asking you to fix both problems together. Now I see that they are unrelated.) I will split the patch.

Consider the following interleaving:

1. S1 executed pg_wait_sampling_reset_profile and set `collector_hdr->request` to `PROFILE_RESET`.2. S2 is executing receive_array. It acquires the queue lock, acquires and immediately releases the collector lock...3. Now collector acquires the collector lock and reads `PROFILE_RESET` from S1.4. S2 continues and sets `collector_hdr->request` to `PROFILE_REQUEST`.5. collector continues and overwrites `collector_hdr->request` with `NO_REQUEST` and `PROFILE_REQUEST` is lost.

Hmm, agreed. Free interference ofcollector_hdr->request setting insidereceive_array() function and non-atomic manipulation with this variable inside collector imposes concurrency issues.

Approved this fix.

@rzharkov could you rewrite your commit message so that schedules causing concurrency issue would be exposes in step-by-step manner. And this message have to include two schedules (your provided and one from@shinderuk) to motivate proposed changes: reorder request keeping after collector lock acquiring in collector process and setting request variable in regular backends under this lock.

@maksm90 I dropped the changes related to init_lock_tag from the patch and rewrote the commit message. Please take a look. Are you satisfied with the commit message?

I dropped the changes related to init_lock_tag from the patch

It looked as harmless refactoring. I would accepted patch with this fix.

I dropped the changes related to init_lock_tag from the patch and rewrote the commit message.

All right! Approved.