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

PG16 - Add citus_truncate_trigger for Citus foreign tables #7170

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

naisila
Copy link
Member

@naisila naisila commented Aug 31, 2023

Relevant PG commit:
postgres/postgres@3b00a94

#7138

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #7170 (449b00c) into main (205b159) will decrease coverage by 0.07%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #7170      +/-   ##
==========================================
- Coverage   93.21%   93.15%   -0.07%     
==========================================
  Files         274      274              
  Lines       59321    59322       +1     
==========================================
- Hits        55296    55260      -36     
- Misses       4025     4062      +37     

TG_ARGV[0], TG_OP, TG_WHEN, TG_LEVEL;
RETURN NULL;
END;$$;
CREATE TRIGGER trig_stmt_before BEFORE TRUNCATE ON foreign_table
Copy link
Member

Choose a reason for hiding this comment

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

I think this test ensures that we have the trigger on the shell table.

It is also possible to create triggers on the shards, see https://github.com/citusdata/citus/blob/main/src/test/regress/sql/distributed_triggers.sql

Do you think is it worth adding such test? I think it'd be at least useful to check it locally before merging.

ps: As far as I remember we do not allow "distributed foreign tables", but still we can probably have a similar test for citus managed table shards?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - I am actually not able to get successful truncate trigger on the shard. It's not about the trigger though - I believe we have something missing on truncating Citus foreign tables logic. Looking into it.

@naisila naisila force-pushed the naisila/pg16_foreign_truncate_trigger branch 2 times, most recently from cb85090 to 6a0bc86 Compare September 4, 2023 13:38
@naisila naisila changed the title PG16 - Add tests for Truncate trigger on Citus foreign table PG16 - Add citus_truncate_trigger for Citus foreign tables Sep 4, 2023
@naisila
Copy link
Member Author

naisila commented Sep 4, 2023

Normally, we support truncating Citus tables through the citus_truncate_trigger. However, this trigger hasn't been active so far for Citus foreign tables since foreign tables did not support TRUNCATE trigger (until now with PG16 Support TRUNCATE triggers on foreign tables. · postgres/postgres@3b00a94 (github.com))

citus/src/backend/distributed/commands/citus_add_local_table_to_metadata.c at main · citusdata/citus (github.com)

However, the Citus foreign table local shard was truncated somehow, even without the citus_truncate_trigger ... I mean truncating Citus foreign tables somehow worked so far, we even have this commit Support TRUNCATE for foreign tables · citusdata/citus@a1c3580 (github.com) and the test is passing.

I am confused as to how the truncate worked so far @onderkalaci

I changed this PR for now from a test PR to actually adding citus_truncate_trigger to Citus foreign tables.

@naisila naisila requested a review from onderkalaci September 4, 2023 13:41
@naisila
Copy link
Member Author

naisila commented Sep 5, 2023

Update:
The following is an explanation as to why the TRUNCATE command worked on Citus foreign tables so far, even though they didn't have the citus_truncate_trigger:
Consider the following scenario:

$ \d
                      List of relations
Schema |           Name            |     Type      |  Owner
--------+---------------------------+---------------+---------
public | foreign_table             | foreign table | naisila
public | foreign_table_102009      | foreign table | naisila
public | foreign_table_test        | table         | naisila
public | foreign_table_test_102008 | table         | naisila

$ select * from pg_dist_placement;
placementid | shardid | shardstate | shardlength | groupid
-------------+---------+------------+-------------+---------
           1 |  102008 |          1 |           0 |       0
           2 |  102009 |          1 |           0 |       0

-- remove foreign_table_test placement row
$ delete from pg_dist_placement where shardid = 102008;
DELETE 1

-- truncate doesn't complete
$ truncate foreign_table;
DEBUG:  starting remote transaction on connection 0x555d068f9790
-- STUCK HERE
^CCancel request sent
DEBUG:  closing remote transaction on connection 0x555d068f9790
ERROR:  canceling statement due to user request

-- reinsert foreign_table_test placement row
$ insert into pg_dist_placement values (1, 102008, 1, 0, 0);
INSERT 0 1

-- now remove foreign_table placement row
$ delete from pg_dist_placement where shardid = 102009;
DELETE 1

-- truncate completes - it doesn't need access to its shard at any point
$ truncate foreign_table;
DEBUG:  starting remote transaction on connection 0x555d068f9790
DEBUG:  closing remote transaction on connection 0x555d068f9790
TRUNCATE TABLE

TRUNCATE foreign_table starts a remote transaction for TRUNCATE foreign_table_test which also generates TRUNCATE foreign_table_test_102008 We can't see the logs of the remote transaction.
Now, TRUNCATE foreign_table_test_102008 should somehow be truncating foreign_table_102009 as well.
My understanding is that given that foreign table's shard is also a foreign table connected to local table's shard, when truncating foreign_table_test_102008, foreign_table_102009 is also truncated by postgres_fdw logic.

$ INSERT INTO foreign_table_test VALUES (1, 'text_test');

$ truncate foreign_table_test_102008;
DEBUG:  building index "pg_toast_17231_index" on table "pg_toast_17231" serially
DEBUG:  index "pg_toast_17231_index" can safely use deduplication
TRUNCATE TABLE

$ select * from foreign_table_test_102008;
DEBUG:  FromList item is not empty
id | data | a
----+------+---
(0 rows)

$ select * from foreign_table_102009;
DEBUG:  FromList item is not empty
DEBUG:  starting remote transaction on connection 0x5652cbe9a4e0
DEBUG:  closing remote transaction on connection 0x5652cbe9a4e0
id | data | a
----+------+---
(0 rows)

So, truncate works without citus_truncate_trigger, but we want to add it here in this PR for consistency.

Note: My guess is that probably other commands on foreign tables work without the need of propagating the command to the foreign table's shard.

@naisila naisila force-pushed the naisila/pg16_foreign_truncate_trigger branch from 6a0bc86 to bef11d2 Compare September 5, 2023 09:14
@@ -1478,11 +1478,16 @@ InsertMetadataForCitusLocalTable(Oid citusLocalTableId, uint64 shardId,
static void
FinalizeCitusLocalTableCreation(Oid relationId)
{
#if PG_VERSION_NUM >= PG_VERSION_16
Copy link
Member

Choose a reason for hiding this comment

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

1 line comment would help, saying that PG 16+ supports truncate triggers on fdw

@naisila naisila force-pushed the naisila/pg16_foreign_truncate_trigger branch from bef11d2 to 9f24ed7 Compare September 5, 2023 16:15
@naisila naisila force-pushed the naisila/pg16_foreign_truncate_trigger branch from 9f24ed7 to 449b00c Compare September 5, 2023 16:20
@naisila naisila merged commit 5c658b4 into main Sep 5, 2023
@naisila naisila deleted the naisila/pg16_foreign_truncate_trigger branch September 5, 2023 16:42
francisjodi pushed a commit that referenced this pull request Nov 13, 2023
Since in PG16, truncate triggers are supported on foreign tables, we add
the citus_truncate_trigger to Citus foreign tables as well, such that the TRUNCATE
command is propagated to the table's single local shard as well.
Note that TRUNCATE command was working for foreign tables even before this
commit: see #7170 (comment) for details

This commit also adds tests with user-enabled truncate triggers on Citus foreign tables:
both trigger on the shell table and on its single foreign local shard.

Relevant PG commit:
postgres/postgres@3b00a94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants