Skip to content

Conversation

@findepi
Copy link
Member

@findepi findepi commented Jan 23, 2023

Doc changes for changes added in #15104
Per #15326 issue.

@findepi findepi added docs no-release-notes This pull request does not require release notes entry labels Jan 23, 2023
@cla-bot cla-bot bot added the cla-signed label Jan 23, 2023
should be used for querying. If time elapsed since last materialized view refresh
is greater than the grace period, the materialized view acts as a normal view and
materialized data is not used. If not specified, the grace period defaults to
infinity. See :doc:`refresh-materialized-view` for more about refreshing
Copy link
Member

@mosabua mosabua Jan 23, 2023

Choose a reason for hiding this comment

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

Re "defaults to infinity". We should clarify what that means in practice. Something like that:

As a result a materialized by default therefore without the specified grace period always uses the materialized data from the last refresh and never falls back to act like a view and retrieve the current data from the source tables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out.
You worded it very precisely. I am afraid some users may not understand. The sentence feels pretty long. Would it be possible to reword it or split?

Ideally, can you please make it a suggestion, or somehow else indicate which piece of text you would want to have replaced with the new sentence(s)?

Copy link
Member

@mosabua mosabua Jan 24, 2023

Choose a reason for hiding this comment

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

Yeah .. I was struggling putting something together.

How about this? After "inifinity." insert this:

The default behavior therefore is that the queries accessing the materialized view always access the data from the most recent refresh and the materialized view storage tables. The query processing never falls back to process the materialized view access like a view and retrieve the current data from the source tables. This only occurs if a grace period is specified.

I know this is long but I feel like this is the only way to truly explain what is and is not happening. Maybe @colebow can chime in and refine my wording some more.

Copy link
Member

Choose a reason for hiding this comment

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

@findepi did you forget to pull the above suggestion in?

@mosabua
Copy link
Member

mosabua commented Jan 23, 2023

Also .. related question .. I am not 100% sure so could you clarify / confirm .. is this a breaking change for MVs that are already defined?

@findepi
Copy link
Member Author

findepi commented Jan 23, 2023

@mosabua see #15326 and #15326 (comment) in particular.

@mosabua
Copy link
Member

mosabua commented Jan 24, 2023

So at this stage the code change is a breaking change. I am not sure if and how we want to communicate this in the release notes @martint @colebow but it also looks like it is a bit late for that since this was merged on the 16th of December so it shipped already with 405.

@findepi
Copy link
Member Author

findepi commented Jan 24, 2023

but it also looks like it is a bit late for that since this was merged on the 16th of December so it shipped already with 405.

Only the syntax, so the breakage didn't happen yet.

However, I am not prepared to discuss breaking changes mitigations on a docs PR.
I am not breaking docs here, or anything else.

@mosabua
Copy link
Member

mosabua commented Jan 24, 2023

but it also looks like it is a bit late for that since this was merged on the 16th of December so it shipped already with 405.

Only the syntax, so the breakage didn't happen yet.

However, I am not prepared to discuss breaking changes mitigations on a docs PR. I am not breaking docs here, or anything else.

Sounds good.

Copy link
Member

@colebow colebow left a comment

Choose a reason for hiding this comment

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

LGTM.

@findepi
Copy link
Member Author

findepi commented Jan 25, 2023

Thanks you @mosabua @colebow @ebyhr for your review!

I've took the what we ended up with and put it together with code changes here: #15842

@findepi findepi closed this Jan 25, 2023
@findepi findepi deleted the findepi/grace-docs branch January 25, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants