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

feat(outputs.postgresql): Add option to create time column with timezone #13763

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Aug 14, 2023

resolves #13747
based on #13750

This PR adds the ability to switch the time-column to PostgeSQL's "timestamp with time zone" type allowing to issue queries with timezones. Please note, the timestamp is still always transmitted as UTC!

@telegraf-tiger telegraf-tiger bot added area/postgresql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Aug 14, 2023
@srebhan
Copy link
Contributor Author

srebhan commented Aug 14, 2023

@phemmer would love to get your review on this one too!

@jcpunk
Copy link

jcpunk commented Aug 15, 2023

With my limited testing this seems to do what I'm looking for.

@phemmer
Copy link
Contributor

phemmer commented Aug 15, 2023

@phemmer would love to get your review on this one too!

Seems fairly reasonable.
What I meant on the other PR regarding defaults was to put them here with the others so they're all in one place:

p := &Postgresql{
Schema: "public",
TagTableSuffix: "_tag",
TagCacheSize: 100000,
Uint64Type: PgNumeric,
CreateTemplates: []*sqltemplate.Template{{}},
AddColumnTemplates: []*sqltemplate.Template{{}},
TagTableCreateTemplates: []*sqltemplate.Template{{}},
TagTableAddColumnTemplates: []*sqltemplate.Template{{}},
RetryMaxBackoff: config.Duration(time.Second * 15),
Logger: models.NewLogger("outputs", "postgresql", ""),
LogLevel: "warn",
}

But it's just personal preference. What you have is functionally fine.

One change I might suggest though is how to specify the type. Instead of a boolean, use a more generic "timestamp_type" field with a text value which can be "timestamptz" (or "timestamp with timezone" for the full name). This makes it more future proof, as it's possible in the future there might be some other data type, such as integer, which would require another parameter, and make everything rather messy.

Oh, and I think this change might be incomplete as it is. Just changing the column type to timezone doesn't serve much purpose. telegraf point timestamps don't contain timezone information. So you have to pull a timezone from somewhere. Where is this coming from?

@srebhan
Copy link
Contributor Author

srebhan commented Aug 15, 2023

One change I might suggest though is how to specify the type. Instead of a boolean, use a more generic "timestamp_type" field with a text value which can be "timestamptz" (or "timestamp with timezone" for the full name). This makes it more future proof, as it's possible in the future there might be some other data type, such as integer, which would require another parameter, and make everything rather messy.

That is reasonable and I will do that.

Regarding the defaults for the column: I like to do it in Init() to also cover the case where the user set the option to an empty string (by accident) as this would likely cause all types of issues...

@jcpunk
Copy link

jcpunk commented Aug 15, 2023

Let me know when the next test binary is posted and I can give it a shot.

@telegraf-tiger
Copy link
Contributor

@jcpunk
Copy link

jcpunk commented Aug 15, 2023

This is working as I'd expect for cases when my provided time string is unix_ms. Telegraf is setting that to UTC for me "somehow".

@powersj powersj merged commit 71905a7 into influxdata:master Aug 25, 2023
27 checks passed
@github-actions github-actions bot added this to the v1.28.0 milestone Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/postgresql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] ability to control type of datetime column in postgresql output
4 participants