-
Notifications
You must be signed in to change notification settings - Fork 206
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.
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
Allow alternative WAL segment sizes for PostgreSQL before 11 #2303
base: integration
Are you sure you want to change the base?
Conversation
@dwsteele, you asked to make PRs from personal forks, not from the company's one. My team lead says that we cannot use personal forks, because there is a possibility of losing access to the PR in case the author of the PR is unavailable (has resigned, gone on vacation, on sick leave, etc.). Therefore, the company policy requires making PRs from the corporate fork. As a solution, if you want to make cosmetic changes, you can form final commit with your changes in the integration branch, and then close the PR. |
3124421
to
f7243e7
Compare
I'm not too excited about adding an option for this. If we are going to let people bypass this then I think it makes more sense to simply remove the constraint at https://github.com/pgbackrest/pgbackrest/blob/release/2.50/src/postgres/interface.c#L176.
You and your colleagues are making valuable contributions so I'm not going to make a big fuss. I still prefer to merge the commit rather than closing it so we'll just continue what we've been doing. |
That is, would you prefer the option without checking for Postgres < 11 that the size of the WAL segment is the default, leaving only the check that the value is valid? |
I can't quite parse this sentence, but I think so? All versions <= 11 are EOL so I'm OK with just removing the constraint on the value being default, which should just mean removing that check and adjusting the test. |
f7243e7
to
0d9a83d
Compare
Before PostgreSQL 11, alternative WAL segment sizes can be selected at compile-time, for example, 64MB. While compile-time settings are generally not well tested by the core, some established forks, such as Greenplum, use them. This patch adds a parameter that allows explicitly specify the expected size of the WAL segment. Which can be useful for backup forks whose base version of Postgres is less than 11.
…ment." This reverts commit f7243e7.
For PostgreSQL before 11, alternate WAL segment sizes can be selected only at compile-time. While compile-time settings are generally not well tested by core, some established forks such as Greenplum use them. This patch removes the limitation of the default WAL segment size for PostgreSQL before 11.
0d9a83d
to
94397ab
Compare
@dwsteele Hello, I would like to know the status of this PR. Do I need to make any other edits? |
@@ -419,7 +419,7 @@ walSegmentNext(const String *walSegment, size_t walSegmentSize, unsigned int pgV | |||
ASSERT(walSegment != NULL); | |||
ASSERT(strSize(walSegment) == 24); | |||
ASSERT(UINT32_MAX % walSegmentSize == walSegmentSize - 1); | |||
ASSERT(pgVersion >= PG_VERSION_11 || walSegmentSize == PG_WAL_SEGMENT_SIZE_DEFAULT); | |||
ASSERT(pgWalSegmentSizeValid(pgVersion, walSegmentSize)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to just remove this assert().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't see that this had been updated. I'd like to keep the implementation simpler since we are dealing with EOL versions of Postgres. See comments.
|
||
FUNCTION_TEST_RETURN(BOOL, IsValidWalSegSize(walSegmentSize)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is worth adding this exception for EOL versions of PostgreSQL. If somebody somehow convinces older versions of PostgreSQL to use large WAL segments sizes, then that is on them.
FormatError, "wal segment size is %u but must be %u for " PG_NAME " <= " PG_VERSION_10_Z, walSegmentSize, | ||
PG_WAL_SEGMENT_SIZE_DEFAULT); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just be removing this and then updating tests.
For PostgreSQL before 11, alternate WAL segment sizes can be selected only at compile-time. While compile-time settings are generally not well tested by core, some established forks such as Greenplum use them.
This patch removes the limitation of the default WAL segment size for PostgreSQL before 11.