Content-Type: multipart/alternative;
 boundary="------------Us8UmO9FwdNYFANceilHWzt7"
Message-ID: <65c49195-7117-67c4-5bfe-45ca8832dfe3@gmail.com>
Date: Tue, 28 Mar 2023 19:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17872: Dropping an attribute of a composite type breaks
 indexes over the type silently
Content-Language: en-US
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
References: <17872-d0fbb799dc3fd85d@postgresql.org>
 <3169550.1679928579@sss.pgh.pa.us> <3182314.1679935605@sss.pgh.pa.us>
 <1aa2eec0-2017-3870-6d88-228d08c587d3@gmail.com>
 <3267914.1679941213@sss.pgh.pa.us>
 <67cb509e-93c7-1c7b-2388-d7e00277b344@gmail.com>
 <3332642.1679943981@sss.pgh.pa.us>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: <3332642.1679943981@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/65c49195-7117-67c4-5bfe-45ca8832dfe3%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------Us8UmO9FwdNYFANceilHWzt7
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

27.03.2023 22:06, Tom Lane wrote:
> Alexander Lakhin<exclusion@gmail.com>  writes:
>> 27.03.2023 21:20, Tom Lane wrote:
>>> Hmm ... really?  I'd just concluded that a partitioned table is okay
>>> as long as it doesn't yet have any partitions.  Even if the modified
>>> type is a partitioning column, there's no structure yet that could
>>> depend on the contents of the type.  (If it does have partitions,
>>> we'll fail when we get to one of those.)
>> The following query leads to a failure on showing a partition definition:
>> CREATE TABLE tbl(a int, b int) PARTITION BY LIST ((tbl));
>> CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
>> ALTER TABLE tbl ALTER COLUMN a TYPE char(5);
> Sure, but there you already have a partition.  If you only had "tbl"
> then there would be no stored partition bounds.

Yes, that's a separate case when a value of the composite type "tbl" stored
in relpartbound for "tblp", but there is no dependency of "tbl"/"tblp" on the
type "tbl".

The similar situation:
CREATE TYPE ctype AS (a int, b int);
CREATE TABLE tbl(x ctype) PARTITION BY LIST (x);
CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
ALTER TYPE ctype ALTER ATTRIBUTE b TYPE char(5);
is prevented with
ERROR:  cannot alter type "ctype" because column "tbl.x" uses it
because there is dependency {"tbl", objsubid = 1} on the "ctype".

Regarding dropping attributes/columns, I've added the
find_composite_type_dependencies() call inside ATPrepDropColumn() and found
that it breaks cases that were valid before (according to regression tests). E.g.:
  CREATE TYPE test_type3 AS (a int);
  CREATE TABLE test_tbl3 (c) AS SELECT '(1)'::test_type3;
  ALTER TYPE test_type3 DROP ATTRIBUTE a, ADD ATTRIBUTE b int;
+ERROR:  cannot alter type "test_type3" because column "test_tbl3.c" uses it

So I agree, that disabling the drop operation when a composite type has
dependencies is not a harmless change. On the other hand, the code might be
not ready to deal with the uniqueness assumption violated.
For example:
CREATE TYPE ctype AS (a int, b int);
CREATE TABLE lp (t ctype) PARTITION BY RANGE (t);
CREATE TABLE lp_1_0 PARTITION OF lp FOR VALUES FROM (ROW(1, 01)::ctype) TO (ROW(1, 10)::ctype);
CREATE TABLE lp_1_1 PARTITION OF lp FOR VALUES FROM (ROW(1, 11)::ctype) TO (ROW(1, 20)::ctype);
ALTER TYPE ctype DROP ATTRIBUTE b;
INSERT INTO lp values(ROW(1)::ctype);
leads to an assertion failure:
...
#5  0x000055d3131e314f in ExceptionalCondition (
     conditionName=conditionName@entry=0x55d313357b85 "next_index == nparts",
     fileName=fileName@entry=0x55d313357887 "partbounds.c", lineNumber=lineNumber@entry=884) at assert.c:66
#6  0x000055d312fe2e2b in create_range_bounds (boundspecs=boundspecs@entry=0x55d314e4b028,
     nparts=nparts@entry=2, key=key@entry=0x55d314ed7170, mapping=mapping@entry=0x7fffa34bc438)
     at partbounds.c:884
#7  0x000055d312fe46c7 in partition_bounds_create (boundspecs=boundspecs@entry=0x55d314e4b028, nparts=2,
     key=key@entry=0x55d314ed7170, mapping=mapping@entry=0x7fffa34bc438) at partbounds.c:336
#8  0x000055d312fe6a2c in RelationBuildPartitionDesc (rel=rel@entry=0x7f609da02920,
     omit_detached=omit_detached@entry=true) at partdesc.c:271
#9  0x000055d312fe6c99 in RelationGetPartitionDesc (rel=rel@entry=0x7f609da02920,
     omit_detached=<optimized out>) at partdesc.c:110
...
Maybe that exact Assert could be replaced with just elog(ERROR), but I'm
afraid it's not the only such place that exists today and will appear
tomorrow. At least without a comprehensive testing this way seems dangerous
to me. Though maybe postgres should be robust enough to not crash in such
situations (caused by a composite type modification or by something else).

Best regards,
Alexander
--------------Us8UmO9FwdNYFANceilHWzt7
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">27.03.2023 22:06, Tom Lane wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:3332642.1679943981@sss.pgh.pa.us">
      <pre class="moz-quote-pre" wrap="">Alexander Lakhin <a class="moz-txt-link-rfc2396E" href="mailto:exclusion@gmail.com">&lt;exclusion@gmail.com&gt;</a> writes:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">27.03.2023 21:20, Tom Lane wrote:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">Hmm ... really?  I'd just concluded that a partitioned table is okay
as long as it doesn't yet have any partitions.  Even if the modified
type is a partitioning column, there's no structure yet that could
depend on the contents of the type.  (If it does have partitions,
we'll fail when we get to one of those.)
</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">The following query leads to a failure on showing a partition definition:
CREATE TABLE tbl(a int, b int) PARTITION BY LIST ((tbl));
CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');
ALTER TABLE tbl ALTER COLUMN a TYPE char(5);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Sure, but there you already have a partition.  If you only had "tbl"
then there would be no stored partition bounds.
</pre>
    </blockquote>
    <br>
    Yes, that's a separate case when a value of the composite type "tbl"
    stored<br>
    in relpartbound for "tblp", but there is no dependency of
    "tbl"/"tblp" on the<br>
    type "tbl".<br>
    <br>
    The similar situation:<br>
    CREATE TYPE ctype AS (a int, b int);<br>
    CREATE TABLE tbl(x ctype) PARTITION BY LIST (x);<br>
    CREATE TABLE tblp PARTITION OF tbl FOR VALUES IN ('(2,4)');<br>
    ALTER TYPE ctype ALTER ATTRIBUTE b TYPE char(5);<br>
    is prevented with<br>
    ERROR:  cannot alter type "ctype" because column "tbl.x" uses it<br>
    because there is dependency {"tbl", objsubid = 1} on the "ctype".<br>
    <br>
    Regarding dropping attributes/columns, I've added the<br>
    find_composite_type_dependencies() call inside ATPrepDropColumn()
    and found<br>
    that it breaks cases that were valid before (according to regression
    tests). E.g.:<br>
     CREATE TYPE test_type3 AS (a int);<br>
     CREATE TABLE test_tbl3 (c) AS SELECT '(1)'::test_type3;<br>
     ALTER TYPE test_type3 DROP ATTRIBUTE a, ADD ATTRIBUTE b int;<br>
    +ERROR:  cannot alter type "test_type3" because column "test_tbl3.c"
    uses it<br>
    <br>
    So I agree, that disabling the drop operation when a composite type
    has<br>
    dependencies is not a harmless change. On the other hand, the code
    might be<br>
    not ready to deal with the uniqueness assumption violated.<br>
    For example:<br>
    CREATE TYPE ctype AS (a int, b int);<br>
    CREATE TABLE lp (t ctype) PARTITION BY RANGE (t);<br>
    CREATE TABLE lp_1_0 PARTITION OF lp FOR VALUES FROM (ROW(1,
    01)::ctype) TO (ROW(1, 10)::ctype);<br>
    CREATE TABLE lp_1_1 PARTITION OF lp FOR VALUES FROM (ROW(1,
    11)::ctype) TO (ROW(1, 20)::ctype);<br>
    ALTER TYPE ctype DROP ATTRIBUTE b;<br>
    INSERT INTO lp values(ROW(1)::ctype);<br>
    leads to an assertion failure:<br>
    ...<br>
    #5  0x000055d3131e314f in ExceptionalCondition (<br>
        conditionName=conditionName@entry=0x55d313357b85 "next_index ==
    nparts",<br>
        fileName=fileName@entry=0x55d313357887 "partbounds.c",
    lineNumber=lineNumber@entry=884) at assert.c:66<br>
    #6  0x000055d312fe2e2b in create_range_bounds
    (boundspecs=boundspecs@entry=0x55d314e4b028,<br>
        nparts=nparts@entry=2, key=key@entry=0x55d314ed7170,
    mapping=mapping@entry=0x7fffa34bc438)<br>
        at partbounds.c:884<br>
    #7  0x000055d312fe46c7 in partition_bounds_create
    (boundspecs=boundspecs@entry=0x55d314e4b028, nparts=2,<br>
        key=key@entry=0x55d314ed7170,
    mapping=mapping@entry=0x7fffa34bc438) at partbounds.c:336<br>
    #8  0x000055d312fe6a2c in RelationBuildPartitionDesc
    (rel=rel@entry=0x7f609da02920,<br>
        omit_detached=omit_detached@entry=true) at partdesc.c:271<br>
    #9  0x000055d312fe6c99 in RelationGetPartitionDesc
    (rel=rel@entry=0x7f609da02920,<br>
        omit_detached=&lt;optimized out&gt;) at partdesc.c:110<br>
    ...<br>
    Maybe that exact Assert could be replaced with just elog(ERROR), but
    I'm<br>
    afraid it's not the only such place that exists today and will
    appear<br>
    tomorrow. At least without a comprehensive testing this way seems
    dangerous<br>
    to me. Though maybe postgres should be robust enough to not crash in
    such<br>
    situations (caused by a composite type modification or by something
    else).<br>
    <br>
    Best regards,<br>
    Alexander<br>
    <span style="font-family:monospace"></span>
  </body>
</html>

--------------Us8UmO9FwdNYFANceilHWzt7--