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.

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

Feature/timelineid in clone section #1961

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sudeepta92
Copy link

@Sudeepta92 Sudeepta92 commented Jul 11, 2022

Add timelineid in clone section in the cluster CRD.

In the cluster config we need timeline_id along with the existing possibility of timestamp and cluster name such as for eg:

clone: 
    cluster: my-cluster-xyz # original cluster
    timestamp: "2022-07-11T12:19:35+00:00"   
    clone_target_timeline: "2" 

@@ -197,7 +197,8 @@ type CloneDescription struct {
S3Endpoint string `json:"s3_endpoint,omitempty"`
S3AccessKeyId string `json:"s3_access_key_id,omitempty"`
S3SecretAccessKey string `json:"s3_secret_access_key,omitempty"`
S3ForcePathStyle *bool `json:"s3_force_path_style,omitempty" defaults:"false"`
S3ForcePathStyle *bool `json:"s3_force_path_style,omitempty" defaults:"false"`
TimelineID string `json:"clone_target_timeline" defaults:"latest"`
Copy link
Member

@FxKu FxKu Jul 12, 2022

Choose a reason for hiding this comment

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

Please use go.fmt for right code formatting. This is misaligned.
And type should better be int32. I know it's used as an environment variable later but we can enforece users to only specify numbers here.

Copy link
Author

Choose a reason for hiding this comment

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

The type has been changed and the formatting is corrected.

@FxKu
Copy link
Member

FxKu commented Jul 12, 2022

I think, we should not use the same name as the Spilo environment variable. We have also omitted the clone_ prefix from other variables. How about wal_timeline_id? Then it should be immediately clear what is meant here. Please, add this new field also to the CRD manifest (and chart). The field has to documented as well.

@@ -211,6 +211,9 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
Type: "string",
Format: "uuid",
},
"clone_target_timeline": {
Type: "string",
Copy link
Member

Choose a reason for hiding this comment

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

type should be integer

Copy link
Author

Choose a reason for hiding this comment

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

Actually the timeline can be characters like A, B etc.. for eg C here: base_0000000C00000001000000A3

Copy link
Member

@FxKu FxKu Jul 12, 2022

Choose a reason for hiding this comment

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

But this is not the timeline ID

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sudeepta92 FxKu is right here. The actual ID can only be an integer. What you see in the WAL archive name is a hex value. So for instance 0000003400000000000000CD would be timeline_id 52. Nevertheless, one can also specify latest or current in the recovery.conf. Which is default if nothing is set.
So I think, you could use and int32 here.

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeahh its a hex value in the end. I will change to int32.

Copy link
Author

Choose a reason for hiding this comment

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

@FxKu @thedatabaseme btw, on changing type to integer back from string it fails when no timeline id is passed as it can no longer receive "latest"/"current".

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so have removed the possibility for string value "latest" and in such a scenario it will calcuate the latest timeline id as integer. You may ignore my previous comment.

@FxKu FxKu added this to the 1.9 milestone Jul 15, 2022
@Sudeepta92
Copy link
Author

I think, we should not use the same name as the Spilo environment variable. We have also omitted the clone_ prefix from other variables. How about wal_timeline_id? Then it should be immediately clear what is meant here. Please, add this new field also to the CRD manifest (and chart). The field has to documented as well.

The parameter name has been changed to "wal_timeline_id" from "clone_target_timeline". And I am working on the docs.

@FxKu
Copy link
Member

FxKu commented Jan 5, 2023

move to next milestone, because Spilo PR is still open

@FxKu FxKu modified the milestones: 1.9, 2.0 Jan 5, 2023
@FxKu FxKu modified the milestones: 1.11.0, 2.0 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Open Questions
Development

Successfully merging this pull request may close these issues.

3 participants