Horton

Thread

Commits

  1. Fix ALTER TABLE .. ADD COLUMN with complex inheritance trees

  1. BUG #18297: Error when adding a column to a parent table with complex inheritance

    The Post Office <noreply@postgresql.org> — 2024-01-16T04:00:02Z

    The following bug has been logged on the website:

Bug reference:      18297
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 16.1
Operating system:   Ubuntu 22.04
Description:        

The following query:
CREATE TABLE a ();
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (b);
CREATE TABLE d () INHERITS (a,b,c);

ALTER TABLE a ADD COLUMN i int;
fails with:
NOTICE:  merging definition of column "i" for child "d"
ERROR:  tuple already updated by self

While with a simpler hierarchy:
CREATE TABLE a ();
CREATE TABLE b () INHERITS (a);
CREATE TABLE c () INHERITS (a,b);

the column added successfully:
ALTER TABLE a ADD COLUMN i int;
NOTICE:  merging definition of column "i" for child "c"
ALTER TABLE

In the failed case the error occurred when table d was processed the third
time. First (following chain a -> b -> c-> d) the table got the column i
added, second (a -> b -> d) it got merged column definition, third
(a -> d) an attempt to merge the column definition once more failed.

This error can be seen at least on REL_12_STABLE .. master.

  2. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Tender Wang <tndrwang@gmail.com> — 2024-01-17T07:48:23Z

    Hmm, thanks for the report.
I can repeat the aboved issue on master, even on pg10 and pg 11.
I analyzed this issue, and I found that ATExecAddColumn(), we forgot to
call CommandCounterIncrement() in if (colDef->inhcount > 0) {...} branch.
So the third(a->d) updates the first(a->b->c->d) tuple.
Attached patch is my quickly fixed solution.

--
Tender Wang
OpenPie:  https://en.openpie.com/


PG Bug reporting form <noreply@postgresql.org> 于2024年1月16日周二 17:32写道：

> The following bug has been logged on the website:
>
> Bug reference:      18297
> Logged by:          Alexander Lakhin
> Email address:      exclusion@gmail.com
> PostgreSQL version: 16.1
> Operating system:   Ubuntu 22.04
> Description:
>
> The following query:
> CREATE TABLE a ();
> CREATE TABLE b () INHERITS (a);
> CREATE TABLE c () INHERITS (b);
> CREATE TABLE d () INHERITS (a,b,c);
>
> ALTER TABLE a ADD COLUMN i int;
> fails with:
> NOTICE:  merging definition of column "i" for child "d"
> ERROR:  tuple already updated by self
>
> While with a simpler hierarchy:
> CREATE TABLE a ();
> CREATE TABLE b () INHERITS (a);
> CREATE TABLE c () INHERITS (a,b);
>
> the column added successfully:
> ALTER TABLE a ADD COLUMN i int;
> NOTICE:  merging definition of column "i" for child "c"
> ALTER TABLE
>
> In the failed case the error occurred when table d was processed the third
> time. First (following chain a -> b -> c-> d) the table got the column i
> added, second (a -> b -> d) it got merged column definition, third
> (a -> d) an attempt to merge the column definition once more failed.
>
> This error can be seen at least on REL_12_STABLE .. master.
>
>

  3. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Richard Guo <guofenglinux@gmail.com> — 2024-01-17T11:54:26Z

    On Wed, Jan 17, 2024 at 3:48 PM Tender Wang <tndrwang@gmail.com> wrote:

> Hmm, thanks for the report.
> I can repeat the aboved issue on master, even on pg10 and pg 11.
> I analyzed this issue, and I found that ATExecAddColumn(), we forgot to
> call CommandCounterIncrement() in if (colDef->inhcount > 0) {...} branch.
> So the third(a->d) updates the first(a->b->c->d) tuple.
> Attached patch is my quickly fixed solution.
>

Indeed.  We may update the same child column multiple times, but there
is no CommandCounterIncrement between.  Nice catch and +1 to the fix.

To nitpick, how about go with the comment as

    /* Make sure the child column change is visible */

which seems clearer.

Also I think it'd be better to include a blank line before and after the
new CommandCounterIncrement statement.

Also I suggest to drop the new added tables after we've run ALTER TABLE
in the test case.

Thanks
Richard

  4. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Tender Wang <tndrwang@gmail.com> — 2024-01-17T15:03:56Z

    Thanks for reviewing the patch.
The attached v2 patch includes all review advices.

--
Tender Wang
OpenPie:  https://en.openpie.com/


