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

Fix ts_get_now_internal to use transaction time #2235

Merged
merged 1 commit into from Aug 31, 2020

Conversation

pmwkaa
Copy link
Contributor

@pmwkaa pmwkaa commented Aug 20, 2020

Related Issue: #2167

Not really sure how to test it, since we not exported this function. If necessary I guess I could make a test c function to wrap it up and then check that the result of it does not change when executed inside an active transaction more then once.

@pmwkaa pmwkaa requested a review from a team as a code owner August 20, 2020 13:05
@pmwkaa pmwkaa requested review from mfreed, mkindahl, gayyappan and a team and removed request for a team August 20, 2020 13:05
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

i think the current behaviour is intentional and not a bug

@pmwkaa pmwkaa requested a review from a team August 20, 2020 13:08
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #2235 into master will increase coverage by 0.33%.
The diff coverage is 93.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2235      +/-   ##
==========================================
+ Coverage   89.94%   90.28%   +0.33%     
==========================================
  Files         211      212       +1     
  Lines       34262    34471     +209     
==========================================
+ Hits        30818    31123     +305     
+ Misses       3444     3348      -96     
Impacted Files Coverage Δ
src/chunk.h 100.00% <ø> (ø)
src/compat.h 100.00% <ø> (ø)
src/dimension.c 95.38% <ø> (-0.10%) ⬇️
src/utils.h 100.00% <ø> (ø)
tsl/src/compression/simple8b_rle.h 97.37% <ø> (ø)
tsl/src/fdw/fdw.c 88.54% <ø> (+1.80%) ⬆️
tsl/src/init.c 88.88% <ø> (ø)
tsl/src/license.c 81.81% <ø> (ø)
tsl/src/nodes/gapfill/gapfill.c 100.00% <ø> (ø)
tsl/src/remote/txn.c 87.63% <46.66%> (-1.17%) ⬇️
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4538fc6...c240bff. Read the comment docs.

Copy link
Contributor

@WireBaron WireBaron left a comment

Choose a reason for hiding this comment

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

This seems more correct than the current implementation, but given Sven's objection we should probably discuss this during a group meeting.

@erimatnor
Copy link
Contributor

FWIW, this now-replacement is used in two places in the continuous aggregate code: first to set the modification time for invalidations in the insert trigger, then in the materializer to calculate the end of the range to materialize. IMO, transaction time makes sense in both of these cases.

@pmwkaa
Copy link
Contributor Author

pmwkaa commented Aug 27, 2020

@svenklemm any thoughts, suggestions?

@svenklemm
Copy link
Member

since caggs seems to be only user of this function i guess its no problem to change it to whatever makes most sense for current caggs

@pmwkaa pmwkaa merged commit cb2da81 into timescale:master Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants