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.

Sign up for GitHub

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

Jump to bottom

[YSQL] add support for dropping a key column #22902

Closed
1 task done
fizaaluthra opened this issue Jun 18, 2024 · 0 comments
Closed
1 task done

[YSQL] add support for dropping a key column #22902

fizaaluthra opened this issue Jun 18, 2024 · 0 comments
Assignees
@fizaaluthra
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue

Comments

@fizaaluthra
Copy link
Member

fizaaluthra commented Jun 18, 2024 •
edited by jira bot
Loading

Jira Link: DB-11810

Description

Requires table rewrite.

Issue Type

kind/enhancement

Warning: Please confirm that this issue does not contain any sensitive information

  • I confirm this issue does not contain any sensitive information.
@fizaaluthra fizaaluthra added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Jun 18, 2024
@yugabyte-ci yugabyte-ci added kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue labels Jun 18, 2024
@fizaaluthra fizaaluthra self-assigned this Jun 18, 2024
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Jun 18, 2024
fizaaluthra added a commit that referenced this issue Jul 4, 2024
@fizaaluthra
[#23094] YSQL: Fix flaky test TestPgRegressFeaturePartition
b81dbaa 
Summary:
The test became flaky after commit 7c8343d fixed the logic for cascaded drops on columns.

The test yb_feature_hash_types creates a domain and creates a table with a key column that has the domain as its type. However, since we currently don't support key column drops (#22902), the test fails when the cleanUpCustomEntities() function in BasePgSQLTest.java attempts to drop the domain (that has the key column as a dependant object) during cleanup.
Moreover, the test is flaky because sometimes the table may be dropped before the domain, as the order of the drops depends on the pg_depend scan output, which isn't deterministic because pg_depend doesn't have a primary key and internally uses ybctid.

Fix the test flakiness by dropping the table in yb_feature_hash_types before the test clean up logic.
Jira: DB-12029

Test Plan: ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressFeaturePartition' -n 20

Reviewers: myang

Reviewed By: myang

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D36329
fizaaluthra added a commit that referenced this issue Jul 12, 2024
@fizaaluthra
[BACKPORT 2024.1][#22874] YSQL: Fix cascaded drops on columns
3bcaf41 
Summary:
Commit 7c8343d / D35866 summary:

Currently, when an object that a column depends on is dropped, the drop isn't
correctly cascaded to the column in the DocDB table itself.

Example:

```
yugabyte=# CREATE TABLE test_dropcolumn(a int, b int, c int);
CREATE TABLE
yugabyte=# CREATE TYPE test_dropcolumn_type AS (a int, b int);
CREATE TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d test_dropcolumn_type;
ALTER TABLE
yugabyte=# DROP TYPE test_dropcolumn_type CASCADE;
NOTICE:  drop cascades to column t of table person
DROP TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d int;
ERROR:  The column already exists: d
```
Although the PG metadata for the column is dropped, the column itself is not
dropped in DocDB. This leads to an inconsistency between DocDB and PG.

This diff fixes the issue by adding a YB drop for the column inside `doDeletion`.
Note: this function is also used during `ALTER TABLE... DROP COLUMN`; however,
alter table commands execute YB schema changes separately (see `YBCPrepareAlterTableCmd`
and `YBCExecAlterTable`). So, an additional flag is added to skip the YB column
drop in `doDeletion` in case it is being invoked by the alter table flow.
Jira: DB-11776

Commit b81dbaa / D36329 summary:

The test became flaky after commit 7c8343d fixed the logic for cascaded drops on columns.

The test yb_feature_hash_types creates a domain and creates a table with a key column that has the domain as its type. However, since we currently don't support key column drops (#22902), the test fails when the cleanUpCustomEntities() function in BasePgSQLTest.java attempts to drop the domain (that has the key column as a dependant object) during cleanup.
Moreover, the test is flaky because sometimes the table may be dropped before the domain, as the order of the drops depends on the pg_depend scan output, which isn't deterministic because pg_depend doesn't have a primary key and internally uses ybctid.

Fix the test flakiness by dropping the table in yb_feature_hash_types before the test clean up logic.
Jira: DB-12029

Original commits:
7c8343d / D35866
b81dbaa / D36329

Test Plan: yb_alter_table

Reviewers: myang

Reviewed By: myang

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36286
fizaaluthra added a commit that referenced this issue Jul 12, 2024
@fizaaluthra
[BACKPORT 2.20][#22874] YSQL: Fix cascaded drops on columns
15344d2 
Summary:
Commit 7c8343d / D35866 summary:

Currently, when an object that a column depends on is dropped, the drop isn't
correctly cascaded to the column in the DocDB table itself.

Example:

yugabyte=# CREATE TABLE test_dropcolumn(a int, b int, c int);
CREATE TABLE
yugabyte=# CREATE TYPE test_dropcolumn_type AS (a int, b int);
CREATE TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d test_dropcolumn_type;
ALTER TABLE
yugabyte=# DROP TYPE test_dropcolumn_type CASCADE;
NOTICE:  drop cascades to column t of table person
DROP TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d int;
ERROR:  The column already exists: d
Although the PG metadata for the column is dropped, the column itself is not
dropped in DocDB. This leads to an inconsistency between DocDB and PG.

This diff fixes the issue by adding a YB drop for the column inside doDeletion.
Note: this function is also used during ALTER TABLE... DROP COLUMN; however,
alter table commands execute YB schema changes separately (see YBCPrepareAlterTableCmd
and YBCExecAlterTable). So, an additional flag is added to skip the YB column
drop in doDeletion in case it is being invoked by the alter table flow.
Jira: DB-11776

Commit b81dbaa / D36329 summary:

The test became flaky after commit 7c8343d fixed the logic for cascaded drops on columns.

The test yb_feature_hash_types creates a domain and creates a table with a key column that has the domain as its type. However, since we currently don't support key column drops (#22902), the test fails when the cleanUpCustomEntities() function in BasePgSQLTest.java attempts to drop the domain (that has the key column as a dependant object) during cleanup.
Moreover, the test is flaky because sometimes the table may be dropped before the domain, as the order of the drops depends on the pg_depend scan output, which isn't deterministic because pg_depend doesn't have a primary key and internally uses ybctid.

Fix the test flakiness by dropping the table in yb_feature_hash_types before the test clean up logic.
Jira: DB-12029

Original commits:
7c8343d / D35866
b81dbaa / D36329

Test Plan: yb_alter_table

Reviewers: myang

Reviewed By: myang

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36289
fizaaluthra added a commit that referenced this issue Jul 12, 2024
@fizaaluthra
[BACKPORT 2.18][#22874] YSQL: Fix cascaded drops on columns
9fbda29 
Summary:
Commit 7c8343d / D35866 summary:

Currently, when an object that a column depends on is dropped, the drop isn't
correctly cascaded to the column in the DocDB table itself.

Example:

```
yugabyte=# CREATE TABLE test_dropcolumn(a int, b int, c int);
CREATE TABLE
yugabyte=# CREATE TYPE test_dropcolumn_type AS (a int, b int);
CREATE TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d test_dropcolumn_type;
ALTER TABLE
yugabyte=# DROP TYPE test_dropcolumn_type CASCADE;
NOTICE:  drop cascades to column t of table person
DROP TYPE
yugabyte=# ALTER TABLE test_dropcolumn ADD COLUMN d int;
ERROR:  The column already exists: d
```
Although the PG metadata for the column is dropped, the column itself is not
dropped in DocDB. This leads to an inconsistency between DocDB and PG.

This diff fixes the issue by adding a YB drop for the column inside `doDeletion`.
Note: this function is also used during `ALTER TABLE... DROP COLUMN`; however,
alter table commands execute YB schema changes separately (see `YBCPrepareAlterTableCmd`
and `YBCExecAlterTable`). So, an additional flag is added to skip the YB column
drop in `doDeletion` in case it is being invoked by the alter table flow.
Jira: DB-11776

Commit b81dbaa / D36329 summary:

The test became flaky after commit 7c8343d fixed the logic for cascaded drops on columns.

The test yb_feature_hash_types creates a domain and creates a table with a key column that has the domain as its type. However, since we currently don't support key column drops (#22902), the test fails when the cleanUpCustomEntities() function in BasePgSQLTest.java attempts to drop the domain (that has the key column as a dependant object) during cleanup.
Moreover, the test is flaky because sometimes the table may be dropped before the domain, as the order of the drops depends on the pg_depend scan output, which isn't deterministic because pg_depend doesn't have a primary key and internally uses ybctid.

Fix the test flakiness by dropping the table in yb_feature_hash_types before the test clean up logic.
Jira: DB-12029

Original commits:
7c8343d / D35866
b81dbaa / D36329

Test Plan: yb_alter_table

Reviewers: myang

Reviewed By: myang

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36290
fizaaluthra added a commit that referenced this issue Jul 15, 2024
@fizaaluthra
[#22902] YSQL: Add support for dropping a key column
19ab966 
Summary:
Add support for dropping a key column using table rewrite.
Note: dropping any key column also drops the primary key on the table.

Code changes:
 - ATExecDropColumn: mark the table and all of its children to be rewritten in phase 3 (for reason YB_AT_REWRITE_ALTER_PRIMARY_KEY) when the column to be dropped is a key column.
 - YBCPrepareAlterTableCmd: skip yb drops for key columns in the alter table execution flow, as the table will be rewritten in alter table phase 3 anyway.
 - YBCDropColumn: note: this function is called when the column is being dropped outside of an alter table flow (see commit 7c8343d).
        - Fix the condition for missing columns: previously, the function would skip over missing columns only when the IF EXISTS clause was present. Without this clause, it would unnecessarily create a drop column handle for a missing column, as the absence of the column is detected later in ATExecDropColumn. Update the condition to always bypass YB alters for missing columns.
        - Since dropping a key column requires a table rewrite in YB, we must appropriately trigger the table rewrite event. Construct a dummy query for this purpose, since we don't have the original query available.
        - Perform the usual ALTER TABLE execution flow by invoking AlterTableInternal().
        - If we are not dropping a key column, we can use the short path (introduced by commit 7c8343d) of simply executing the YB drop column.
Jira: DB-11810

Test Plan: yb_feature_alter_table, yb_alter_table_rewrite

Reviewers: myang

Reviewed By: myang

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D35940
jasonyb pushed a commit that referenced this issue Jul 17, 2024
@yugabyte-ci @jaki
[BACKPORT pg15-cherrypicks] all: Bulk port from master - 55
6c7ea34 
Summary:
 cc63aaf [docs] updates to diagnostics report page (#23162)
 Excluded: 1773ae2 [#22937] docdb: Backward scans: make pggate be aware of fast backward scan capability
 39c6228 [PLAT-12732] toggle tls and cert rotation v2 apis
 d7cf125 [PLAT-14539][xCluster] need_bootstrap API does not work for old universes
 65232ff [PLAT-14654]: Volume Size does not increase when ULTRA storage type is selected
 4b39933 [PLAT-14606]: Disable options to ensure shrinking the RF is not permitted  edit universe scenario
 98d3fed [#23182] YSQL: Fix upgrade test failure when using 2.20.3.1 snapshot
 Excluded: 19ab966 [#22902] YSQL: Add support for dropping a key column
 835e30d [#22479] docdb: Pass epoch through DB cloning calls
 3273e9b [#21789] docdb: Add tablet splitting support for clone
 52f7e79 [#23064] YSQL: pg_partman: disable p_retention_schema parameter
 66ed3a5 [#23197]  YSQL: pg_partman: Disable Gist index creation
 3996f55 [YNP][PLAT-14664] make node register to provider idempotent
 Excluded: 6ec058d [PLAT-14668] - Move YSQL/YCQL configuration RBAC check to universe actions level and integrate RBAC for PG Compatibility
 Excluded: 18bb9b8 [#23034] YSQL: Add Support for OIDC IDP URL (jwt_jwks_url) to fetch and refresh JKWS
 68cb1d2 [PLAT-14675][YNP] Fix the formatting for preflight checks
 db445ce [PLAT-14471][PLAT-14576] Added configurable deadline, keepAlive and unavailable retries to Ybc Java client
 Excluded: 47da28c [#23192] YSQL: Simplify/cleanup code in PgDml/PgSelect/PgSelectIndex etc
 Excluded: 1bc7a50 fix : gflag group toggle issue

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36624
fizaaluthra added a commit that referenced this issue Jul 18, 2024
@fizaaluthra
[BACKPORT pg15-cherrypicks][#22902] YSQL: Add support for dropping a …
f46e0fe 
…key column

Summary:
- tablecmds.c:
  - declarations:
    - master commit 19ab966 adds a new parameter `yb_tab` to the function `ATExecDropColumn`.
    - adjacent lines conflict with upstream PG commit 1281a5c907b41e992a66deb13c3aa61888a62268, which changes the definition of the preceding function ATPrepDropColumn.
- ybccmds.c:
    - YBCDropColumn:
      - master commit 19ab966 adds a call to raw_parser.
      - upstream PG commit 844fe9f159a948377907a63d0ef3fb16dc51ce50 adds a new argument for raw_parser.
      - resolve the compilation error by using the appropriate argument (`RAW_PARSE_DEFAULT`).
- yb_alter_table_rewrite.out:
    - error message for `ALTER TABLE test_drop_pk_column_part DROP COLUMN id; -- should fail`:
       - master commit 19ab966 adds this test.
       - change the error message as per PG commit a0555ddab9b672a04681ce0d9f6c94104c01b15f.
Jira: DB-11810

Test Plan: yb_feature_alter_table, yb_alter_table_rewrite

Reviewers: myang, jason, tfoucher

Reviewed By: tfoucher

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fizaaluthra fizaaluthra

Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@yugabyte-ci @fizaaluthra