gpdb: Wrong lock mode used for UPDATE / DELETE of the AO/AOCS partitioned table

Bug Report

With GDD enabled, when update / delete on AO/AOCS partitioned table (without unique index), i found that RowExclusiveLock was used instead of ExclusiveLock.

After #13897 , RelationIsAppendOptimized is changed to false when relation is a partitioned table, which changed lock level to RowExclusiveLock when update / delete AO/AOCS table. Is this expected behavior or a bug?

Greenplum version or build

master branch

Step to reproduce the behavior

create table partlockt (i int, t text) using ao_row
partition by range(i)
(start(1) end(10) every(1));

begin;
delete from partlockt where i = 4;
select * from pg_locks where relation = 'partlockt_1_prt_4'::regclass;
select * from pg_locks where relation = 'partlockt'::regclass;

About this issue

Original URL
State: closed
Created a year ago
Comments: 16 (16 by maintainers)

Most upvoted comments

Hi @linxuh and @charliettxx , thanks for reporting the issue and proposing the fix.

Firstly, the reason why RelationIsAppendOptimized was changed to exclude partition table in https://github.com/greenplum-db/gpdb/pull/13897 is to keep existing behavior unchanged:

Before that PR, partition tables do not have a valid access method (pg_class.relam is always 0). So RelationIsAppendOptimized would always return false for partitioned tables.
After that PR, partition tables carries an access method. So we had to exclude partitioned tables so that RelationIsAppendOptimized still always returns false for partitioned tables.

However, what I didn’t realize at that time is that the “existing behavior” was not the right behavior. In GP6, partitioned tables could return true for RelationIsAppendOptimized because GP6 uses a GP-specific data field pg_class.relstorage to indicate the table storage format, and partitioned tables could have valid pg_class.relstorage. I guess during the PG12 merge when we replaced relstorage with relam, we changed the behavior inadvertently.

That’s why in GP6, exclusive lock would be acquired in the concerning scenario (due to the reason @kainwen mentioned), but not in GP7. So yes, we should fix this issue.

Now about the fix: I agree we should remove the relkind check in RelationIsAppendOptimized. The macro should be as generic as just checking the relstorage/relam, same as RelationIsAoRows and RelationIsAoCols. However, there could be some other issues after that. For example, in RelationBuildDesc we build the pg_appendonly information for a non-partitioned AO table (partitioned table doesn’t have pg_appendonly entry in GP7): https://github.com/greenplum-db/gpdb/blob/main/src/backend/utils/cache/relcache.c#L1297-L1298. So after the change of RelationIsAppendOptimized, we need to change the condition in RelationBuildDesc to exclude partitioned table. Basically, we need to move the relkind != RELKIND_PARTITIONED_TABLE check from RelationIsAppendOptimized to where it is needed. @charliettxx we can discuss more when you start working on the fix.

huansong on May 10, 2023

Greenplum is MPP database, holding RowExclusive Lock for Update|Delete statement will lead concurrently update|delete happen on segment which means it is possible for transactions wait for others on segments. This can lead to global deadlock.

Since Greenplum 6, we introduce global deadlock detector, when it is turned on, for heap table’s update | delete, we can hold the same lock level as Postgres.

However, even with global deadlock detector enabled, for AO|AOCS table, due to its implementation of visibility map design, it does not support concurrenly update|delete on segments. That is why Greenplum always hold Exclusive Lock for AO|AOCS tables. This is documented behavior: https://docs.vmware.com/en/VMware-Greenplum/6/greenplum-database/admin_guide-dml.html .

I close this issue, feel free to reopen if needed.

kainwen on May 5, 2023