Re: [HACKERS] Event triggers + table partitioning cause server crashin current master

On 2017/05/16 1:18, Mark Dilger wrote:
> 
>> On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnorter@gmail.com> wrote:
>>
>> I can confirm that this fixes the crash that I was seeing.  I have read
>> through the patch briefly, but will give it a more thorough review in the
>> next few hours.

Thanks a lot for the review.

> My only negative comment is that your patch follows a precedent of using
> event trigger commands named for AlterTable for operations other than
> an ALTER TABLE command.  You clearly are not starting the precedent
> here, as they are already used for IndexStmt and ViewStmt processing, but
> expanding the precedent by using them in DefineRelation, where they were
> not used before, seems to move in the wrong direction.  Either the functions
> should be renamed to something more general to avoid implying that the
> function is ALTER TABLE specific, or different functions should be defined
> and used, or ...?  I am uncertain what the right solution would be.

Hmm.  I think an alternative to doing what I did in my patch is to get rid
of calling AlterTableInternal() from DefineRelation() altogether.
Instead, the required ALTER TABLE commands can be added during the
transform phase, which will create a new AlterTableStmt and add it to the
list of things to be done after the relation is defined.  That way,
ProcessUtilitySlow() takes care of doing the event trigger stuff instead
of having to worry about it ourselves.  Attached updated patch does that.

PS: actually, we're talking elsewhere [1] of getting rid of adding
explicit NOT NULL constraints on range partition keys altogether, so even
if we apply this patch to fix the bug, there is a good chance that the
code will go away (of course, the bug won't exist too)

I divided the patch into 2 parts.

0001 fixes the bug you reported (not so directly though as my previous
     patch did)

0002 fixes the bug I found while working on this, whereby the content of
     CreateStmt will become invalid when creating partitions which could
     cause crash in certain case

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d%40lab.ntt.co.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers