Fix segfault when setting chunk replica identity

[fabriziomello]

The segfault happened executing AlterTableInternal for setting the REPLICA IDENTITY during chunk creation (introduced by #5512) that is dispatched by a trigger invoked by a DDL statement.

Fixed it by using our internal ts_alter_table_with_event_trigger handler function to setup the event trigger context.

Fixes #7406

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.04%. Comparing base (59f50f2) to head (2ba3df8).
Report is 588 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7434      +/-   ##
==========================================
+ Coverage   80.06%   81.04%   +0.97%     
==========================================
  Files         190      229      +39     
  Lines       37181    42514    +5333     
  Branches     9450    10625    +1175     
==========================================
+ Hits        29770    34455    +4685     
- Misses       2997     3749     +752     
+ Partials     4414     4310     -104     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mkindahl]

The commit comment is very focused on the code, so you might want to re-write it to explain the context so your future self can understand what the problem was.

This problem can occur when you insert into a hypertable from inside a trigger, so starting there might be a good idea, that is:

Insert into hypertable calls AlterTableInternal [why?]... if called from inside a trigger, this will not have currentCommand set up [why and why does it work if not in a trigger?]... we need to wrap the call in ...Start and ...End.

A few suggestions for adding comments and one more call to AlterTableInternal that you might want to deal with (but fine to submit as separate PR if you prefer that).

Approving since the code looks fine and I see no issues with the fix.

[mkindahl]

This can be called in a nested fashion since the start and end commands remember the previous value of currentCommand. You might want to add a comment explaining why the start and end are necessary though, for your future self.

[fabriziomello]

Actually we have a function ts_alter_table_with_event_trigger to wrap the event trigger context and execute the AlterTableInternal that have the enough comments about why it is necessary.

[mkindahl]

The first argument to AlterTableInternal should be an OID and it indeed looks like table_id is taken from the ObjectAddress for the chunk. You might want to double-check, but it seems to make no difference for this particular case.

[fabriziomello]

Well the chunk->table_id is the OID of the chunk relation returned by the ts_chunk_create_table that internally returns the ObjectAddress->objectId returned by DefineRelation so we're fine here.

[fabriziomello]

The commit comment is very focused on the code, so you might want to re-write it to explain the context so your future self can understand what the problem was.

Ok.

This problem can occur when you insert into a hypertable from inside a trigger, so starting there might be a good idea, that is:

Will recheck but it was not so simple like this to reproduce the case.

Insert into hypertable calls AlterTableInternal [why?]...

Because of this PR #5515. But looking now at the implementation looks like we didn't did a good job because we always call the AlterTableInternal, so I think we should improve it a bit:

➜ git diff                                                         
diff --git a/src/chunk.c b/src/chunk.c
index c2535d368..89105ba4a 100644
--- a/src/chunk.c
+++ b/src/chunk.c
@@ -875,6 +875,14 @@ static void
 chunk_set_replica_identity(const Chunk *chunk)
 {
    Relation ht_rel = relation_open(chunk->hypertable_relid, AccessShareLock);
+
+   /* Do nothing if hypertable is using the default replica identity */
+   if (ht_rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT)
+   {
+       table_close(ht_rel, NoLock);
+       return;
+   }
+
    ReplicaIdentityStmt stmt = {
        .type = T_ReplicaIdentityStmt,
        .identity_type = ht_rel->rd_rel->relreplident,

if called from inside a trigger, this will not have currentCommand set up [why and why does it work if not in a trigger?]... we need to wrap the call in ...Start and ...End.

Will recheck!

A few suggestions for adding comments and one more call to AlterTableInternal that you might want to deal with (but fine to submit as separate PR if you prefer that).

The other one need to investigate if it is possible to raise the same problem.