Revert #201 partially #386
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Current greenplum_path.sh is a result of apache#201 to make greenplum_path.sh always using the Python during ./configure. But it inadvertently changes user's $PATH. This portion of apache#201 is reverted, and it should be fine if user use pip install --user to install dependencies.
2 tasks
|
@my-ship-it what about merge this? :) |
foreyes
pushed a commit
to foreyes/cloudberrydb
that referenced
this pull request
Jun 17, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
foreyes
pushed a commit
to foreyes/cloudberrydb
that referenced
this pull request
Jul 4, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
foreyes
pushed a commit
to foreyes/cloudberrydb
that referenced
this pull request
Jul 4, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
foreyes
pushed a commit
to foreyes/cloudberrydb
that referenced
this pull request
Jul 8, 2024
Current greenplum_path.sh is a result of apache#201 to make greenplum_path.sh always using the Python during ./configure. But it inadvertently changes user's $PATH. This portion of apache#201 is reverted, and it should be fine if user use pip install --user to install dependencies.
foreyes
pushed a commit
to foreyes/cloudberrydb
that referenced
this pull request
Jul 22, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
jiaqizho
pushed a commit
to jiaqizho/cloudberrydb
that referenced
this pull request
Jul 23, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
jiaqizho
pushed a commit
to jiaqizho/cloudberrydb
that referenced
this pull request
Jul 23, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
jiaqizho
pushed a commit
to jiaqizho/cloudberrydb
that referenced
this pull request
Jul 24, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
jiaqizho
pushed a commit
to jiaqizho/cloudberrydb
that referenced
this pull request
Aug 7, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
jiaqizho
pushed a commit
to jiaqizho/cloudberrydb
that referenced
this pull request
Aug 14, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (apache#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
my-ship-it
pushed a commit
that referenced
this pull request
Aug 19, 2024
The pg_resgroup_move_query() function implemented in 51ee26b has several disadvantages: 1) Slot leaking. The slot acquired in initiator process doesn't free if target process isn't alive, didn't receive a signal, or even received, but at the time it was in idle state. 2) Race condition between UnassignResGroup() called at the end of transaction (or honestly any other code) and handler called immediately after USR1 signal. 3) Not working entrydb process moving. Previously, the USR1 signal was sent to only one process, target or entrydb. Entrydb moving was never tested properly. Improvements added to solve the first problem: 1) Feedback from target process to initiator. Target process can set initiator's latch to quickly interrupt pg_resgroup_move_query() from awaiting. 2) Existed movetoResSlot parameter acts like a mark. Initiator sets it and waits on latch. If movetoResSlot become NULL, slot control is on the target process side. 3) Initiator PID. Used by target process to get initiator's latch. 4) Mutex. To guard all critical moveto* parameters from parallel changes. To solve the second problem, there was an attempt to use Postgres interruptions. I was unhappy to know GPDB use raw InterruptPending value to do some pre-cancellation of dispatched queries. GPDB thinks InterruptPending can be triggered only by "negative" events, which leads to cancelation. The temporary solution with InterruptPending-like "positive" flag showed that we may wait for next CHECK_FOR_INTERRUPTS() call for a long. For example, some fat insert may do group moving only at the end of dispatching, which makes no sense. Thus, I decided to decrease the probability of races by using additional IsTransactionState() check. IMPORTANT NOTICE. The existed solution still affected by race conditions. The current handler's implementation is an example of bad design and should be reworked. To solve the third problem, now we send signals to target and entrydb processes separately. We do UnassignResGroup() for one process only, not cleaning all counters for still working entrydb process inside handler. More, entrydb moving logic is now separated from segments moving, and so, it became more readable. New GUC (gp_resource_group_move_timeout) was added to limit a time we're waiting for feedback from target. New regression tests shows there is no slot leaking with some rare and now correctly resolved cases. * Move faulty assertions so they not shoot in case process already moving. * Fix entrydb process wrong slot resetting on moving (#386) Entrydb and main processes may share same resgroup slot or, in other words, they have same session slot. When moving process to new group, we free old slot only when last (in our case entrydb) process moved to new group. At the same time we zeroing current session slot. The `sessionResetSlot()` called from `UnassignResGroup()` didn't check which slot we zeroing. This caused a bug, when we was unable to move same session twice (with the error like `ERROR: process XXX is in IDLE state`). The reason - new session slot set in main process, was zeroed by entrydb process. From now, we analyze current session slot before zeroing. --------- Co-authored-by: Alexey Gordeev <goa@arenadata.io>
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current greenplum_path.sh is a result of #201 to make greenplum_path.sh always using the Python during
./configure. But it inadvertently changes user'sPATH. This portion of #201 is reverted, and it should be fine if people all usepip install --userto install dependencies.