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: Added new tables and columns in pg_catalog #60758

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Feb 18, 2021

Taking over #59498 since @mnovelodou is out and we want to get this in before stability.

Previously, added tables and columns existed in postgres but not cockroach db
This was inadequate because this may cause errors at tools that expects these
columns to exists
To address this, this patch contains the result of diff tool to add what is
missing

Release note (sql change): Added missing tables and columns at pg_catalog
New columns in table pg_attribute:
atthasmissing
New columns in table pg_class:
relrowsecurity
relforcerowsecurity
relispartition
relispopulated
relreplident
relrewrite
New columns in table pg_collation:
collprovider
collversion
collisdeterministic
New columns in table pg_index:
indnkeyatts
New columns in table pg_proc:
prokind
prosupport
New columns in table pg_aggregate:
aggmfinalmodify
aggfinalmodify
New Tables:
pg_replication_origin_roname_index
pg_config
pg_largeobject_loid_pn_index
pg_ts_config
pg_operator_oid_index
pg_conversion_oid_index
pg_depend_depender_index
pg_stat_user_functions
pg_index_indrelid_index
pg_group
pg_db_role_setting_databaseid_rol_index
pg_statio_user_tables
pg_partitioned_table_partrelid_index
pg_publication_oid_index
pg_publication
pg_publication_tables
pg_stat_xact_user_tables
pg_ts_config_map_index
pg_replication_origin_roiident_index
pg_stat_progress_vacuum
pg_file_settings
pg_statio_all_indexes
pg_shseclabel_object_index
pg_enum_oid_index
pg_statistic_relid_att_inh_index
pg_largeobject_metadata_oid_index
pg_stat_archiver
pg_statio_sys_sequences
pg_collation_oid_index
pg_subscription
pg_namespace_oid_index
pg_amop
pg_stat_database_conflicts
pg_tablespace_oid_index
pg_class_oid_index
pg_range_rngtypid_index
pg_description_o_c_o_index
pg_opclass_oid_index
pg_sequence_seqrelid_index
pg_trigger_oid_index
pg_timezone_abbrevs
pg_ts_parser_oid_index
pg_transform
pg_extension_oid_index
pg_statio_user_sequences
pg_shdepend_reference_index
pg_foreign_data_wrapper_oid_index
pg_ts_config_oid_index
pg_ts_dict_oid_index
pg_init_privs_o_c_o_index
pg_user_mappings
pg_default_acl_role_nsp_obj_index
pg_stat_slru
pg_constraint_contypid_index
pg_stat_progress_create_index
pg_transform_type_lang_index
pg_authid_oid_index
pg_shmem_allocations
pg_statio_sys_tables
pg_statio_all_sequences
pg_policy_oid_index
pg_shdepend_depender_index
pg_attribute_relid_attnum_index
pg_event_trigger_oid_index
pg_amproc
pg_cast_oid_index
pg_constraint_conparentid_index
pg_statio_sys_indexes
pg_conversion_default_index
pg_statistic_ext
pg_shadow
pg_trigger_tgconstraint_index
pg_ts_template
pg_cast_source_target_index
pg_type_oid_index
pg_amproc_fam_proc_index
pg_aggregate_fnoid_index
pg_stat_progress_basebackup
pg_default_acl_oid_index
pg_foreign_server_oid_index
pg_statistic_ext_data_stxoid_index
pg_transform_oid_index
pg_language_oid_index
pg_db_role_setting
pg_amop_opr_fam_index
pg_user_mapping_oid_index
pg_hba_file_rules
pg_am_oid_index
pg_statio_all_tables
pg_statio_user_indexes
pg_stat_progress_analyze
pg_replication_origin
pg_depend_reference_index
pg_stat_bgwriter
pg_attrdef_adrelid_adnum_index
pg_stat_sys_indexes
pg_statistic_ext_oid_index
pg_foreign_table_relid_index
pg_user_mapping_user_server_index
pg_seclabel_object_index
pg_subscription_rel_srrelid_srsubid_index
pg_publication_rel_prrelid_prpubid_index
pg_inherits_parent_index
pg_ts_dict
pg_stat_user_tables
pg_stat_progress_cluster
pg_stat_xact_all_tables
pg_stat_database
pg_shdescription_o_c_index
pg_publication_rel_oid_index
pg_largeobject
pg_publication_rel
pg_rewrite_oid_index
pg_stat_all_indexes
pg_amop_fam_strat_index
pg_proc_oid_index
pg_database_oid_index
pg_sequences
pg_subscription_oid_index
pg_enum_typid_sortorder_index
pg_ts_parser
pg_ts_config_map
pg_attrdef_oid_index
pg_timezone_names
pg_ts_template_oid_index
pg_statistic_ext_relid_index
pg_index_indexrelid_index
pg_amop_oid_index
pg_amproc_oid_index
pg_rules
pg_opfamily
pg_stat_xact_sys_tables
pg_policies
pg_constraint_oid_index
pg_stat_sys_tables
pg_stat_xact_user_functions
pg_available_extension_versions
pg_stat_all_tables
pg_auth_members_role_member_index
pg_auth_members_member_role_index
pg_inherits_relid_seqno_index
pg_opfamily_oid_index
pg_class_tblspc_relfilenode_index
pg_cursors
pg_stat_gssapi
pg_stat_ssl
pg_stat_user_indexes

Fixes #58001

@RichardJCai RichardJCai requested review from rafiss and a team February 18, 2021 22:23
@RichardJCai RichardJCai requested a review from a team as a code owner February 18, 2021 22:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RichardJCai RichardJCai force-pushed the add_missing_pg_catalog_tables_cols_rcai_02182021 branch from f3363d0 to 114545c Compare February 18, 2021 22:26
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

just a few nits

pkg/sql/vtable/pg_catalog.go Outdated Show resolved Hide resolved
pkg/sql/vtable/pg_catalog.go Show resolved Hide resolved
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
@rafiss
Copy link
Collaborator

rafiss commented Feb 19, 2021

won't we need to update the diff tool test to account for these new columns?

Copy link
Contributor Author

@RichardJCai RichardJCai 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 @rafiss and @RichardJCai)


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

Previously, rafiss (Rafi Shamim) wrote…

nit: since this message is user-visible, maybe we should just say "was created for compatibility"

Gotcha


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

Previously, rafiss (Rafi Shamim) wrote…

do you know why relpartbound isn't added here? as described on https://www.postgresql.org/docs/13/catalog-pg-class.html

I think it's because we don't support the pg_node_tree type, columns aren't added if we don't support the type.

@RichardJCai RichardJCai force-pushed the add_missing_pg_catalog_tables_cols_rcai_02182021 branch 2 times, most recently from 475182d to 9288d07 Compare February 19, 2021 16:13
@RichardJCai
Copy link
Contributor Author

won't we need to update the diff tool test to account for these new columns?

Updated the expected_diff_json

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm once tests are passing

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


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

Previously, RichardJCai (Richard Cai) wrote…

I think it's because we don't support the pg_node_tree type, columns aren't added if we don't support the type.

ah interesting. fair enough. i see that we do have some existing tables where we have a column that should be pg_node_tree, but we just use string. e.g. https://www.postgresql.org/docs/9.1/catalog-pg-index.html

i think in a follow up PR we should maybe add the pg_node_tree type, since it's just an alias for string. or if not that, maybe in a follow up PR we could manually add the relpartbound column since we know of at least one tool that will rely on it: https://cockroachlabs.slack.com/archives/CPD3ANLMB/p1613690899072100

Previously, added tables and columns existed in postgres but not cockroach db
This was inadequate because this may cause errors at tools that expects these
columns to exists
To address this, this patch contains the result of diff tool to add what is
missing

Release note (sql change): Added missing tables and columns at pg_catalog
New columns in table pg_attribute:
        atthasmissing
New columns in table pg_class:
        relrowsecurity
        relforcerowsecurity
        relispartition
        relispopulated
        relreplident
        relrewrite
New columns in table pg_collation:
        collprovider
        collversion
        collisdeterministic
New columns in table pg_index:
        indnkeyatts
New columns in table pg_proc:
        prokind
        prosupport
New columns in table pg_aggregate:
        aggmfinalmodify
        aggfinalmodify