Richard Guo <guofenglinux@gmail.com> 于2024年1月17日周三 19:54写道：

>
> On Wed, Jan 17, 2024 at 3:48 PM Tender Wang <tndrwang@gmail.com> wrote:
>
>> Hmm, thanks for the report.
>> I can repeat the aboved issue on master, even on pg10 and pg 11.
>> I analyzed this issue, and I found that ATExecAddColumn(), we forgot to
>> call CommandCounterIncrement() in if (colDef->inhcount > 0) {...} branch.
>> So the third(a->d) updates the first(a->b->c->d) tuple.
>> Attached patch is my quickly fixed solution.
>>
>
> Indeed.  We may update the same child column multiple times, but there
> is no CommandCounterIncrement between.  Nice catch and +1 to the fix.
>
> To nitpick, how about go with the comment as
>
>     /* Make sure the child column change is visible */
>
> which seems clearer.
>
> Also I think it'd be better to include a blank line before and after the
> new CommandCounterIncrement statement.
>
> Also I suggest to drop the new added tables after we've run ALTER TABLE
> in the test case.
>
> Thanks
> Richard
>

  5. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Alexander Law <exclusion@gmail.com> — 2024-01-18T09:00:01Z

    Hello Tender Wang and Richard,

17.01.2024 18:03, Tender Wang wrote:
> Thanks for reviewing the patch.
> The attached v2 patch includes all review advices.
>
> --
> Tender Wang
> OpenPie: https://en.openpie.com/
>
> Richard Guo <guofenglinux@gmail.com> 于2024年1月17日周三 19:54写道：
>
>
>     Indeed.  We may update the same child column multiple times, but there
>     is no CommandCounterIncrement between.  Nice catch and +1 to the fix.
>
>     To nitpick, how about go with the comment as
>
>         /* Make sure the child column change is visible */
>
>     which seems clearer.
>
>     Also I think it'd be better to include a blank line before and after the
>     new CommandCounterIncrement statement.
>
>     Also I suggest to drop the new added tables after we've run ALTER TABLE
>     in the test case.
>
>     Thanks
>     Richard
>

Thank you for working on this!

(Maybe it's possible to slightly modify an existing hierarchy with tables
a, b, c in inherit.sql for the purpose of the check added.)

I've found another situation where the same error is produced:
CREATE ROLE u;
DROP ROLE u, u;
ERROR:  tuple already updated by self

Perhaps, you would like to fix it in passing too. I've rechecked all the
other object types, that can be DROPped with a list (namely, AGGREGATE,
DOMAIN, EXTENSION, FOREIGN DATA WRAPPER, FOREIGN TABLE, FUNCTION, INDEX,
MATERIALIZED VIEW, OPERATOR, PROCEDURE, ROUTINE, SEQUENCE, SERVER,
STATISTICS, TABLE, TYPE, VIEW), and found that all of these handle
such duplicate entries with no error.

Best regards,
Alexander

  6. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Tender Wang <tndrwang@gmail.com> — 2024-01-18T15:42:21Z

    Alexander Lakhin <exclusion@gmail.com> 于2024年1月18日周四 17:00写道：

> Hello Tender Wang and Richard,
>
> 17.01.2024 18:03, Tender Wang wrote:
>
> Thanks for reviewing the patch.
> The attached v2 patch includes all review advices.
>
> --
> Tender Wang
> OpenPie:  https://en.openpie.com/
>
> Richard Guo <guofenglinux@gmail.com> 于2024年1月17日周三 19:54写道：
>
>>
>> Indeed.  We may update the same child column multiple times, but there
>> is no CommandCounterIncrement between.  Nice catch and +1 to the fix.
>>
>> To nitpick, how about go with the comment as
>>
>>     /* Make sure the child column change is visible */
>>
>> which seems clearer.
>>
>> Also I think it'd be better to include a blank line before and after the
>> new CommandCounterIncrement statement.
>>
>> Also I suggest to drop the new added tables after we've run ALTER TABLE
>> in the test case.
>>
>> Thanks
>> Richard
>>
>
> Thank you for working on this!
>
> (Maybe it's possible to slightly modify an existing hierarchy with tables
> a, b, c in inherit.sql for the purpose of the check added.)
>
> I've found another situation where the same error is produced:
> CREATE ROLE u;
> DROP ROLE u, u;
> ERROR:  tuple already updated by self
>

Indeed, but this issue cann't simply calling CommandCounterIncrement() to
fix.
It will report "could not find tuple for role" if we simply calling
CommandCounterIncrement() when drop seond 'u'.

I think we can sort the role_addresses list and skip the same role id. I
don't intend to fix above two issues in one patch.
So I add a new 0001 attached patch.


Perhaps, you would like to fix it in passing too. I've rechecked all the
> other object types, that can be DROPped with a list (namely, AGGREGATE,
> DOMAIN, EXTENSION, FOREIGN DATA WRAPPER, FOREIGN TABLE, FUNCTION, INDEX,
> MATERIALIZED VIEW, OPERATOR, PROCEDURE, ROUTINE, SEQUENCE, SERVER,
> STATISTICS, TABLE, TYPE, VIEW), and found that all of these handle
> such duplicate entries with no error.
>

I'm not sure if there are other problems like this.


> Best regards,
> Alexander
>

  7. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Alexander Law <exclusion@gmail.com> — 2024-01-19T09:00:00Z

    18.01.2024 18:42, Tender Wang wrote:

>
> I think we can sort the role_addresses list and skip the same role id. I don't intend to fix above two issues in one 
> patch.
> So I add a new 0001 attached patch.
>
>
>     Perhaps, you would like to fix it in passing too. I've rechecked all the
>     other object types, that can be DROPped with a list (namely, AGGREGATE,
>     DOMAIN, EXTENSION, FOREIGN DATA WRAPPER, FOREIGN TABLE, FUNCTION, INDEX,
>     MATERIALIZED VIEW, OPERATOR, PROCEDURE, ROUTINE, SEQUENCE, SERVER,
>     STATISTICS, TABLE, TYPE, VIEW), and found that all of these handle
>     such duplicate entries with no error.
>
>
> I'm not sure if there are other problems like this.
>

Yes, I've encountered a similar issue, this time with ALTER (TEXT SEARCH
CONFIGURATION):
CREATE TEXT SEARCH CONFIGURATION ispell_tst (COPY=english);
CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell, DictFile=ispell_sample, AffFile=ispell_sample);
ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word WITH ispell;
ERROR:  tuple already updated by self

And again, I could not get similar errors with all the other ALTER commands,
that accept lists.

Sorry for mixing two different problems together, I should have reported it
separately.

Best regards,
Alexander

  8. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Tender Wang <tndrwang@gmail.com> — 2024-01-22T10:36:47Z

    Alexander Lakhin <exclusion@gmail.com> 于2024年1月19日周五 17:00写道：

> 18.01.2024 18:42, Tender Wang wrote:
>
>
> I think we can sort the role_addresses list and skip the same role id. I
> don't intend to fix above two issues in one patch.
> So I add a new 0001 attached patch.
>
>
> Perhaps, you would like to fix it in passing too. I've rechecked all the
>> other object types, that can be DROPped with a list (namely, AGGREGATE,
>> DOMAIN, EXTENSION, FOREIGN DATA WRAPPER, FOREIGN TABLE, FUNCTION, INDEX,
>> MATERIALIZED VIEW, OPERATOR, PROCEDURE, ROUTINE, SEQUENCE, SERVER,
>> STATISTICS, TABLE, TYPE, VIEW), and found that all of these handle
>> such duplicate entries with no error.
>>
>
> I'm not sure if there are other problems like this.
>
>
> Yes, I've encountered a similar issue, this time with ALTER (TEXT SEARCH
> CONFIGURATION):
> CREATE TEXT SEARCH CONFIGURATION ispell_tst (COPY=english);
> CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell,
> DictFile=ispell_sample, AffFile=ispell_sample);
> ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word
> WITH ispell;
> ERROR:  tuple already updated by self
>

Yes, this is another issue that DDL operation on an same object twice.
Maybe we can report error on Parse phase.
But as you say, all the other DDL commands run OK, so maybe it's better to
process this case in execution phase.
I will send a patch later.

And I'm just curious that how do you find these issues? Use some tools?


>
> And again, I could not get similar errors with all the other ALTER
> commands,
> that accept lists.
>
> Sorry for mixing two different problems together, I should have reported it
> separately.
>
> Best regards,
> Alexander
>


-- 
--
Tender Wang
OpenPie:  https://en.openpie.com/

  9. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Alexander Law <exclusion@gmail.com> — 2024-01-22T17:00:01Z

    Hi,

22.01.2024 13:36, Tender Wang wrote:
>
>     Yes, I've encountered a similar issue, this time with ALTER (TEXT SEARCH
>     CONFIGURATION):
>     CREATE TEXT SEARCH CONFIGURATION ispell_tst (COPY=english);
>     CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell, DictFile=ispell_sample, AffFile=ispell_sample);
>     ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word WITH ispell;
>     ERROR:  tuple already updated by self
>
>
> Yes, this is another issue that DDL operation on an same object twice. Maybe we can report error on Parse phase.
> But as you say, all the other DDL commands run OK, so maybe it's better to process this case in execution phase.
> I will send a patch later.

Thank you for working on this!

As these two cases look like exceptions to the common behavior, I wonder
whether we need some extra functions to deal with duplicates.
(I haven't look yet how such duplicates processed for other object types.)

>
> And I'm just curious that how do you find these issues? Use some tools?

I discovered these two issues with my kind of fuzzer, just watching out
for the interesting errors.

Best regards,
Alexander

  10. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Tender Wang <tndrwang@gmail.com> — 2024-01-23T03:19:57Z

    Alexander Lakhin <exclusion@gmail.com> 于2024年1月23日周二 01:00写道：

> Hi,
>
> 22.01.2024 13:36, Tender Wang wrote:
>
>
> Yes, I've encountered a similar issue, this time with ALTER (TEXT SEARCH
>> CONFIGURATION):
>> CREATE TEXT SEARCH CONFIGURATION ispell_tst (COPY=english);
>> CREATE TEXT SEARCH DICTIONARY ispell (Template=ispell,
>> DictFile=ispell_sample, AffFile=ispell_sample);
>> ALTER TEXT SEARCH CONFIGURATION ispell_tst ALTER MAPPING FOR word, word
>> WITH ispell;
>> ERROR:  tuple already updated by self
>>
>
> Yes, this is another issue that DDL operation on an same object twice.
> Maybe we can report error on Parse phase.
> But as you say, all the other DDL commands run OK, so maybe it's better to
> process this case in execution phase.
> I will send a patch later.
>
>
> Thank you for working on this!
>
> As these two cases look like exceptions to the common behavior, I wonder
> whether we need some extra functions to deal with duplicates.
> (I haven't look yet how such duplicates processed for other object types.)
>

For example,  drop extension pageinspect ,pageinspect;
The duplicates will be done in object_address_present_add_flags(), some
codes as below:
....
  if (object->classId == thisobj->classId &&
object->objectId == thisobj->objectId)
{
if (object->objectSubId == thisobj->objectSubId)
{
ObjectAddressExtra *thisextra = addrs->extras + i;

thisextra->flags |= flags;
result = true;
}
....
The object will not be added to targetObjects if above func returned true.

Your reported two issues have different code path, and duplicates do not
processed.

I am considering whether there is a unified way of handling this, otherwise
each one will have to be dealt with separately.


>
> And I'm just curious that how do you find these issues? Use some tools?
>
>
>
> I discovered these two issues with my kind of fuzzer, just watching out
> for the interesting errors.
>
> Best regards,
> Alexander
>

--
Tender Wang
OpenPie:  https://en.openpie.com/

  11. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Michael Paquier <michael@paquier.xyz> — 2024-01-24T06:12:53Z

    On Thu, Jan 18, 2024 at 12:00:01PM +0300, Alexander Lakhin wrote:
> Thank you for working on this!

Yep, this one is obviously wrong so I have applied and backpatched the
fix for tablecmds.c.   

> (Maybe it's possible to slightly modify an existing hierarchy with tables
> a, b, c in inherit.sql for the purpose of the check added.)

I was wondering about that, but decided to keep a separate test as
suggested as the other tests with inheritance are already tightly
linked to their own assumptions, mostly with renames and constraints.
--
Michael

  12. Re: BUG #18297: Error when adding a column to a parent table with complex inheritance

    Michael Paquier <michael@paquier.xyz> — 2024-01-24T06:20:42Z

    On Tue, Jan 23, 2024 at 11:19:57AM +0800, Tender Wang wrote:
> Your reported two issues have different code path, and duplicates do not
> processed.
> 
> I am considering whether there is a unified way of handling this, otherwise
> each one will have to be dealt with separately.

Yes, I don't agree with what you have posted on [1], inventing a new
way to handle what is basically a way to remove duplicated
ObjectAddresses, and that's what we use in the DropRole() path.

The problem reported on this thread has now been fixed as of
bb812ab0917e, so why not moving the rest to a different thread with a
proper subject?  This will attract a better audience for the topic
dealt with.

[1]: https://www.postgresql.org/message-id/CAHewXN=dPJDvaoME0G9vyemUmY-TpDuqQfcHJRWfUvSWX1p=rQ@mail.gmail.com
--
Michael