Thread: [BUGS] BUG #14866: The generated constraint in the typed table causes theserver to crash

The following bug has been logged on the website:

Bug reference:      14866
Logged by:          Мансур Галиев
Email address:      gomer94@yandex.ru
PostgreSQL version: 10.0
Operating system:   Ubuntu 16.04.3 LTS x64
Description:

This sql code crashes the server:

CREATE TYPE comp AS (f1 integer, f2 text, f3 bigint);

CREATE TABLE typed_table OF comp (  f1 WITH OPTIONS GENERATED ALWAYS AS IDENTITY);



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

On Mon, Oct 23, 2017 at 4:44 PM,  <gomer94@yandex.ru> wrote:
> This sql code crashes the server:
>
> CREATE TYPE comp AS (f1 integer, f2 text, f3 bigint);
>
> CREATE TABLE typed_table OF comp (
>    f1 WITH OPTIONS GENERATED ALWAYS AS IDENTITY);

Thanks for the report. I can reproduce the crasher here. The problem
happens when doing a type name lookup when transforming the CREATE
clause in the parser. Looking into it now.
-- 
Michael


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

On Mon, Oct 23, 2017 at 11:09 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Oct 23, 2017 at 4:44 PM,  <gomer94@yandex.ru> wrote:
>> This sql code crashes the server:
>>
>> CREATE TYPE comp AS (f1 integer, f2 text, f3 bigint);
>>
>> CREATE TABLE typed_table OF comp (
>>    f1 WITH OPTIONS GENERATED ALWAYS AS IDENTITY);
>
> Thanks for the report. I can reproduce the crasher here. The problem
> happens when doing a type name lookup when transforming the CREATE
> clause in the parser. Looking into it now.

Looking at the grammar of CREATE TABLE, child partitions crash as well
for the same reasons:
CREATE TABLE itest_parent (f1 date NOT NULL, f2 int) PARTITION BY RANGE (f1);
CREATE TABLE itest_child PARTITION OF itest_parent (
   f2 WITH OPTIONS GENERATED ALWAYS AS IDENTITY
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');

Attached is a patch to address the problem. There are a couple of
things to consider:
- transformColumnDefinition is missing the fact that a type may not be
set for a column defined, and as far as I can see the type name is
needed beforehand to allow the generation of all the serial commands.
This can happen when using CREATE TABLE OF, as you reported, for which
the data type can be found in the type defined. But this can happen as
well when declaring a child partition.
- MergeAttributes() needs to copy the identity field from the origin
to the target ColumnDef for both things.
As both MergeAttributes() and transformColumnDefinition() are linked
with each other, I went to the most straight-forward, less ugly
solution as child should not inherit identity constraints by default
from the parent. Comments are welcome.

I am adding Peter E in CC, who is the author and committer of the
feature. I am adding an entry in the CF as well.
-- 
Michael

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

On 10/23/17 18:21, Michael Paquier wrote:
> Attached is a patch to address the problem. There are a couple of
> things to consider:
> - transformColumnDefinition is missing the fact that a type may not be
> set for a column defined, and as far as I can see the type name is
> needed beforehand to allow the generation of all the serial commands.
> This can happen when using CREATE TABLE OF, as you reported, for which
> the data type can be found in the type defined. But this can happen as
> well when declaring a child partition.

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

> I am adding Peter E in CC, who is the author and committer of the
> feature. I am adding an entry in the CF as well.

There are two entries for this now.  Maybe remove one?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

On Wed, Nov 1, 2017 at 1:36 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 10/23/17 18:21, Michael Paquier wrote:
>> Attached is a patch to address the problem. There are a couple of
>> things to consider:
>> - transformColumnDefinition is missing the fact that a type may not be
>> set for a column defined, and as far as I can see the type name is
>> needed beforehand to allow the generation of all the serial commands.
>> This can happen when using CREATE TABLE OF, as you reported, for which
>> the data type can be found in the type defined. But this can happen as
>> well when declaring a child partition.
>
> 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.

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

>> I am adding Peter E in CC, who is the author and committer of the
>> feature. I am adding an entry in the CF as well.
>
> There are two entries for this now.  Maybe remove one?

Fixed.
-- 
Michael


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

Sorry Michael.  I didn't notice this in my gmail inbox until today.

On 2017/11/01 23:05, Michael Paquier wrote:
> On Wed, Nov 1, 2017 at 1:36 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 10/23/17 18:21, Michael Paquier wrote:
>>> Attached is a patch to address the problem. There are a couple of
>>> things to consider:
>>> - transformColumnDefinition is missing the fact that a type may not be
>>> set for a column defined, and as far as I can see the type name is
>>> needed beforehand to allow the generation of all the serial commands.
>>> This can happen when using CREATE TABLE OF, as you reported, for which
>>> the data type can be found in the type defined. But this can happen as
>>> well when declaring a child partition.
>>
>> 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.

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.

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

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.

You'll find the updated version in attached 0002.

Sorry again for the delay in replying.

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

On Thu, Nov 9, 2017 at 2:39 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.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.eisentraut@2ndquadrant.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


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

On Fri, Nov 10, 2017 at 11:33 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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?

And this gives the attached. That still looks like the safest thing to
do for now on v10 and HEAD after more thoughts.
-- 
Michael

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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

On 2017/11/11 18:11, Michael Paquier wrote:
> On Fri, Nov 10, 2017 at 11:33 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> 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?
> 
> And this gives the attached. That still looks like the safest thing to
> do for now on v10 and HEAD after more thoughts.

That looks good to me.  That is, if the consensus turns out to be that we
issue error in those cases.

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

On Mon, Nov 13, 2017 at 4:32 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 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.

Another idea that could be done here is to rework as well
transformOfType() so as its column definition logic is pulled down to
MergeAttributes(). There is some logic telling that the typed columns
are first in the list. That's not lovely IMO.

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

Yeah, possibly. This really requires more thinking as we likely don't
want to make the code more a mess than it is now. There is no urgency
in getting things refactored now anyway. And perhaps there will be no
need to do so.
-- 
Michael

On Wed, Nov 15, 2017 at 10:23 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Yeah, possibly. This really requires more thinking as we likely don't
> want to make the code more a mess than it is now. There is no urgency
> in getting things refactored now anyway. And perhaps there will be no
> need to do so.

Peter, you are marked as a reviewer of this patch which is a bug fix.
Are you planning to look at it? For now I am bumping it to the next
CF.
-- 
Michael

On 11/27/17 20:39, Michael Paquier wrote:
> On Wed, Nov 15, 2017 at 10:23 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Yeah, possibly. This really requires more thinking as we likely don't
>> want to make the code more a mess than it is now. There is no urgency
>> in getting things refactored now anyway. And perhaps there will be no
>> need to do so.
> 
> Peter, you are marked as a reviewer of this patch which is a bug fix.
> Are you planning to look at it? For now I am bumping it to the next
> CF.

I'll look at it at some point.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/13/17 02:33, Amit Langote wrote:
> On 2017/11/11 18:11, Michael Paquier wrote:
>> On Fri, Nov 10, 2017 at 11:33 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> 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?
>>
>> And this gives the attached. That still looks like the safest thing to
>> do for now on v10 and HEAD after more thoughts.
> 
> That looks good to me.  That is, if the consensus turns out to be that we
> issue error in those cases.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Dec 9, 2017 at 2:27 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> committed

Thanks, Peter and Amit.
-- 
Michael

On 2017/12/09 13:00, Michael Paquier wrote:
> On Sat, Dec 9, 2017 at 2:27 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> committed
> 
> Thanks, Peter and Amit.

Thank you too.

Regards,
Amit