New Tables:
        pg_replication_origin_roname_index
        pg_config
        pg_largeobject_loid_pn_index
        pg_ts_config
        pg_operator_oid_index
        pg_conversion_oid_index
        pg_depend_depender_index
        pg_stat_user_functions
        pg_index_indrelid_index
        pg_group
        pg_db_role_setting_databaseid_rol_index
        pg_statio_user_tables
        pg_partitioned_table_partrelid_index
        pg_publication_oid_index
        pg_publication
        pg_publication_tables
        pg_stat_xact_user_tables
        pg_ts_config_map_index
        pg_replication_origin_roiident_index
        pg_stat_progress_vacuum
        pg_file_settings
        pg_statio_all_indexes
        pg_shseclabel_object_index
        pg_enum_oid_index
        pg_statistic_relid_att_inh_index
        pg_largeobject_metadata_oid_index
        pg_stat_archiver
        pg_statio_sys_sequences
        pg_collation_oid_index
        pg_subscription
        pg_namespace_oid_index
        pg_amop
        pg_stat_database_conflicts
        pg_tablespace_oid_index
        pg_class_oid_index
        pg_range_rngtypid_index
        pg_description_o_c_o_index
        pg_opclass_oid_index
        pg_sequence_seqrelid_index
        pg_trigger_oid_index
        pg_timezone_abbrevs
        pg_ts_parser_oid_index
        pg_transform
        pg_extension_oid_index
        pg_statio_user_sequences
        pg_shdepend_reference_index
        pg_foreign_data_wrapper_oid_index
        pg_ts_config_oid_index
        pg_ts_dict_oid_index
        pg_init_privs_o_c_o_index
        pg_user_mappings
        pg_default_acl_role_nsp_obj_index
        pg_stat_slru
        pg_constraint_contypid_index
        pg_stat_progress_create_index
        pg_transform_type_lang_index
        pg_authid_oid_index
        pg_shmem_allocations
        pg_statio_sys_tables
        pg_statio_all_sequences
        pg_policy_oid_index
        pg_shdepend_depender_index
        pg_attribute_relid_attnum_index
        pg_event_trigger_oid_index
        pg_amproc
        pg_cast_oid_index
        pg_constraint_conparentid_index
        pg_statio_sys_indexes
        pg_conversion_default_index
        pg_statistic_ext
        pg_shadow
        pg_trigger_tgconstraint_index
        pg_ts_template
        pg_cast_source_target_index
        pg_type_oid_index
        pg_amproc_fam_proc_index
        pg_aggregate_fnoid_index
        pg_stat_progress_basebackup
        pg_default_acl_oid_index
        pg_foreign_server_oid_index
        pg_statistic_ext_data_stxoid_index
        pg_transform_oid_index
        pg_language_oid_index
        pg_db_role_setting
        pg_amop_opr_fam_index
        pg_user_mapping_oid_index
        pg_hba_file_rules
        pg_am_oid_index
        pg_statio_all_tables
        pg_statio_user_indexes
        pg_stat_progress_analyze
        pg_replication_origin
        pg_depend_reference_index
        pg_stat_bgwriter
        pg_attrdef_adrelid_adnum_index
        pg_stat_sys_indexes
        pg_statistic_ext_oid_index
        pg_foreign_table_relid_index
        pg_user_mapping_user_server_index
        pg_seclabel_object_index
        pg_subscription_rel_srrelid_srsubid_index
        pg_publication_rel_prrelid_prpubid_index
        pg_inherits_parent_index
        pg_ts_dict
        pg_stat_user_tables
        pg_stat_progress_cluster
        pg_stat_xact_all_tables
        pg_stat_database
        pg_shdescription_o_c_index
        pg_publication_rel_oid_index
        pg_largeobject
        pg_publication_rel
        pg_rewrite_oid_index
        pg_stat_all_indexes
        pg_amop_fam_strat_index
        pg_proc_oid_index
        pg_database_oid_index
        pg_sequences
        pg_subscription_oid_index
        pg_enum_typid_sortorder_index
        pg_ts_parser
        pg_ts_config_map
        pg_attrdef_oid_index
        pg_timezone_names
        pg_ts_template_oid_index
        pg_statistic_ext_relid_index
        pg_index_indexrelid_index
        pg_amop_oid_index
        pg_amproc_oid_index
        pg_rules
        pg_opfamily
        pg_stat_xact_sys_tables
        pg_policies
        pg_constraint_oid_index
        pg_stat_sys_tables
        pg_stat_xact_user_functions
        pg_available_extension_versions
        pg_stat_all_tables
        pg_auth_members_role_member_index
        pg_auth_members_member_role_index
        pg_inherits_relid_seqno_index
        pg_opfamily_oid_index
        pg_class_tblspc_relfilenode_index
        pg_cursors
        pg_stat_gssapi
        pg_stat_ssl
        pg_stat_user_indexes
@RichardJCai RichardJCai force-pushed the add_missing_pg_catalog_tables_cols_rcai_02182021 branch from 9288d07 to 612b83d Compare February 19, 2021 18:18
@RichardJCai
Copy link
Contributor Author

Thanks for the review!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Feb 19, 2021

Build succeeded:

@jordanlewis
Copy link
Member

jordanlewis commented Dec 20, 2021

I'm noticing that the tables that were added have the columns out of order. If people use SELECT * on system catalogs (not that they should) they'll get columns out of order.

I think we should fix this. Can we enhance the diff tool to take into account ordering of columns?

For example, pg_cursors was defined like this:

CREATE TABLE pg_catalog.pg_cursors (
	is_scrollable BOOL,
	name STRING,
	statement STRING,
	creation_time TIMESTAMPTZ,
	is_binary BOOL,
	is_holdable BOOL
)

But, to be compatible with Postgres, the order should be name, statement, is_holdable, is_binary, is_scrollable, creation_time.

@rafiss
Copy link
Collaborator

rafiss commented Dec 20, 2021

I think we did this kind of intentionally for tables where we modified an existing definition -- if people are using SELECT * (which we agree they shouldn't), this means they are depending on the existing order of the the columns.

But yeah, seems like for the ones where we introduced a new table, we should match the order. This seems quite safe to do for the empty tables, so I'm in favor. Are you making an issue? Mind making an issue?

@jordanlewis
Copy link
Member

Opened #74101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: create empty pg_catalog tables that are missing
4 participants