> On May 15, 2017, at 9:37 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> 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 for your continued efforts. It is passed midnight here, so I think I will
save reviewing your new patches until sometime tomorrow morning.
Since we are passed feature freeze, one option is to keep the patch simple
and not do more than you did in your first patch, though until I review the new
patch, I don't know for sure.
Mark Dilger
