Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The postgresql.auto.conf should copy from segment #12212

Merged
merged 1 commit into from Nov 25, 2021
Merged

The postgresql.auto.conf should copy from segment #12212

merged 1 commit into from Nov 25, 2021

Conversation

water32
Copy link
Contributor

@water32 water32 commented Jun 24, 2021

For command ALTER SYSTEM, will modify the config file postgresql.auto.conf, while execute gpexpand, copy this file from master is not correct.

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Communicate in the mailing list if needed
  • Pass make installcheck
  • Review a PR in return to support the community

ginobiliwang
ginobiliwang reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@ginobiliwang ginobiliwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@water32
We may use sub-function or use lambda to remove all these small code block as they are duplicate code.

@ashwinstar
Copy link
Contributor

Would be good to have behave test added on the lines of current gpexpand tests to validate the intended behavior to have GUC value of segment in newer segment and not of coodinator/master node. Plus, this PR is currently directed towards 6X_STABLE. I think the problem would be same for master branch as well. So, would be nice to start from there and drill-down.

@kainwen
Copy link
Contributor

kainwen commented Jul 4, 2021

Would be good to have behave test added on the lines of current gpexpand tests to validate the intended behavior to have GUC value of segment in newer segment and not of coodinator/master node. Plus, this PR is currently directed towards 6X_STABLE. I think the problem would be same for master branch as well. So, would be nice to start from there and drill-down.

@ashwinstar
When I try to use alter system to add a case for this on master branch, I find the auto.conf is consistent across all the segments. But on 6X, it is not consistent.

The root cause of different results of alter system is master branch dispatch the statement however 6X does not dispatch.

Questions:

  1. is there any other method to write the content of auto.conf
  2. what is the correct behavior of alter system?

@water32 Thanks for reporting the issue and the PR. It seems we need more time to dig into this.

@ashwinstar
Copy link
Contributor

Would be good to have behave test added on the lines of current gpexpand tests to validate the intended behavior to have GUC value of segment in newer segment and not of coodinator/master node. Plus, this PR is currently directed towards 6X_STABLE. I think the problem would be same for master branch as well. So, would be nice to start from there and drill-down.

@ashwinstar
When I try to use alter system to add a case for this on master branch, I find the auto.conf is consistent across all the segments. But on 6X, it is not consistent.

The root cause of different results of alter system is master branch dispatch the statement however 6X does not dispatch.

Questions:

1. is there any other method to write the content of `auto.conf`

Well, for testing can use utility mode connection to coordinator and perform ALTER SYSTEM on master branch to have different context for coordinator and segments.

2. what is the correct behavior of `alter system`?

correct is heavy word in this context :-) Both GPDB6 and master branch behavior can be said as correct. Depending on context user may wish the command to behavior in certain way. We currently don't document ALTER SYSTEM as supported user facing command for GPDB though mostly used for internal tests. Ideally, we need to enhance ALTER SYSTEM functionality to provide capability similar to gpconfig to say coordinator value or coordinator only update. Till we don't enhance, currently master branch will always dispatch and hence will be rare case to have postgresql.auto.conf to have different value. I hope this helps.

@kainwen kainwen mentioned this pull request Nov 17, 2021
Merged
kainwen added a commit to kainwen/gpdb that referenced this pull request Nov 22, 2021
@kainwen
Copy postgresql.auto.conf from segment while gpexpand.
77d9ef0 
The config file postgresql.auto.conf might not the same across
QD or QEs. While execute gpexpand, copy this file from master
is not correct. We should copy them from segment for newly added
segments.

In master branch it is not easy to add a normal case by alter system,
because we dispatch AlterSystem statement to QEs. We add the
case based on the advice in
greenplum-db#12212 (comment)
kainwen added a commit that referenced this pull request Nov 24, 2021
@kainwen
Copy postgresql.auto.conf from segment while gpexpand.
de93d31 
The config file postgresql.auto.conf might not the same across
QD or QEs. While execute gpexpand, copy this file from master
is not correct. We should copy them from segment for newly added
segments.

In master branch it is not easy to add a normal case by alter system,
because we dispatch AlterSystem statement to QEs. We add the
case based on the advice in
#12212 (comment)

Authored-by: Miao Chen <miaochen@mail.ustc.edu.cn>
@water32 @kainwen
Copy postgresql.auto.conf from segment while gpexpand.
a0f7719 
The config file postgresql.auto.conf might not the same across
QD or QEs. While execute gpexpand, copy this file from master
is not correct. We should copy them from segment for newly added
segments.
@kainwen
Copy link
Contributor

kainwen commented Nov 25, 2021

Push this. It does not change anything impacting ICW, and local gpexpand test is OK.

@kainwen kainwen merged commit e3e9586 into greenplum-db:6X_STABLE Nov 25, 2021
kainwen added a commit to kainwen/gpdb that referenced this pull request Nov 27, 2021
@kainwen
Revert "Copy postgresql.auto.conf from segment while gpexpand. (green…
7efa605 
…plum-db#12212)"

This reverts commit e3e9586.

gpexpand job becomes red since previous commit, revert first and
wait for later fixing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ginobiliwang ginobiliwang ginobiliwang left review comments

@SmartKeyerror SmartKeyerror SmartKeyerror approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@water32 @ashwinstar @kainwen @ginobiliwang @SmartKeyerror