Re: [BUGS] BUG #14866: The generated constraint in the typed tablecauses the server to crash - Mailing list pgsql-bugs

On 2017/11/10 11:33, Michael Paquier wrote:
> On Thu, Nov 9, 2017 at 2:39 PM, Amit Langote wrote:
>> 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.

*After* the patch 0002 for partition tables, lock on the parent table is
taken by parse_utilcmds.c, not tablecmds.c anymore, which is what I'm
assuming you said above.

> 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

I got similar feelings about that.  As you seem to say say, it would be
nice if generateSerialExtraStmts() didn't add the CreateSeqStmt and
AlterSeqStmt nodes directly to CreateStmtCxt.blist and
CreateStmtCxt.alist, respectively.

However, as you might know, any processing that needs a CreateStmtContext
and manipulates its blist and alist fields must occur within
parse_utilcmds.c.  If following that rule means we now have to take the
lock on the partition's parent table earlier than it used to, then I don't
see a problem with it, although I may be missing something.

In early versions of the partitioning patch, there used to be a
transformPartitionOf() which would do the same job as transformOfType(),
that is, make new ColumnDef's from the parent table's attributes.  But, I
removed it in favor of making MergeAttributes() create those ColumnDef's,
because the latter needed to lock and open the parent anyway.  However, to
handle identity columns, it seems we might need to go back to having a
transformPartitionOf() after all, but it's going to be quite some work.

> 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.

The duplication is unfortunate, but the code in tablecmds.c you seem to be
talking about (in MergeAttributes()) won't be able to satisfy the
requirement that the sequence for the identity column of a typed table be
created before the table itself is created.  By that point, we're already
in DefineRelation() creating the table.  In fact, I think to fix the
duplication, it might be better to work on removing the relevant code in
MergeAttributes().

Anyway, I agree with you that the code restructuring required might be
hard to commit as bug fix in the v10 branch.  So your downthread patch
that produces error upon specifying IDENTITY option for typed table or
partition table columns seems like a good compromise for now.

Thanks,
Amit



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