sql: populated pg_depend with table and view dependencies #57240

mnovelodou · 2020-11-30T19:19:10Z

Previously, pg_depend only contained dependencies from foreign keys,
but this table in postgres also cointains dependencies from table-views,
To address this, this patch populates this dependency information

Fixes #56115

Release note (sql change): added table-view dependency information
in pg_depend to improve compatibility with postgresql.

blathers-crl · 2020-11-30T19:19:14Z

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 have added a few people who may be able to assist in reviewing:

@jayshrivastava (author of sql: store dependencies in pg_depend #56115)

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

cockroach-teamcity · 2020-11-30T19:19:17Z

This change is

blathers-crl · 2020-12-02T22:49:44Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

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

blathers-crl · 2020-12-02T23:08:57Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

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

rafiss

looks pretty good! i have a few requests for testing and commenting

rafiss · 2020-12-02T22:28:22Z

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -992,6 +1002,35 @@ JOIN pg_constraint ON objid=pg_constraint.oid AND refobjid=pg_constraint.conindi
 contype
 f

+# Testing table-view dependencies in pg_depend, This query will not work the same way
+# In PostgreSQL as pg_depend.classid points to pg_rewrite then in pg_rewrite


I think these Postgres docs also say that pg_depend.classid points directly to pg_class.oid
https://www.postgresql.org/docs/current/catalog-pg-depend.html

but it looks like in Postgres, pg_depend.objid points to pg_rewrite.ev_class. can you update this comment?

i looked into this further.. i think soon we should try to populate pg_rewrite. i created #57417 -- can you include this link in the comment?

rafiss · 2020-12-02T22:29:03Z

pkg/sql/logictest/testdata/logic_test/pg_catalog

+# pg_rewrite yet.
+
+statement ok
+CREATE TABLE source_table(a INT, b INT, c INT);


can you make this CREATE TABLE source_table(a INT PRIMARY KEY, b INT, c INT);

then i think fewer other tests will be affected

rafiss · 2020-12-02T23:21:36Z

pkg/sql/pg_catalog.go

@@ -1153,6 +1153,31 @@ https://www.postgresql.org/docs/9.5/catalog-pg-depend.html`,
 					depTypeAuto,          // deptype
 				)
 			}
+
+			reportViewDependency := func(dep *descpb.TableDescriptor_Reference) error {


how does this code handle views that depend on other views? can you add a test for that?

The loop already pick views and descriptor already have dependencies at other views, but this is a nice catch, I tried it and it works, I am going to add a test case for this as well.

rafiss · 2020-12-02T23:22:36Z

pkg/sql/logictest/testdata/logic_test/pg_catalog

+CREATE TABLE source_table(a INT, b INT, c INT);
+CREATE VIEW depend_view AS SELECT b, c FROM source_table;
+
+query TTTTT


can you make this query TTTTT colnames so that the output includes the column names?

rafiss · 2020-12-02T23:23:20Z

pkg/sql/logictest/testdata/logic_test/pg_catalog

+SELECT
+     depend_cat_class.relname depend_catalog,
+	   source_cat_class.relname source_catalog,
+	   depend_class.relname depend,


can you also check for source_class.relname?

rafiss · 2020-12-02T23:26:55Z

pkg/sql/pg_catalog.go

+			reportViewDependency := func(dep *descpb.TableDescriptor_Reference) error {
+				for _, colID := range dep.ColumnIDs {
+					if err := addRow(
+						pgClassTableOid,                //classid


can you also add a comment here that described how we don't use pg_rewrite (and link to issue #57417)

rafiss · 2020-12-02T23:34:43Z

I noticed that you merged master into this branch. the recommended way to update the PR is to rebase your branch on top of master, then force push. that way the git history is easier to read. for example:

git pull --rebase origin master
git push <your remote name> --force pg-depend-views

(in this case you should also remove your merge commit too)

blathers-crl · 2020-12-03T21:26:12Z

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.}

rafiss

looks good!

bors r+

Reviewed 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

craig · 2020-12-10T21:33:34Z

Merge conflict.

rafiss · 2020-12-10T21:33:58Z

bors r-

@mnovelodou i just noticed there is a merge conflict. can you please update it when you have a chance?

rafiss

marking as requested change

Previously, pg_depend only contained dependencies from foreign keys, but this table in postgres also cointains dependencies from table-views, To address this, this patch populates this dependency information Fixes cockroachdb#56115 Release note (sql change): added table-view dependency information in pg_depend to improve compatibility with postgresql.

rafiss

looks good!

bors r+

craig · 2020-12-11T17:36:39Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Nov 30, 2020

blathers-crl bot requested a review from jayshrivastava November 30, 2020 19:19

rafiss requested review from rafiss and removed request for jayshrivastava December 1, 2020 18:08

mnovelodou force-pushed the pg-depend-views branch from 5e023ff to 4ab0d7d Compare December 2, 2020 23:08

mnovelodou force-pushed the pg-depend-views branch from 4ab0d7d to d909bea Compare December 2, 2020 23:14

rafiss requested changes Dec 2, 2020

View reviewed changes

mnovelodou force-pushed the pg-depend-views branch from d909bea to 9f6faf8 Compare December 3, 2020 21:26

rafiss approved these changes Dec 10, 2020

View reviewed changes

rafiss requested changes Dec 10, 2020

View reviewed changes

mnovelodou force-pushed the pg-depend-views branch from 9f6faf8 to c723c40 Compare December 10, 2020 23:46

mnovelodou requested a review from rafiss December 11, 2020 15:10

rafiss approved these changes Dec 11, 2020

View reviewed changes

rafiss removed the O-community Originated from the community label Dec 11, 2020

craig bot merged commit 2b897f3 into cockroachdb:master Dec 11, 2020

jseldess mentioned this pull request Jan 29, 2021

sql: populated pg_depend with table and view dependencies cockroachdb/docs#9578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: populated pg_depend with table and view dependencies #57240

sql: populated pg_depend with table and view dependencies #57240

mnovelodou commented Nov 30, 2020

blathers-crl bot commented Nov 30, 2020

cockroach-teamcity commented Nov 30, 2020

blathers-crl bot commented Dec 2, 2020

blathers-crl bot commented Dec 2, 2020

rafiss left a comment

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

mnovelodou Dec 3, 2020

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

rafiss Dec 2, 2020

mnovelodou Dec 3, 2020

rafiss commented Dec 2, 2020

blathers-crl bot commented Dec 3, 2020

rafiss left a comment

craig bot commented Dec 10, 2020

rafiss commented Dec 10, 2020

rafiss left a comment

rafiss left a comment

craig bot commented Dec 11, 2020

sql: populated pg_depend with table and view dependencies #57240

sql: populated pg_depend with table and view dependencies #57240

Conversation

mnovelodou commented Nov 30, 2020

blathers-crl bot commented Nov 30, 2020

cockroach-teamcity commented Nov 30, 2020

blathers-crl bot commented Dec 2, 2020

blathers-crl bot commented Dec 2, 2020

rafiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss commented Dec 2, 2020

blathers-crl bot commented Dec 3, 2020

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Dec 10, 2020

rafiss commented Dec 10, 2020

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Dec 11, 2020