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

[6x] Dump ALTER statements for child tables with different schemas #14962

Merged

Conversation

kyeap-vmware
Copy link
Contributor

Dump ALTER statements for child tables with different schemas

ALTER TABLE name SET SCHEMA statements are dumped to support restore and
upgrade of child partitions whose schemas are different than their
parents. In binary upgrade mode, child partition oids are reserved using the
parent's schema because that is where they initially land on creation.

pipeline: https://cm.ci.gpdb.pivotal.io/teams/main/pipelines/gpdb-cm-kyeap-vmware?group=all

@@ -26,7 +26,6 @@ static void check_gphdfs_external_tables(void);
static void check_gphdfs_user_roles(void);
static void check_unique_primary_constraint(void);
static void check_for_array_of_partition_table_types(ClusterInfo *cluster);
static void check_partition_schemas(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Need to update the public facing docs for "Schemas on partitioned tables" as this is now removed.

src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
src/bin/pg_dump/pg_dump.c Outdated Show resolved Hide resolved
ALTER TABLE name SET SCHEMA statements are dumped to support restore and
upgrade of child partitions whose schemas are different than their
parents. In binary upgrade mode, child partition oids are reserved using the
parent's schema because that is where they initially land on creation.
Pg_dump now dumps ALTER TABLE name SET SCHEMA statements for child
partitions. This means partition tables containing child partitions that
belong to a schema different than it's parent can be dumped, restored,
and upgraded. Remove the pg_upgrade check looking for this.
@ashwinstar
Copy link
Contributor

ashwinstar commented Feb 28, 2023

Can't the change be validated in pg_upgrade test which run as part of ICW test itself if add the partition table with different schema object type as part of some test in ICW?

@jimmyyih
Copy link
Member

jimmyyih commented Mar 2, 2023

Can't the change be validated in pg_upgrade test which run as part of ICW test itself if add the partition table with different schema object type as part of some test in ICW?

Kinda, but we would have to modify and implement new testing logic in the pg_upgrade CI job then to do the full end-to-end testing of this change. Without this PR, it's a logical error where pg_upgrade would succeed and users would notice only later in post-upgrade testing of applications hitting those partitions. The pg_upgrade CI job only tests if pg_upgrade fails or succeeds. As part of the gpupgrade repository, post-upgrade testing is done to make sure objects are logically correct and still work as expected (e.g. post-upgrade query if the partition is still in the different schema and is usable). That's the divide between the GPDB and gpupgrade repositories in terms of testing. I think that's fine for now.

@kalensk
Copy link
Contributor

kalensk commented Mar 2, 2023

The pg_upgrade CI job only tests if pg_upgrade fails or succeeds. As part of the gpupgrade repository, post-upgrade testing is done to make sure objects are logically correct and still work as expected (e.g. post-upgrade query if the partition is still in the different schema and is usable). That's the divide between the GPDB and gpupgrade repositories in terms of testing. I think that's fine for now.

To add a bit more context. There's a bit of a "chicken and egg" issue in terms of testing in the GPDB repo. That is, gpupgrade is independent of the GPDB repo and is not a dependency of GPDB. Because of this the pg_upgrade job in the GPDB CI basically does a crude implementation of gpupgrade in BASH which is "okay". Ideally we would use gpupgrade itself, but doing so adds a bit of a circular dependency.

The pg_upgrade acceptance tests in the gpupgrade repo leverage gpupgrade to easily upgrade the cluster and expand on these tests.

With much more thought and prototyping I am sure there are opportunities to improve pg_upgrade testing in the GPDB repo itself for faster and better feedback.

@ashwinstar
Copy link
Contributor

Main aspect I am hearing is pg_upgrade test in GPDB uses pg_dump to perform the validation which itself is broken and hence insufficient to help validate the fix. This clearly showcases the testing/validation gap which exists in GPDB repository. Thanks.

@kyeap-vmware kyeap-vmware merged commit 0693e15 into greenplum-db:6X_STABLE Mar 3, 2023
@kyeap-vmware kyeap-vmware deleted the partition-schema-alter branch March 3, 2023 03:05
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.

None yet

4 participants