sql: Align timestamp math with PostgreSQL

[bobvawter]

Our implementation of time+duration math delegates to golang's
time.Time.AddDate(), which has different rounding semantics when compared to
PostgreSQL when normalizing invalid month-of-day values.

The most likely input case is that the duration will have no month component
and we can fall back to go's implementation. Another fast-path of checking the
day-of-month allows us to skip the special-case logic for start dates that
aren't the 29th, 30th, or 31st of a month.

name                                      old time/op    new time/op    delta
Add/fast-path-by-no-months-in-duration-8    63.2ns ± 4%    66.2ns ± 1%    +4.75%  (p=0.016 n=5+5)
Add/fast-path-by-day-number-8               70.5ns ± 5%    91.1ns ± 1%   +29.20%  (p=0.008 n=5+5)
Add/no-adjustment-8                         70.6ns ± 3%   206.8ns ± 1%  +192.83%  (p=0.008 n=5+5)
Add/with-adjustment-8                       68.1ns ± 3%   228.4ns ± 1%  +235.59%  (p=0.008 n=5+5)

(No difference in allocations)

Resolves #30976

Release note (sql change): The normalized results of certain timestamp +
duration operations involving year or month durations have been adjusted to
agree with the values returned by PostgreSQL. In cases such as
'2018-01-31'::TIMESTAMP + '1 month', where an intermediate result of February
31st needs to be normalized, previous versions of CockroachDB would "round up"
to March 1. Instead, we would now "round down" to February 28th to agree with
the values returned by PostgreSQL. This change also affects the results of
generate_sequence() when used with timestamps.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[knz]

cc @dt

[knz]

cc @dt

[knz]

LGTM but would welcome additional reviewer's opinions.

[bobvawter]

Does the timestamp-math change need to be hidden behind a setting or cluster version? I’m wondering what happens if you run select some_timestamp + '1 month' from table. Is more than one node going to be evaluating that addition?

[bobvawter]

Does the timestamp-math change need to be hidden behind a setting or cluster version? I’m wondering what happens if you run select some_timestamp + '1 month' from table. Is more than one node going to be evaluating that addition?

[knz]

Yes more than 1 node is going to be evaluating this. This is an "interesting" question.

[knz]

Yes more than 1 node is going to be evaluating this. This is an "interesting" question.

[knz]

@jordanlewis do you want to provide guidance about who to talk to to make this behavior properly conditional?

[knz]

@jordanlewis do you want to provide guidance about who to talk to to make this behavior properly conditional?

[jordanlewis]

Hmm. Bumping the DistSQL minimum version will prevent problems with DistSQL, but it won't prevent the fact that, during a rolling upgrade, two nodes might give two different answers.

I believe the right thing to do is gate the new behavior on a cluster version. I don't remember how this is done.

[jordanlewis]

Hmm. Bumping the DistSQL minimum version will prevent problems with DistSQL, but it won't prevent the fact that, during a rolling upgrade, two nodes might give two different answers.

I believe the right thing to do is gate the new behavior on a cluster version. I don't remember how this is done.

[knz]

@bobvawter here is an example of cluster version gate

