Skip to content

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" 

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.

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