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: pg_sequences implemented on pg_catalog #65420

Merged
merged 1 commit into from
May 20, 2021

Conversation

mnovelodou
Copy link
Contributor

@mnovelodou mnovelodou commented May 18, 2021

fixes #65522

Previously, pg_sequences was added for compatiblity but not implemented
This was inadequate because these tables return values in postgres
To address this, this patch implements pg_sequences

Release note (sql change): pg_sequences table was implemented on pg_catalog

@blathers-crl
Copy link

blathers-crl bot commented May 18, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels May 18, 2021
@mnovelodou mnovelodou requested a review from rafiss May 18, 2021 18:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl
Copy link

blathers-crl bot commented May 18, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

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


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

				if sequenceValue != opts.Start-opts.Increment {
					// Before using for the first time, sequenceValue will be:
					// opts.Start - opts.Increment.

nit: comment should be above the if block


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

					tree.NewDString(scName),                 // schemaname
					tree.NewDString(table.GetName()),        // sequencename
					getOwnerName(table),                     // sequenceowner

Should this be the sequence owner which is on the sequence opts? Could you check against postgres?

Also, if I'm correct, it would be worth adding a test which sets a sequence owner.


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

SELECT * FROM pg_seclabel

statement ok

Could you add a test that sets the CACHE option as well?

Previously, pg_sequences was added for compatiblity but not implemented
This was inadequate because these tables return values in postgres
To address this, this patch implements pg_sequences

Release note (sql change): pg_sequences table was implemented on pg_catalog

Fixes cockroachdb#65522
Copy link
Contributor Author

@mnovelodou mnovelodou left a comment

Choose a reason for hiding this comment

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

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


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

Previously, arulajmani (Arul Ajmani) wrote…

nit: comment should be above the if block

Done.


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

Previously, arulajmani (Arul Ajmani) wrote…

Should this be the sequence owner which is on the sequence opts? Could you check against postgres?

Also, if I'm correct, it would be worth adding a test which sets a sequence owner.

Nice catch!

I took a look at postgres and seems like there are 2 different things:
"OWNED BY" and "OWNER", You can ALTER SEQUENCE and change both values.
The first one (OWNED BY) refers to a table.column which in case the column or the entire table gets deleted, the sequence will get deleted as well. sequence opts owner refers to this table.column:

type TableDescriptor_SequenceOpts_SequenceOwner struct {
// Sequence Owner's Column ID
OwnerColumnID ColumnID protobuf:"varint,1,opt,name=owner_column_id,json=ownerColumnId,casttype=ColumnID" json:"owner_column_id"
// Sequence Owner's Table ID
OwnerTableID ID protobuf:"varint,2,opt,name=owner_table_id,json=ownerTableId,casttype=ID" json:"owner_table_id"
}

Owner refers to the username that owns the sequence. Which in this case is correct, but I will add a test case that will verify this.

Thank you for raising this up!


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

Previously, arulajmani (Arul Ajmani) wrote…

Could you add a test that sets the CACHE option as well?

Done.

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @mnovelodou, and @rafiss)


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

Previously, mnovelodou (Miguel Novelo) wrote…

Nice catch!

I took a look at postgres and seems like there are 2 different things:
"OWNED BY" and "OWNER", You can ALTER SEQUENCE and change both values.
The first one (OWNED BY) refers to a table.column which in case the column or the entire table gets deleted, the sequence will get deleted as well. sequence opts owner refers to this table.column:

type TableDescriptor_SequenceOpts_SequenceOwner struct {
// Sequence Owner's Column ID
OwnerColumnID ColumnID protobuf:"varint,1,opt,name=owner_column_id,json=ownerColumnId,casttype=ColumnID" json:"owner_column_id"
// Sequence Owner's Table ID
OwnerTableID ID protobuf:"varint,2,opt,name=owner_table_id,json=ownerTableId,casttype=ID" json:"owner_table_id"
}

Owner refers to the username that owns the sequence. Which in this case is correct, but I will add a test case that will verify this.

Thank you for raising this up!

Thanks for confirming!

@arulajmani
Copy link
Collaborator

bors r+

@craig
Copy link
Contributor

craig bot commented May 20, 2021

Build succeeded:

@craig craig bot merged commit 28c81ff into cockroachdb:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: populate pg_catalog.pg_sequences
3 participants