Re: BUG #14866: The generated constraint in the typed table causes the server to crash

On Thu, Nov 9, 2017 at 2:39 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Sorry Michael. I didn't notice this in my gmail inbox until today.

No problem, happy to see your input on the matter. I took me a bit of
time to digest matters discussed on this thread a bit more.

> On 2017/11/01 23:05, Michael Paquier wrote:
>> On Wed, Nov 1, 2017 at 1:36 PM, Peter Eisentraut
>> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>> I wonder whether we should even allow this. The SQL standard does not
>>> allow identity columns in typed tables, so there is support for that.
>>
>> OK. At the same time being able to support that is not complicated
>> either, even if the patch I sent earlier is a bit grotty in the way it
>> does handle it. I can see arguments in favor of either solution.
>
> Agree that it's not that hard to support IDENTITY on typed table columns.

Not that hard, still... See my arguments downthread.

> I rewrote your patch a bit so that it now makes transformColumnDefinition
> use the ColumnDef initialized by transformOfType which contains enough
> information to get the needed type OID. I think you might find it a bit
> simpler. :)
>
> Also, I carved it out into the attached patch 0001.

Yeah. I can see the difference.

>>> I'm not sure whether it makes sense in partitions. You are supposed to
>>> insert through the partition root, so making identity columns in
>>> partitions would just be confusing.
>>
>> Robert, Amit, do you have opinions on the matter? It is possible to
>> have multiple level of partitions as well.
>
> IMHO, we should support IDENTITY on partitions, just like we support
> specifying DEFAULT on partitions. We support issuing direct INSERT on
> partitions no matter where in the hierarchy it is, so having these
> features work normally makes sense to me.

Okay, yes there is room for support of such a feature.

> I looked at your patch and saw a minor problem with it. With your patch,
> there exists a lock-strength-upgrade deadlock hazard. It is making an
> earlier phase of partition creation (transformCreateStmt) take an
> AccessShareLock on the parent table, whereas a later phase
> (DefineRelation) will take an AccessExclusiveLock. I modified your patch
> to take the AccessExclusiveLock to begin with and added a comment nearby
> clarifying the same.

Yeah, I have been wondering about similar matters. Good thing that you
took the time to do a lookup. The proposed patches are really giving
me a bad feeling about all this as I look more at the code. Any lock
resolution done now happens within tablecmds.c, and not when doing the
parsing so I think that this patch needs actually far more work that
what's proposed. For one, it seems to me that CONST_IDENTITY handling
should be refactored so as they get a treatment similar to
CONSTR_CHECK and CONSTR_FOREIGN, which appends those clauses to
dedicated lists, which then creates sub-nodes in the ALTER TABLE code
paths. There is already a node for AddIdentity so it would be way
better to rely on that and make all lock resolutions remain there. And
for two, for typed tables, this patch adds another code path to
complain about ERRCODE_UNDEFINED_COLUMN, which is also a duplicate of
something that should fail in tablecmds.c in my opinion.

All in all, I would be more on board with just issuing errors when
trying to attempt typed table creation with identity columns and
partitions for now, by complaining with a
ERRCODE_FEATURE_NOT_SUPPORTED. Any person willing to add this support
later on could always provide a patch for it, I am now rather
convinced that what I proposed upthread is not the correct way to do
so, and there is no rush in implementing it correctly for v11 or even
later.

Peter, Robert, Amit, thoughts?
--
Michael