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.

Sign up for GitHub

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

c2c: allow user to pass start time as a decimal to crdb_internal.fingerprint() #104219

Merged
merged 1 commit into from
Jun 13, 2023
Merged

c2c: allow user to pass start time as a decimal to crdb_internal.fingerprint() #104219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/generated/sql/functions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3106,6 +3106,8 @@ may increase either contention or retry errors, or both.</p>
</span></td><td>Volatile</td></tr>
<tr><td><a name="crdb_internal.encode_key"></a><code>crdb_internal.encode_key(table_id: <a href="int.html">int</a>, index_id: <a href="int.html">int</a>, row_tuple: anyelement) &rarr; <a href="bytes.html">bytes</a></code></td><td><span class="funcdesc"><p>Generate the key for a row on a particular table and index.</p>
</span></td><td>Stable</td></tr>
<tr><td><a name="crdb_internal.fingerprint"></a><code>crdb_internal.fingerprint(span: <a href="bytes.html">bytes</a>[], start_time: <a href="decimal.html">decimal</a>, all_revisions: <a href="bool.html">bool</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
</span></td><td>Stable</td></tr>
<tr><td><a name="crdb_internal.fingerprint"></a><code>crdb_internal.fingerprint(span: <a href="bytes.html">bytes</a>[], start_time: <a href="timestamp.html">timestamptz</a>, all_revisions: <a href="bool.html">bool</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
</span></td><td>Stable</td></tr>
<tr><td><a name="crdb_internal.fingerprint"></a><code>crdb_internal.fingerprint(span: <a href="bytes.html">bytes</a>[], stripped: <a href="bool.html">bool</a>) &rarr; <a href="int.html">int</a></code></td><td><span class="funcdesc"><p>This function is used only by CockroachDB’s developers for testing purposes.</p>
Expand Down
3 changes: 2 additions & 1 deletion pkg/ccl/streamingccl/replicationtestutils/testutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ func (c *TenantStreamingClusters) RequireDestinationFingerprintAtTimestamp(
func FingerprintTenantAtTimestampNoHistory(
t sqlutils.Fataler, db *sqlutils.SQLRunner, tenantID uint64, timestamp string,
) string {
fingerprintQuery := fmt.Sprintf("SELECT * FROM crdb_internal.fingerprint(crdb_internal.tenant_span($1::INT), 0::TIMESTAMPTZ, false) AS OF SYSTEM TIME %s", timestamp)
fingerprintQuery := fmt.Sprintf("SELECT * FROM crdb_internal.fingerprint(crdb_internal."+
"tenant_span($1::INT), 0::DECIMAL, false) AS OF SYSTEM TIME %s", timestamp)
return db.QueryStr(t, fingerprintQuery, tenantID)[0][0]
}

Expand Down
11 changes: 3 additions & 8 deletions pkg/cmd/roachtest/tests/cluster_to_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,17 +583,12 @@ func (rd *replicationDriver) compareTenantFingerprintsAtTimestamp(
rd.t.Status(fmt.Sprintf("comparing tenant fingerprints between start time %s and end time %s",
startTime.UTC(), endTime.UTC()))

// TODO(adityamaru,lidorcarmel): Once we agree on the format and precision we
// display all user facing timestamps with, we should revisit how we format
// the start time to ensure we are fingerprinting from the most accurate lower
// bound.
microSecondRFC3339Format := "2006-01-02 15:04:05.999999"
startTimeStr := startTime.Format(microSecondRFC3339Format)
startTimeDecimal := hlc.Timestamp{WallTime: startTime.UnixNano()}.AsOfSystemTime()
aost := hlc.Timestamp{WallTime: endTime.UnixNano()}.AsOfSystemTime()
fingerprintQuery := fmt.Sprintf(`
SELECT *
FROM crdb_internal.fingerprint(crdb_internal.tenant_span($1::INT), '%s'::TIMESTAMPTZ, true)
AS OF SYSTEM TIME '%s'`, startTimeStr, aost)
FROM crdb_internal.fingerprint(crdb_internal.tenant_span($1::INT), '%s'::DECIMAL, true)
AS OF SYSTEM TIME '%s'`, startTimeDecimal, aost)

var srcFingerprint int64
fingerPrintMonitor := rd.newMonitor(ctx)
Expand Down
28 changes: 17 additions & 11 deletions pkg/sql/sem/builtins/builtins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7627,6 +7627,22 @@ expires until the statement bundle is collected`,
Info: "This function is used only by CockroachDB's developers for testing purposes.",
Volatility: volatility.Stable,
},
tree.Overload{
Types: tree.ParamTypes{
{Name: "span", Typ: types.BytesArray},
{Name: "start_time", Typ: types.Decimal},
{Name: "all_revisions", Typ: types.Bool},
// NB: The function can be called with an AOST clause that will be used
// as the `end_time` when issuing the ExportRequests for the purposes of
// fingerprinting.
},
ReturnType: tree.FixedReturnType(types.Int),
Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
return verboseFingerprint(ctx, evalCtx, args)
},
Info: "This function is used only by CockroachDB's developers for testing purposes.",
Volatility: volatility.Stable,
},
tree.Overload{
Types: tree.ParamTypes{
{Name: "span", Typ: types.BytesArray},
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need the TimestampTZ overload? or can we just replace it with the more accurate decimal overload that you added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept the TimestampTZ overload because it's userfriendly and bc i'm lazy and didn't want to refactor all the tests that already use it :D.

Expand All @@ -7638,17 +7654,7 @@ expires until the statement bundle is collected`,
},
ReturnType: tree.FixedReturnType(types.Int),
Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) {
if len(args) != 3 {
return nil, errors.New("argument list must have three elements")
}
span, err := parseSpan(args[0])
if err != nil {
return nil, err
}
startTime := tree.MustBeDTimestampTZ(args[1]).Time
startTimestamp := hlc.Timestamp{WallTime: startTime.UnixNano()}
allRevisions := bool(tree.MustBeDBool(args[2]))
return fingerprint(ctx, evalCtx, span, startTimestamp, allRevisions /* stripped */, false)
return verboseFingerprint(ctx, evalCtx, args)
},
Info: "This function is used only by CockroachDB's developers for testing purposes.",
Volatility: volatility.Stable,
Expand Down
8 changes: 4 additions & 4 deletions pkg/sql/sem/builtins/fingerprint_builtin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/storage"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
Expand Down Expand Up @@ -89,10 +88,11 @@ func TestFingerprint(t *testing.T) {
}

fingerprint := func(t *testing.T, startKey, endKey string, startTime, endTime hlc.Timestamp, allRevisions bool) int64 {
decimal := eval.TimestampToDecimal(endTime)
aost := endTime.AsOfSystemTime()
var fingerprint int64
query := fmt.Sprintf(`SELECT * FROM crdb_internal.fingerprint(ARRAY[$1::BYTES, $2::BYTES], $3, $4) AS OF SYSTEM TIME %s`, decimal.String())
require.NoError(t, sqlDB.QueryRow(query, roachpb.Key(startKey), roachpb.Key(endKey), startTime.GoTime(), allRevisions).Scan(&fingerprint))
query := fmt.Sprintf(`SELECT * FROM crdb_internal.fingerprint(ARRAY[$1::BYTES, $2::BYTES],$3::DECIMAL, $4) AS OF SYSTEM TIME '%s'`, aost)
Copy link
Contributor

Choose a reason for hiding this comment

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

so this test is for the decimal overload, do we have a test for the TimestampTZ one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, there's a test below this one in this package and plenty of other unit tests in other packages use the timestamp overload.

require.NoError(t, sqlDB.QueryRow(query, roachpb.Key(startKey), roachpb.Key(endKey),
startTime.AsOfSystemTime(), allRevisions).Scan(&fingerprint))
return fingerprint
}

Expand Down
27 changes: 27 additions & 0 deletions pkg/sql/sem/builtins/fingerprint_builtins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,33 @@ import (
"github.com/cockroachdb/errors"
)

func verboseFingerprint(
ctx context.Context, evalCtx *eval.Context, args tree.Datums,
) (tree.Datum, error) {
if len(args) != 3 {
return nil, errors.New("argument list must have three elements")
}
span, err := parseSpan(args[0])
if err != nil {
return nil, err
}

// The startTime can either be a timestampTZ or a decimal.
var startTimestamp hlc.Timestamp
if parsedDecimal, ok := tree.AsDDecimal(args[1]); ok {
startTimestamp, err = hlc.DecimalToHLC(&parsedDecimal.Decimal)
if err != nil {
return nil, err
}
} else {
startTime := tree.MustBeDTimestampTZ(args[1]).Time
startTimestamp = hlc.Timestamp{WallTime: startTime.UnixNano()}
}

allRevisions := bool(tree.MustBeDBool(args[2]))
return fingerprint(ctx, evalCtx, span, startTimestamp, allRevisions /* stripped */, false)
}

func fingerprint(
ctx context.Context,
evalCtx *eval.Context,
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/sem/builtins/fixed_oids.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2383,6 +2383,7 @@ var builtinOidsArray = []string{
2410: `crdb_internal.pretty_value(raw_value: bytes) -> string`,
2411: `to_char(date: date, format: string) -> string`,
2412: `crdb_internal.unsafe_lock_replica(range_id: int, lock: bool) -> bool`,
2413: `crdb_internal.fingerprint(span: bytes[], start_time: decimal, all_revisions: bool) -> int`,
}

var builtinOidsBySignature map[string]oid.Oid
Expand Down