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: implement pg_prepared_statements virtual table #42018

Merged
merged 1 commit into from Oct 31, 2019

Conversation

@otan
Copy link
Collaborator

otan commented Oct 30, 2019

  • resolves #41778
  • add List() to the prepared statements accessor
  • implemented pg_prepared_statements, with some fun caveats
    • i think we always use wire format, so from_sql is always false?
    • had to add an extra data type for CreatedAt, not sure how to increment SizeOf here with the location variable set.
    • other caveats are documented as a comment.

Release note (sql change): introduces pg_prepared_statements table for use

@otan otan requested a review from yuzefovich Oct 30, 2019
@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Oct 30, 2019

This change is Reviewable

@otan otan force-pushed the otan-cockroach:otan-virtual_table branch 4 times, most recently from 4f641c9 to 00cadd2 Oct 30, 2019
@otan otan requested a review from cockroachdb/sql-execution Oct 30, 2019
Copy link
Collaborator

arulajmani left a comment

Left a few minor comments

Reviewed 1 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan and @yuzefovich)


pkg/sql/conn_executor.go, line 2347 at r1 (raw file):

// List is part of the preparedStatementsAccessor interface.
func (ps connExPrepStmtsAccessor) List() map[string]*PreparedStatement {
	// Return a copy of the data, to prevent modification of the map.

The map isn't being modified by the function that calls List() . Do we need to make this copy?


pkg/sql/pg_catalog.go, line 1804 at r1 (raw file):

			if err := addRow(
				tree.NewDString(name),
				tree.NewDString("PREPARE "+name+argumentsStr+" AS "+stmt.SQL+";"),

You could use fmt.Sprintf here


pkg/sql/pg_catalog.go, line 1809 at r1 (raw file):

				tree.DBoolFalse, // from_sql
			); err != nil {
				return err

You could simplify this by simply returning addRow(...) and removing the return nil below

Copy link
Contributor

yuzefovich left a comment

Nice work getting this done so quickly! A few comments from me.

Reviewed 8 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @otan)


pkg/sql/conn_executor.go, line 2347 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

The map isn't being modified by the function that calls List() . Do we need to make this copy?

I think I'd rather be on the safe side and return a copy (unless there is a huge performance burden). This way, in case some other function will modify the map later, the bug will be avoided.


pkg/sql/pg_catalog.go, line 1766 at r1 (raw file):

// The statement field differs in that it uses the parsed version
// of the PREPARE statement.
// The from_sql statement will always return false, as we use the wire protocol.

I don't think this is true - if we're talking to CRDB via CLI we could issue a PREPARE statement, and I think from_sql would be true in that case. cc @mjibson


pkg/sql/pg_catalog.go, line 1804 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

You could use fmt.Sprintf here

I agree, that's what we usually do (same goes for argumentsStr above).


pkg/sql/pg_catalog.go, line 1809 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

You could simplify this by simply returning addRow(...) and removing the return nil below

I don't think so - here we're returning from inside of the loop, but return nil is outside of it.


pkg/sql/prepared_stmt.go, line 82 at r1 (raw file):

// of prepared statements.
type preparedStatementsAccessor interface {
	// List returns all prepared statements as a map keyed by name.

[nit]: I'd call out explicitly that the map is a copy.


pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1844 at r1 (raw file):


statement ok
CREATE TABLE types(a timestamptz, b integer);

[super nit]: semicolons are not needed in the logic test queries.


pkg/sql/sqlbase/prepared_statement.go, line 44 at r1 (raw file):

	InferredTypes []oid.Oid

	// CreatedAt is the timestamp this prepare statement was made.

[nit]: I feel like preposition at is missing at the end.


pkg/sql/sqlbase/prepared_statement.go, line 67 at r1 (raw file):

	// This is missing pm.CreatedAt.Location.
	// TODO: confirm whether we should add it in.

I'd add use unsafe.Sizeof(time.Time{}) as the size of a single CreatedAt (which should evaluate to 24 bytes). time.Time has a pointer to time.Location, and I feel like that actual time.Location struct is cached and is shared among different timestamps. Feel free to dig in and figure that out :)

Copy link
Member

mjibson left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @otan, and @yuzefovich)


pkg/sql/pg_catalog.go, line 1766 at r1 (raw file):

Previously, yuzefovich wrote…

I don't think this is true - if we're talking to CRDB via CLI we could issue a PREPARE statement, and I think from_sql would be true in that case. cc @mjibson

Yahor is correct. However, I'm not sure we currently distinguish between the two. If we don't, it's probably best to leave this as is (as false) and mark it with a TODO in the code. We should probably only spend time implementing it correctly if we can demonstrate an ORM that cares.

Copy link
Collaborator Author

otan left a comment

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


pkg/sql/conn_executor.go, line 2347 at r1 (raw file):

Previously, yuzefovich wrote…

I think I'd rather be on the safe side and return a copy (unless there is a huge performance burden). This way, in case some other function will modify the map later, the bug will be avoided.

I was following the style guide, which tells me to as long as it isn't a perf burden... I don't think this is (for now)


pkg/sql/pg_catalog.go, line 1766 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Yahor is correct. However, I'm not sure we currently distinguish between the two. If we don't, it's probably best to leave this as is (as false) and mark it with a TODO in the code. We should probably only spend time implementing it correctly if we can demonstrate an ORM that cares.

i think we found a way. so marking this as done.


pkg/sql/pg_catalog.go, line 1804 at r1 (raw file):

Previously, yuzefovich wrote…

I agree, that's what we usually do (same goes for argumentsStr above).

Done.


pkg/sql/pg_catalog.go, line 1809 at r1 (raw file):

Previously, yuzefovich wrote…

I don't think so - here we're returning from inside of the loop, but return nil is outside of it.

yes, this is in a loop.


pkg/sql/prepared_stmt.go, line 82 at r1 (raw file):

Previously, yuzefovich wrote…

[nit]: I'd call out explicitly that the map is a copy.

Done.


pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1844 at r1 (raw file):

Previously, yuzefovich wrote…

[super nit]: semicolons are not needed in the logic test queries.

Done.


pkg/sql/sqlbase/prepared_statement.go, line 44 at r1 (raw file):

Previously, yuzefovich wrote…

[nit]: I feel like preposition at is missing at the end.

don't feel strongly but i think both are okay.


pkg/sql/sqlbase/prepared_statement.go, line 67 at r1 (raw file):

Previously, yuzefovich wrote…

I'd add use unsafe.Sizeof(time.Time{}) as the size of a single CreatedAt (which should evaluate to 24 bytes). time.Time has a pointer to time.Location, and I feel like that actual time.Location struct is cached and is shared among different timestamps. Feel free to dig in and figure that out :)

I don't think we need to, as res := int64(unsafe.Sizeof(*pm)) should take care of it
see https://play.golang.org/p/ysgmEWFvAwa

trusting you on the cachedness :P

@otan otan force-pushed the otan-cockroach:otan-virtual_table branch from 00cadd2 to 0c53933 Oct 30, 2019
@otan otan requested review from yuzefovich and arulajmani Oct 30, 2019
Copy link
Collaborator

arulajmani left a comment

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


pkg/sql/pg_catalog.go, line 1809 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

yes, this is in a loop.

My bad, I missed the loop.

@otan otan force-pushed the otan-cockroach:otan-virtual_table branch 3 times, most recently from 2a8e7a8 to 8f638f1 Oct 31, 2019
@otan otan requested a review from arulajmani Oct 31, 2019
Copy link
Contributor

yuzefovich left a comment

:lgtm:

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @mjibson, @otan, and @yuzefovich)


