Prohibit starting of postmaster with EXECUTE role.

dkovalev1 commented 4 weeks ago

Normally segment node is started in utility role and execute role is used only when backend process is started. However if one tries to start segment directly in the execute mode using pg_ctl, this leads to SIGSEGV in the postgres process.

It happens because pointer to shared memory flag shmCleanupBackends is left uninitialized. This pointer initialized in the function tmShmemInit() only for GP_ROLE_DISPATCH and GP_ROLE_UTILITY and the flag itself is meaningful only for GP_ROLE_DISPATCH.

This patch prohibits start of the postmaster in execute mode and raises error in this case. To ensure that flag shmCleanupBackends is initialized correctly an assert was added.

This scenario assumes unexpected startup procedure and quite specific use case not falling into regular regression tests. Automated testing of this case would require quite complicated TAP test and does not bring any value to normal use cases. Considering this, it seems not feasible to make a specific automated test for this scenario.

BenderArenadata commented 4 weeks ago

Allure report https://allure.adsw.io/launch/81914

BenderArenadata commented 3 weeks ago

Allure report https://allure.adsw.io/launch/81995

KnightMurloc commented 3 weeks ago

What does it mean to run postmaster (if I understood correctly) with the execute role? Is there any point in supporting this?

dkovalev1 commented 3 weeks ago

What does it mean to run postmaster (if I understood correctly) with the execute role? Is there any point in supporting this?

If we try to start it with pg_ctl with f.e. -o "gp_role=execute", this will try to set gp_role to execute at postmaster startup. This is normal for adb 7.x, but makes no sense for 6.x. Execute role (QE) is normal operation mode for the backend on the segments and set only when user session starts. What do you mean by supporting this?

KnightMurloc commented 3 weeks ago

What do you mean by supporting this?

I mean, what kind of user case is there for this? Might it be worth adding a check and fail with an meaningful error if postmaster is started with this role?

KnightMurloc commented 3 weeks ago

It seems to me that it is worth prohibiting the start of postmaster with the execute role, since this does not make practical sense and if for some reason it is started with this role, then this is clearly an error.

BenderArenadata commented 3 weeks ago

Allure report https://allure.adsw.io/launch/82221

dkovalev1 commented 3 weeks ago

It seems to me that it is worth prohibiting the start of postmaster with the execute role, since this does not make practical sense and if for some reason it is started with this role, then this is clearly an error.

Although it seems good idea for me as well, it's not very practical to do it. Here we don't start the it in the specific mode. Instead it just started normally and pass a set of options to it, one of it (okay, may be the one we pass), by coincidence is gp_role mode. When postgres process starts it applies a set of options from defaults, from configuration file, command line, and some of that option combinations might be invalid, incorrect or differ from the user's expectations. AFAIK the is no and it's extremely difficult to check all options combinations for correctness, this is left for the user - most likely developer or DBA in this case. Also, the user don't start pg_ctl directly in case of GP/ADB, instead he using scripts which already contain correct options set. And, I am not completely sure that starting with gp_role=execute has no practical usage. So, I believe checking for gp_role=execute at start is not feasible.

KnightMurloc commented 2 weeks ago

However if we know that one of the options is incorrect, then it is worth informing the user about it as early as possible. Moreover, in this state, we cannot even take a lock on the table.