pkg/sql/sqlbase/structured.go:   
       if !st.Version.IsMinSupported(cluster.VersionBitArrayColumns) {

[knz]

@bobvawter here is an example of cluster version gate

pkg/sql/sqlbase/structured.go:   
       if !st.Version.IsMinSupported(cluster.VersionBitArrayColumns) {

[knz]

I would recommend a (hidden) session setting in SessionData
which is initialized when the session starts usiong the cluster version gate

(SEssionData variables are transferred across distsql flows)

[knz]

I would recommend a (hidden) session setting in SessionData
which is initialized when the session starts usiong the cluster version gate

(SEssionData variables are transferred across distsql flows)

[danhhz]

I haven't been in the duration stuff (or even thought about it) in ~2 years, so going to remove myself from the review, if that's okay.

[danhhz]

I haven't been in the duration stuff (or even thought about it) in ~2 years, so going to remove myself from the review, if that's okay.

[bobvawter]

Is the SessionData or cluster version available from a static context, or is this going to require a ton of plumbing to get the necessary version down into Add()?

[bobvawter]

Is the SessionData or cluster version available from a static context, or is this going to require a ton of plumbing to get the necessary version down into Add()?

[bobvawter]

It looks like there's an EvalContext available at many/most of the call-chains that eventually call into Add(). I'll see what getting all of that plumbing set up looks like.

[bobvawter]

It looks like there's an EvalContext available at many/most of the call-chains that eventually call into Add(). I'll see what getting all of that plumbing set up looks like.

[mjibson]

I'm not convinced we need cluster or session settings to handle this. Don't we already change implementation of functions regularly without gating them behind any kind of setting?

[mjibson]

I'm not convinced we need cluster or session settings to handle this. Don't we already change implementation of functions regularly without gating them behind any kind of setting?

[knz]

@mjibson that's a valid argument in general. -- if we could argue the previous behavior was unequivocally a bug , then you'd be right we don't need a flag.

However it's not so clear "on its face" that "Jan 31 + 1 month" should be equal to Feb 29 or March 1 (there are other places in postgres, and in cockroachdb, where 1 month = 30 days no matter what). There is an argument to be made in favor of either way, so I can't confidently say that the current behavior would be perceived as a bug by every user looking at it carefully.

[knz]

@mjibson that's a valid argument in general. -- if we could argue the previous behavior was unequivocally a bug , then you'd be right we don't need a flag.

However it's not so clear "on its face" that "Jan 31 + 1 month" should be equal to Feb 29 or March 1 (there are other places in postgres, and in cockroachdb, where 1 month = 30 days no matter what). There is an argument to be made in favor of either way, so I can't confidently say that the current behavior would be perceived as a bug by every user looking at it carefully.

[bobvawter]

PTAL: I've updated this patch with a session variable and cluster setting to allow users to disable the new date math in case their app depends on the golang math. I'm also using this as an opportunity to delay switching to the new logic until a cluster is fully upgraded.

[bobvawter]

PTAL: I've updated this patch with a session variable and cluster setting to allow users to disable the new date math in case their app depends on the golang math. I'm also using this as an opportunity to delay switching to the new logic until a cluster is fully upgraded.

[bobvawter]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/util/duration/duration.go, line 275 at r1 (raw file):

Previously, knz (kena) wrote…

You have peeked into pg's implementation to determine what to do here. Please reference the particular pg version you looked at, and the filename inside the source tree you looked at, and the C function name(s) (or tests), in the comment.

Added this info to the doc on AdditionModeCompatible.

[mjibson]

Here's a more thorough argument about why we should not have either a cluster version check or a session setting and instead just fix it and move on.

Let's begin by looking at the recent changes in builtins that changed the results of a SQL function call (i.e., not fixes for panics):

• sql: properly ignore invalid chars in encoding names
• sql: hashes with NULL input should return NULL
• sql: Fix the handling of NULL arguments in string_agg
• sql: correctly handle NULL arrays in generator funcs

And some for eval.go:

• sql: fix evaluation of IS NULL
• sql: null array element in cast is null

I think every one of the above changes would behave differently during a cluster upgrade, just like this one. I believe none of them had session setting or cluster version checks, we just called it a fixed bug and moved on with life. If someone was depending on the old behavior, too bad.

Another argument, a sort of proof by contradiction: I really really don't want to have a policy that we maintain with session settings any old behavior that doesn't match postgres in addition to the new behavior that does match it. For example, if we are changing the dates as above to make it match postgres and introduce various settings to allow users to choose which implementation they want, would we also not do the same for, say, integer division? We made the choice years ago to have int / int return a decimal instead of an int like postgres does. I think if we apply the same thought process about the date math above to integer division, we would have to change it to match postgres and add a setting to allow for the previous behavior. I think it would be a bad choice to do this (have a setting) for int division, so I also think it's a bad choice for date math.

(I am in full support of just changing int division to match postgres if our policy is to just match postgres during edge cases. I'm equally happy to leave it as is.)

[mjibson]

Here's a more thorough argument about why we should not have either a cluster version check or a session setting and instead just fix it and move on.

Let's begin by looking at the recent changes in builtins that changed the results of a SQL function call (i.e., not fixes for panics):

• sql: properly ignore invalid chars in encoding names
• sql: hashes with NULL input should return NULL
• sql: Fix the handling of NULL arguments in string_agg
• sql: correctly handle NULL arrays in generator funcs

And some for eval.go:

• sql: fix evaluation of IS NULL
• sql: null array element in cast is null

I think every one of the above changes would behave differently during a cluster upgrade, just like this one. I believe none of them had session setting or cluster version checks, we just called it a fixed bug and moved on with life. If someone was depending on the old behavior, too bad.

Another argument, a sort of proof by contradiction: I really really don't want to have a policy that we maintain with session settings any old behavior that doesn't match postgres in addition to the new behavior that does match it. For example, if we are changing the dates as above to make it match postgres and introduce various settings to allow users to choose which implementation they want, would we also not do the same for, say, integer division? We made the choice years ago to have int / int return a decimal instead of an int like postgres does. I think if we apply the same thought process about the date math above to integer division, we would have to change it to match postgres and add a setting to allow for the previous behavior. I think it would be a bad choice to do this (have a setting) for int division, so I also think it's a bad choice for date math.

(I am in full support of just changing int division to match postgres if our policy is to just match postgres during edge cases. I'm equally happy to leave it as is.)

[knz]

Bob I think I am OK with you taking that approach, matt's arguments notwithstanding, for two reasons:

• I'll defer to your judgement about seriousness
• it's also a good exercise for us to understand how such a version conditional needs to be implemented in practice. You could consider this issue as an opportunity to teach something to the rest of the team we'll need to use in other serious cases.

[knz]

Bob I think I am OK with you taking that approach, matt's arguments notwithstanding, for two reasons:

• I'll defer to your judgement about seriousness
• it's also a good exercise for us to understand how such a version conditional needs to be implemented in practice. You could consider this issue as an opportunity to teach something to the rest of the team we'll need to use in other serious cases.

[bobvawter]

PTAL: Here's hoping this is the last rev. After a discussion with Ben, this patch drops the user-configurable knobs, but is still version-aware to avoid inconsistencies within mixed-mode clusters. The version check is performed once and memoized in the SessionData.

[bobvawter]

PTAL: Here's hoping this is the last rev. After a discussion with Ben, this patch drops the user-configurable knobs, but is still version-aware to avoid inconsistencies within mixed-mode clusters. The version check is performed once and memoized in the SessionData.

[dt]

honestly you can still have a nasty heisenbug with drivers that connection pool since every statement you run might get a different session -- I'm increasingly in the "just rip the band-aid off" school.

[dt]

honestly you can still have a nasty heisenbug with drivers that connection pool since every statement you run might get a different session -- I'm increasingly in the "just rip the band-aid off" school.

[bobvawter]

@dt, true, but at least within any given execution it will be consistent.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/settings/cluster/cockroach_versions.go, line 288 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yeah, things get a little weird if we do this out of order. Today's the day to make the 2.1 cluster version anyway.

But I don't think it's a great idea to tie a change like this to the cluster version - the change happens at exactly the point where the upgrade becomes irreversible. I think it might be better to just make the change as soon as the new binary is used while rolling back is still possible.

I thought we wanted all of the nodes to agree that they can use the new date math before any of them start to do so to avoid inconsistency within any given session, but that we're ok with just changing the math behavior out from under the user. Is there some other means to inform the decision about which mode to use in a SessionData?

---

pkg/sql/sessiondata/session_data.go, line 81 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Put a TODO here to remove this in the 2.2 release cycle, since at that point we know it's safe to remove the old behavior.

Done.

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/settings/cluster/cockroach_versions.go, line 288 at r2 (raw file):

Previously, bobvawter (Bob Vawter) wrote…

I thought we wanted all of the nodes to agree that they can use the new date math before any of them start to do so to avoid inconsistency within any given session, but that we're ok with just changing the math behavior out from under the user. Is there some other means to inform the decision about which mode to use in a SessionData?

I think a distsql version is a good idea, which will prevent inconsistency within a session.

[knz]

I'm rather comfortable with the end result here. Just a minor nit about the generator interface and this will be good to co from my perspective.

Reviewed 1 of 4 files at r1, 21 of 21 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/sem/builtins/generator_builtins.go, line 260 at r2 (raw file):

// with integer bounds.
type seriesValueGenerator struct {
	ctx                                 *tree.EvalContext

It'd be better and less error-prone to change the interface of Next() to take an evalcontext as argument, than to store a reference to it here.

[bobvawter]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/settings/cluster/cockroach_versions.go, line 288 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think a distsql version is a good idea, which will prevent inconsistency within a session.

Ah, I see now that's actually a separate thing from the cluster version. Will revise.

[bobvawter]

Reviewable status: complete! 0 of 0 LGTMs obtained

a discussion (no related file):
PTAL: Now with a new distsql version check to enable legacy mode.

---

---

pkg/sql/sem/builtins/generator_builtins.go, line 260 at r2 (raw file):

Previously, knz (kena) wrote…

It'd be better and less error-prone to change the interface of Next() to take an evalcontext as argument, than to store a reference to it here.

That was my first thought, but it turns into a serious sprawl through a whole lot of other value-generation methods, since we would wind up having to also change the sql.valueIterator and sql.planNode interfaces in order to update projectSetNode.Next().

[bdarnell]

LGTM (from a versioning perspective; I didn't look at the implementation closely)

[bdarnell]

LGTM (from a versioning perspective; I didn't look at the implementation closely)

[bobvawter]

bors r+

[bobvawter]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[bobvawter]

Retrying bors due to setup failure in orm tests; looks like pip couldn't pull down a package?

bors r+

[bobvawter]

Retrying bors due to setup failure in orm tests; looks like pip couldn't pull down a package?

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[bobvawter]

orm passed in retry, now roachtest flaked (was ok in first build). Retrying.

bors r+

[bobvawter]

orm passed in retry, now roachtest flaked (was ok in first build). Retrying.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[craig]

Build failed

• GitHub CI (Cockroach)

[knz]

today is not your lucky day it seems...
perhaps try a rebase?

[knz]

today is not your lucky day it seems...
perhaps try a rebase?

[bobvawter]

Retrying after rebase

bors r+

[bobvawter]

Retrying after rebase

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)