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

sql: a few compatibility features #40194

Merged
merged 4 commits into from
Aug 28, 2019

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Aug 24, 2019

  • add pg_my_temp_schema() builtin
  • add array overlaps (&&) operator
  • add array contains (@>) and contained_by(<@) operators
  • add support for SET SESSION AUTHORIZATION DEFAULT
  • add pg_get_function_identity_arguments builtiin

Closes #18442.
Closes #33337.
Closes #39821.
Closes #40211.

@jordanlewis jordanlewis requested review from justinj and a team August 24, 2019 16:29
@jordanlewis jordanlewis requested a review from a team as a code owner August 24, 2019 16:29
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jordanlewis jordanlewis force-pushed the pg-my-temp-schema branch 5 times, most recently from f0e5da2 to 05b1d0b Compare August 27, 2019 21:45
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

r1 and r4 OK

There's a code mix-up between r2 and r3, so you can either fix the mix-up or squash the commits together.

r5 I really want to trust your understanding of the pg spec but there is currently no guardrail if anyone else tweaks the implementation later - so maybe more tests would help here.

Reviewed 2 of 2 files at r1, 15 of 15 files at r2, 3 of 3 files at r3, 7 of 9 files at r4, 3 of 3 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @justinj)


pkg/sql/logictest/testdata/logic_test/builtin_function, line 2252 at r5 (raw file):

# Sanity check pg_get_function_identity_arguments.
query T
SELECT pg_get_function_identity_arguments('convert_from'::regproc::oid)

I think the complexity of this function mandates a wider diversity of tests.
You probably want a mix of:

  • polymorphic functions
  • functions with zero arguments
  • overloaded functions

and see that the behavior is consistent (and compatible with pg) for all of them.


pkg/sql/sem/builtins/pg_builtins.go, line 627 at r1 (raw file):

	),

	"pg_my_temp_schema": makeBuiltin(defProps(),

Maybe add link to pg doc where this is explained in comment above.


pkg/sql/sem/tree/eval.go, line 296 at r2 (raw file):

// ArrayContains return true if the haystack contains all needles.
func ArrayContains(haystack *DArray, needles *DArray) (*DBool, error) {

Where is this function used? It seems you got it inlined below.


pkg/sql/sem/tree/eval.go, line 303 at r2 (raw file):

		var found bool
		for j := range haystack.Array {
			if needles.Array[i] == haystack.Array[j] {

I think you need Compare here too


pkg/sql/sem/tree/eval.go, line 307 at r3 (raw file):

		var found bool
		for _, hay := range haystack.Array {
			if needle.Compare(ctx, hay) == 0 {

ok this change you can pull into the previous commit


pkg/sql/sem/tree/eval.go, line 2095 at r3 (raw file):

				haystack := MustBeDArray(left)
				needles := MustBeDArray(right)
				return ArrayContains(ctx, haystack, needles)

ditto

Copy link
Member Author

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

TFTR. I squashed r2 and r3 which I think resolves the issues. I added more tests as well.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @justinj and @knz)


pkg/sql/logictest/testdata/logic_test/builtin_function, line 2252 at r5 (raw file):

Previously, knz (kena) wrote…

I think the complexity of this function mandates a wider diversity of tests.
You probably want a mix of:

  • polymorphic functions
  • functions with zero arguments
  • overloaded functions

and see that the behavior is consistent (and compatible with pg) for all of them.

Done.


pkg/sql/sem/builtins/pg_builtins.go, line 627 at r1 (raw file):

Previously, knz (kena) wrote…

Maybe add link to pg doc where this is explained in comment above.

Done.


pkg/sql/sem/tree/eval.go, line 296 at r2 (raw file):

Previously, knz (kena) wrote…

Where is this function used? It seems you got it inlined below.

Done.


pkg/sql/sem/tree/eval.go, line 303 at r2 (raw file):

Previously, knz (kena) wrote…

I think you need Compare here too

Done.


pkg/sql/sem/tree/eval.go, line 307 at r3 (raw file):

Previously, knz (kena) wrote…

ok this change you can pull into the previous commit

Done.


pkg/sql/sem/tree/eval.go, line 2095 at r3 (raw file):

Previously, knz (kena) wrote…

ditto

Done.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

LGTM with two nits

Reviewed 13 of 15 files at r7, 9 of 9 files at r8, 3 of 3 files at r9.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @justinj)


pkg/sql/parser/sql.y, line 3126 at r9 (raw file):

| SESSION AUTHORIZATION non_reserved_word_or_sconst
  {
    return unimplemented(sqllex, "set session authorization name")

maybe file an issue and link it here? I understand that there are still several errors without issue number throughout the grammar, but I think Andy was asking that any new such errors should be paired with an issue.


pkg/sql/sem/builtins/pg_builtins.go, line 615 at r9 (raw file):

	// pg_get_function_identity_arguments returns the argument list necessary to
	// identify a function, in the form it would need to appear in within ALTER
	// FUNCTION, for instance. This form omits default values.

nit: URL to pg doc here too

Release note (sql change): add the pg_my_temp_schema builtin for
Postgres compatibility.
This commit adds support for the && operator on 2 arrays, which returns
true if the elements have at least 1 common element, and the @> and <@
operators on 2 arrays, which return true if the array on the left
contains the array or the right, or vice versa.

In addition, it refactors the && operator from being syntactic sugar for
the inet_overlaps builtin to be a normal operator.

Release note (sql change): Add the overlaps (&&), contains (@>) and
contained_by (<@) operators for arrays.
Currently a no-op.

Release note: None
This is used by a couple of UI tools. It returns a textual list of the
argument types of a function that's identified by its OID.

Release note (sql change): add pg_get_function_identity_arguments
builtin
@jordanlewis
Copy link
Member Author

Fixed nits. TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 28, 2019

Build failed

@jordanlewis
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Aug 28, 2019
40194: sql: a few compatibility features r=jordanlewis a=jordanlewis

- add pg_my_temp_schema() builtin
- add array overlaps (&&) operator
- add array contains (@>) and contained_by(<@) operators
- add support for SET SESSION AUTHORIZATION DEFAULT
- add pg_get_function_identity_arguments builtiin

Closes #18442.
Closes #33337.
Closes #39821.
Closes #40211.

40220: storage: run Store RaftMessageHandler methods in Stopper task r=nvanbenschoten a=nvanbenschoten

Informs #40213.

This change fixes the flakiness of `TestNodeLivenessSetDecommissioning` observed in #40213, although it doesn't fix the crash we saw on a qualification cluster. The commit ensures that each RaftMessageHandler handler method ties its lifetime to that of its Store's Stopper. This is necessary because the Store's Stopper might not be the same as the RaftTransport's stopper.

40246: exec: reset the columnarizer's internal batch r=yuzefovich a=yuzefovich

The columnarizer "owns" a batch, but it is never reset (namely,
selection vector can remain unset).

Release note: None

40280: debug: fix and improve pretty-printing of ConfChange Raft entries r=nvanbenschoten a=nvanbenschoten

The tryRaftLogEntry function was trying to decode into the wrong struct type, resulting in the error `proto: wrong wireType = 0 for field Desc`.

The second commit unifies the pretty-printing of ConfChange Raft entries with that of non-ConfChange Raft entries. This allows us to print the WriteBatch of ConfChanges.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 28, 2019

Build succeeded

@craig craig bot merged commit bc9c056 into cockroachdb:master Aug 28, 2019
@jordanlewis jordanlewis deleted the pg-my-temp-schema branch August 28, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants