Re: Segfault when creating partition with a primary key and sql_drop trigger exists

> On Wed, 3 Oct 2018 at 09:53, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Sep 28, 2018 at 12:17:00PM +0900, Michael Paquier wrote:
> > I think that Alvaro should definitely look at this patch to be sure, or
> > I could do it, but I would need to spend way more time on this and check
> > event trigger interactions.
> >
> > Anyway, I was struggling a bit regarding the location where adding a
> > regression test. event_trigger.sql makes the most sense but in tests
> > for drops the objects are created before the event trigger is defined,
> > so that would need to move around so as the original problem is
> > reproducible. Perhaps you have an idea for that?
>
> Okay. I have spent more time on this issue, and I have been able to
> integrate a test in the existing event_trigger.sql which is able to
> reproduce the reported failure. Attached is what I am finishing with.
>
> I still want to do more testing on it, and the day is ending here.
>
> Thoughts?

Sorry, couldn't answer your previous message, since was away. So far I don't
see any problems with your proposed patch.

> On Thu, 4 Oct 2018 at 17:22, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> I admit I'm surprised that your patch fixes the bug. sql_drop was added
> before the command-stashing was added for pg_event_trigger_ddl_commands
> was added, and sql_drop only processes objects from the list passed to
> performMultipleDeletions, so adding the EventTriggerAlterTableStart() /
> End() calls should not affect it ... evidently I must be missing
> something here.
>
> Still looking.

I also find strange another part of this problem, namely why we ended up doing
AlterTableInternal at all, since I assumed that after MergeAttributes all
attnotnull should be merged with OR. But for example these would work:

CREATE TABLE collections_list_1
PARTITION OF collections_list
FOR VALUES IN (1);

CREATE TABLE collections_list_1
PARTITION OF collections_list (key, ts, collection_id not null, value)
FOR VALUES IN (1);

Looks like in MergeAttributes at the branch:

if (is_partition && list_length(saved_schema) > 0)

we override not null property of ColumnDef:

if (coldef->is_from_parent)
{
coldef->is_not_null = restdef->is_not_null;

It looks a bit confusing, so I wonder if it's how it should be?