From fdc5abc0cafd3608c54b5a687b89e9647b437349 Mon Sep 17 00:00:00 2001 From: Zhang Mingli Date: Sat, 12 Oct 2024 13:43:07 +0800 Subject: [PATCH] Fix reference leak on AO/AOCS partition tables with unique index. (#649) * Fix reference leak on AO/AOCS partition tables with unique index. Fix https://github.com/cloudberrydb/cloudberrydb/issues/557. This is introduced by GPDB commit ed364586b99. For AO/AOCS partition tables, it's possible to update across partitions. And it does have deleteDesc and uniqueCheckDesc if there were. The DeleteDesc and InsertDesc are not enough to decide if the partition has a update operation or not. Some partitions have visimap but some not, clean them if possible. Authored-by: Zhang Mingli avamingli@gmail.com --- src/backend/access/aocs/aocsam_handler.c | 14 ++ .../access/appendonly/appendonlyam_handler.c | 14 ++ src/test/regress/expected/.gitignore | 1 + src/test/regress/greenplum_schedule | 2 + .../input/ao_unique_index_partition.source | 164 ++++++++++++++++++ .../output/ao_unique_index_partition.source | 150 ++++++++++++++++ src/test/regress/sql/.gitignore | 1 + 7 files changed, 346 insertions(+) create mode 100644 src/test/regress/input/ao_unique_index_partition.source create mode 100644 src/test/regress/output/ao_unique_index_partition.source diff --git a/src/backend/access/aocs/aocsam_handler.c b/src/backend/access/aocs/aocsam_handler.c index f7bc82c18a7..b17931eed14 100644 --- a/src/backend/access/aocs/aocsam_handler.c +++ b/src/backend/access/aocs/aocsam_handler.c @@ -338,6 +338,20 @@ aoco_dml_finish(Relation relation, CmdType operation) AppendOnlyVisimap_Finish_forUniquenessChecks(state->uniqueCheckDesc->visimap); pfree(state->uniqueCheckDesc->visimap); } + else + { + /* + * Github issue: https://github.com/cloudberrydb/cloudberrydb/issues/557 + * + * For partition tables, it's possible to update across partitions. + * And it does have deleteDesc and uniqueCheckDesc if there were. + * Some partitions have visimap but some not, clean them if possible. + */ + if (state->uniqueCheckDesc->visimap) + { + AppendOnlyVisimapStore_Finish(&state->uniqueCheckDesc->visimap->visimapStore, AccessShareLock); + } + } state->uniqueCheckDesc->visimap = NULL; state->uniqueCheckDesc->visiMapDelete = NULL; diff --git a/src/backend/access/appendonly/appendonlyam_handler.c b/src/backend/access/appendonly/appendonlyam_handler.c index 2318c313692..3aae9b21eed 100644 --- a/src/backend/access/appendonly/appendonlyam_handler.c +++ b/src/backend/access/appendonly/appendonlyam_handler.c @@ -308,6 +308,20 @@ appendonly_dml_finish(Relation relation, CmdType operation) AppendOnlyVisimap_Finish_forUniquenessChecks(state->uniqueCheckDesc->visimap); pfree(state->uniqueCheckDesc->visimap); } + else + { + /* + * Github issue: https://github.com/cloudberrydb/cloudberrydb/issues/557 + * + * For partition tables, it's possible to update across partitions. + * And it does have deleteDesc and uniqueCheckDesc if there were. + * Some partitions have visimap but some not, clean them if possible. + */ + if (state->uniqueCheckDesc->visimap) + { + AppendOnlyVisimapStore_Finish(&state->uniqueCheckDesc->visimap->visimapStore, AccessShareLock); + } + } state->uniqueCheckDesc->visimap = NULL; state->uniqueCheckDesc->visiMapDelete = NULL; diff --git a/src/test/regress/expected/.gitignore b/src/test/regress/expected/.gitignore index cfd35715eb4..a621cd3224a 100644 --- a/src/test/regress/expected/.gitignore +++ b/src/test/regress/expected/.gitignore @@ -63,3 +63,4 @@ /directory_table.out /directory_table_optimizer.out /tag.out +/ao_unique_index_partition.out diff --git a/src/test/regress/greenplum_schedule b/src/test/regress/greenplum_schedule index a76140c2214..ae3426ae813 100755 --- a/src/test/regress/greenplum_schedule +++ b/src/test/regress/greenplum_schedule @@ -234,6 +234,8 @@ test: uao_ddl/alter_drop_allcol_row uao_ddl/alter_drop_allcol_column uao_ddl/alt test: uao_dml/uao_dml_cursor_row uao_dml/uao_dml_select_row uao_dml/uao_dml_cursor_column uao_dml/uao_dml_select_column uao_dml/uao_dml_unique_index_delete_row uao_dml/uao_dml_unique_index_delete_column uao_dml/uao_dml_unique_index_update_row uao_dml/uao_dml_unique_index_update_column +# test ao partition unique index update +test: ao_unique_index_partition # disable autovacuum for the test test: disable_autovacuum diff --git a/src/test/regress/input/ao_unique_index_partition.source b/src/test/regress/input/ao_unique_index_partition.source new file mode 100644 index 00000000000..f8c45fdf6b9 --- /dev/null +++ b/src/test/regress/input/ao_unique_index_partition.source @@ -0,0 +1,164 @@ +-- +-- Github issue: https://github.com/cloudberrydb/cloudberrydb/issues/557 +-- Test update on ao partition tables with unique index. +-- +create schema t_issue_557; +set search_path to t_issue_557; +CREATE TABLE IF NOT EXISTS t_issue_557_ao +( +product_id INT, +is_audited BOOLEAN DEFAULT FALSE, +quantity SMALLINT, +total_sales BIGINT, +unit_price REAL, +discount DOUBLE PRECISION, +description TEXT, +sale_date TIMESTAMP, +order_date DATE, +status CHAR(10), +customer_name VARCHAR(20), +price DECIMAL(20, 10) +) +DISTRIBUTED BY (product_id) +PARTITION BY HASH(description); + +CREATE TABLE t_issue_557_ao_part1 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 0) +WITH (appendonly=true); + +CREATE TABLE t_issue_557_ao_part2 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 1) +WITH (appendonly=true); + +CREATE TABLE t_issue_557_ao_part3 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 2) +WITH (appendonly=true); +-- Create Indexes + +-- Unique +CREATE UNIQUE INDEX on t_issue_557_ao(product_id,description); + +INSERT INTO t_issue_557_ao ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; + +UPDATE t_issue_557_ao +SET status = 'Closed', +description = description || ' Audited'; + +DELETE FROM t_issue_557_ao; + +INSERT INTO t_issue_557_ao ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; + +UPDATE t_issue_557_ao +SET status = 'Closed', +description = description || ' Audited'; + +-- AOCO +CREATE TABLE IF NOT EXISTS t_issue_557_aocs +( +product_id INT, +is_audited BOOLEAN DEFAULT FALSE, +quantity SMALLINT, +total_sales BIGINT, +unit_price REAL, +discount DOUBLE PRECISION, +description TEXT, +sale_date TIMESTAMP, +order_date DATE, +status CHAR(10), +customer_name VARCHAR(20), +price DECIMAL(20, 10) +) +DISTRIBUTED BY (product_id) +PARTITION BY HASH(description); + +CREATE TABLE t_issue_557_aocs_part1 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 0) +WITH (appendonly=true, orientation=column); + +CREATE TABLE t_issue_557_aocs_part2 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 1) +WITH (appendonly=true, orientation=column); + +CREATE TABLE t_issue_557_aocs_part3 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 2) +WITH (appendonly=true, orientation=column); +-- Create Indexes + +-- Unique +CREATE UNIQUE INDEX on t_issue_557_aocs(product_id,description); + +INSERT INTO t_issue_557_aocs ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; + +UPDATE t_issue_557_aocs +SET status = 'Closed', +description = description || ' Audited'; + +DELETE FROM t_issue_557_aocs; + +INSERT INTO t_issue_557_aocs ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; + +UPDATE t_issue_557_aocs +SET status = 'Closed', +description = description || ' Audited'; + +-- start_ignore +drop schema t_issue_557 cascade; +-- end_ignore diff --git a/src/test/regress/output/ao_unique_index_partition.source b/src/test/regress/output/ao_unique_index_partition.source new file mode 100644 index 00000000000..9072846b536 --- /dev/null +++ b/src/test/regress/output/ao_unique_index_partition.source @@ -0,0 +1,150 @@ +-- +-- Github issue: https://github.com/cloudberrydb/cloudberrydb/issues/557 +-- Test update on ao partition tables with unique index. +-- +create schema t_issue_557; +set search_path to t_issue_557; +CREATE TABLE IF NOT EXISTS t_issue_557_ao +( +product_id INT, +is_audited BOOLEAN DEFAULT FALSE, +quantity SMALLINT, +total_sales BIGINT, +unit_price REAL, +discount DOUBLE PRECISION, +description TEXT, +sale_date TIMESTAMP, +order_date DATE, +status CHAR(10), +customer_name VARCHAR(20), +price DECIMAL(20, 10) +) +DISTRIBUTED BY (product_id) +PARTITION BY HASH(description); +CREATE TABLE t_issue_557_ao_part1 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 0) +WITH (appendonly=true); +NOTICE: table has parent, setting distribution columns to match parent table +CREATE TABLE t_issue_557_ao_part2 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 1) +WITH (appendonly=true); +NOTICE: table has parent, setting distribution columns to match parent table +CREATE TABLE t_issue_557_ao_part3 +PARTITION OF t_issue_557_ao +FOR VALUES WITH (MODULUS 3, REMAINDER 2) +WITH (appendonly=true); +NOTICE: table has parent, setting distribution columns to match parent table +-- Create Indexes +-- Unique +CREATE UNIQUE INDEX on t_issue_557_ao(product_id,description); +INSERT INTO t_issue_557_ao ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; +UPDATE t_issue_557_ao +SET status = 'Closed', +description = description || ' Audited'; +DELETE FROM t_issue_557_ao; +INSERT INTO t_issue_557_ao ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; +UPDATE t_issue_557_ao +SET status = 'Closed', +description = description || ' Audited'; +-- AOCO +CREATE TABLE IF NOT EXISTS t_issue_557_aocs +( +product_id INT, +is_audited BOOLEAN DEFAULT FALSE, +quantity SMALLINT, +total_sales BIGINT, +unit_price REAL, +discount DOUBLE PRECISION, +description TEXT, +sale_date TIMESTAMP, +order_date DATE, +status CHAR(10), +customer_name VARCHAR(20), +price DECIMAL(20, 10) +) +DISTRIBUTED BY (product_id) +PARTITION BY HASH(description); +CREATE TABLE t_issue_557_aocs_part1 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 0) +WITH (appendonly=true, orientation=column); +NOTICE: table has parent, setting distribution columns to match parent table +CREATE TABLE t_issue_557_aocs_part2 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 1) +WITH (appendonly=true, orientation=column); +NOTICE: table has parent, setting distribution columns to match parent table +CREATE TABLE t_issue_557_aocs_part3 +PARTITION OF t_issue_557_aocs +FOR VALUES WITH (MODULUS 3, REMAINDER 2) +WITH (appendonly=true, orientation=column); +NOTICE: table has parent, setting distribution columns to match parent table +-- Create Indexes +-- Unique +CREATE UNIQUE INDEX on t_issue_557_aocs(product_id,description); +INSERT INTO t_issue_557_aocs ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; +UPDATE t_issue_557_aocs +SET status = 'Closed', +description = description || ' Audited'; +DELETE FROM t_issue_557_aocs; +INSERT INTO t_issue_557_aocs ( +product_id, +is_audited, +description, +status +) +SELECT +x.id, -- product_id +FALSE, +'Product description ' || x.id, -- description +'Closed' +FROM ( +SELECT * FROM generate_series(1, 20) AS id +) AS x; +UPDATE t_issue_557_aocs +SET status = 'Closed', +description = description || ' Audited'; +-- start_ignore +drop schema t_issue_557 cascade; +-- end_ignore diff --git a/src/test/regress/sql/.gitignore b/src/test/regress/sql/.gitignore index 007848e363e..c4b4bcf5ade 100644 --- a/src/test/regress/sql/.gitignore +++ b/src/test/regress/sql/.gitignore @@ -60,3 +60,4 @@ /external_table_union_all.sql /directory_table.sql /tag.sql +/ao_unique_index_partition.sql