pkg/sql/prepared_stmt.go, line 104 at r2 (raw file):

type preparedStatementsAccessor interface {
	// List returns all prepared statements as a map keyed by name.
	// The map itself is a copy the prepared statements.

[nit]: s/copy/copy of/g.


pkg/sql/logictest/testdata/logic_test/pg_catalog, line 1859 at r2 (raw file):


statement ok
DROP TABLE types;

[super nit]: last semicolon :)

Release note (sql change): introduces pg_prepared_statements table for
use
@otan otan force-pushed the otan-cockroach:otan-virtual_table branch from 8f638f1 to af0f58b Oct 31, 2019
@otan

This comment has been minimized.

Copy link
Collaborator Author

otan commented Oct 31, 2019

bors r+

craig bot pushed a commit that referenced this pull request Oct 31, 2019
42016: colexec: add support for IS NULL and IS NOT NULL r=yuzefovich a=yuzefovich

This commit adds IS NULL projection and selection operators which
adds support for IS NULL and IS NOT NULL operators by the vectorized
engine.

Release note: None

42018: sql: implement pg_prepared_statements virtual table r=otan a=otan

* resolves #41778
* add List() to the prepared statements accessor
* implemented pg_prepared_statements, with some fun caveats
   * i think we always use wire format, so from_sql is always false?
   * had to add an extra data type for CreatedAt, not sure how to increment SizeOf here with the location variable set.
   * other caveats are documented as a comment.

Release note (sql change): introduces pg_prepared_statements table for use

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Oct 31, 2019

Build succeeded

@craig craig bot merged commit af0f58b into cockroachdb:master Oct 31, 2019
3 checks passed
3 checks passed
GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.