Thread: pg_restore error with partitioned table having exclude constraint

On Wed, 16 Apr 2025 at 23:11, Keith Paskett <keith.paskett@logansw.com> wrote:
> Postgresql 17.4
>
> A table partitioned by LIST with an exclusion constraint errors on creating the constraint on the parent table when
doing
> a pg_dump/pg_restore
>
> ERROR:
>
> pg_restore: error: could not execute query: ERROR:  cannot attach index
> "pkg708_had_working_hist_context_id_had_person_id_active_ts_excl" as a partition of index
"had_working_hist_tsr_excl"
>
> DETAIL:  The index definitions do not match.
>
> Command was: ALTER INDEX apps.had_working_hist_tsr_excl ATTACH PARTITION
> apps_part.pkg708_had_working_hist_context_id_had_person_id_active_ts_excl;
>
> TABLE AFTER pg_restore
>
> \d+ apps.had_working_hist
>
>                                                                   Partitioned table "apps.had_working_hist"
>
>        Column        |           Type           | Collation | Nullable |                    Default
 |
 
> Storage  | Compression | Stats target | Description 
>
>
---------------------+--------------------------+-----------+----------+------------------------------------------------+----------+-------------+--------------+-------------
>
>
>  had_working_hist_id | integer                  |           | not null |
nextval('apps.had_working_hist_seq'::regclass)|
 
> plain    |             |              | 
>
>  context_id          | integer                  |           | not null |
 |
 
> plain    |             |              | 
>
>  had_person_id       | integer                  |           | not null |
 |
 
> plain    |             |              | 
>
>  comment             | text                     |           |          |
 |
 
> extended |             |              | 
>
>  active_tsr          | tstzrange                |           | not null |
 |
 
> extended |             |              | 
>
>  add_tstz            | timestamp with time zone |           | not null | CURRENT_TIMESTAMP
 |
 
> plain    |             |              | 
>
>  add_by_id           | integer                  |           | not null |
 |
 
> plain    |             |              | 
>
>  mod_tstz            | timestamp with time zone |           | not null | CURRENT_TIMESTAMP
 |
 
> plain    |             |              | 
>
>  mod_by_id           | integer                  |           | not null |
 |
 
> plain    |             |              | 
>
> Partition key: LIST (context_id)
>
> Indexes:
>
>     "had_working_hist_pkey" PRIMARY KEY, btree (had_working_hist_id, context_id)
>
>     "had_working_hist_add_by_id_idx" btree (add_by_id)
>
>     "had_working_hist_had_person_id_idx" btree (had_person_id)
>
>     "had_working_hist_mod_by_id_idx" btree (mod_by_id)
>
>     "had_working_hist_tsr_excl" EXCLUDE USING gist (context_id WITH =, had_person_id WITH =, active_tsr WITH &&)
INVALID
>
> Foreign-key constraints:
>
>     "had_working_hist__add_by_id_fk" FOREIGN KEY (add_by_id) REFERENCES persons(person_id)
>
>     "had_working_hist__context_id_fk" FOREIGN KEY (context_id) REFERENCES apm_packages(package_id)
>
>     "had_working_hist__had_person_id_fk" FOREIGN KEY (had_person_id, context_id) REFERENCES apps.had_person
> (had_person_id, context_id)
>
>     "had_working_hist__mod_by_id_fk" FOREIGN KEY (mod_by_id) REFERENCES persons(person_id)
>
> Partitions: apps_part.pkg708_had_working_hist FOR VALUES IN (708)
>

Hi, Keith

I can replicate this issue on the current master branch. After some investigation,
I found the following code at the end of the CompareIndexInfo() function:

    /* No support currently for comparing exclusion indexes. */
    if (info1->ii_ExclusionOps != NULL || info2->ii_ExclusionOps != NULL)
        return false;

I believe this is why the exclusion index is rejected.  Commit 8b08f7d482
introduces a change that disables the creation of exclusion constraints on
partitioned tables, while commit 8c852ba9a4 allows some exclusion consistency
on partitions.

Here is a patch to fix it.  It just compares the OIDs of two exclusion constraints.



-- 
Regrads,
Japin Li

Hello,

On 2025-Apr-16, Keith Paskett wrote:

> A table partitioned by LIST with an exclusion constraint errors on
> creating the constraint on the parent table when doing a
> pg_dump/pg_restore

Was this working previously?

Thanks,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Thu, 17 Apr 2025 at 15:06, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Hello,
>
> On 2025-Apr-16, Keith Paskett wrote:
>
>> A table partitioned by LIST with an exclusion constraint errors on
>> creating the constraint on the parent table when doing a
>> pg_dump/pg_restore
>
> Was this working previously?
>
> Thanks,

It seems PG 16 does not support exclusion constraints on partitioned tables.

[local]:2119558 postgres=# SELECT version();
                                                version
-------------------------------------------------------------------------------------------------------
 PostgreSQL 16.8 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0, 64-bit
(1 row)

[local]:2119558 postgres=# CREATE TABLE had_working_hist (context_id serial not null, had_person_id integer not null,
active_tsrtstzrange not null) partition by LIST (context_id); 
CREATE TABLE
[local]:2119558 postgres=# ALTER TABLE had_working_hist ADD CONSTRAINT had_working_hist_tsr_excl EXCLUDE USING btree
(context_idWITH =, had_person_id WITH =); 
ERROR:  exclusion constraints are not supported on partitioned tables
LINE 1: ALTER TABLE had_working_hist ADD CONSTRAINT had_working_hist...
                                         ^

--
Regrads,
Japin Li

On 2025-Apr-17, Japin Li wrote:

> It seems PG 16 does not support exclusion constraints on partitioned tables.

Yeah, my recollection is that they were purposefully disallowed (mainly
because I didn't want to research how to fully make them work when
adding local partitioned indexes), and that we needed to do more work if
we wanted to let them through.  I suspect commit 8c852ba9a4 was mistaken
to allow that case without looking for further implications.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X  - http://www.thelinuxreview.com/TUX/)

On Thu, 17 Apr 2025 at 17:18, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-Apr-17, Japin Li wrote:
>
>> It seems PG 16 does not support exclusion constraints on partitioned tables.
>
> Yeah, my recollection is that they were purposefully disallowed (mainly
> because I didn't want to research how to fully make them work when
> adding local partitioned indexes), and that we needed to do more work if
> we wanted to let them through.  I suspect commit 8c852ba9a4 was mistaken
> to allow that case without looking for further implications.
>

Sorry, I’m unclear on “more work.” Can you explain further?

--
Regrads,
Japin Li

On 2025-Apr-24, Japin Li wrote:

> On Thu, 17 Apr 2025 at 17:18, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > On 2025-Apr-17, Japin Li wrote:
> >
> >> It seems PG 16 does not support exclusion constraints on
> >> partitioned tables.
> >
> > Yeah, my recollection is that they were purposefully disallowed
> > (mainly because I didn't want to research how to fully make them
> > work when adding local partitioned indexes), and that we needed to
> > do more work if we wanted to let them through.  I suspect commit
> > 8c852ba9a4 was mistaken to allow that case without looking for
> > further implications.
> 
> Sorry, I’m unclear on “more work.” Can you explain further?

Well, there are no tests in the patch.  8c852ba9a434 added some, but
it's now clear that something was overlooked.  I think this patch should
make more of an effort to cover all interesting cases in regression
tests if there are holes in coverage; and also add something to verify
that pg_dump and pg_upgrade work correctly for these constraints.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Si quieres ser creativo, aprende el arte de perder el tiempo"

On Mon, 28 Apr 2025 at 10:42, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-Apr-24, Japin Li wrote:
>
>> On Thu, 17 Apr 2025 at 17:18, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>> > On 2025-Apr-17, Japin Li wrote:
>> >
>> >> It seems PG 16 does not support exclusion constraints on
>> >> partitioned tables.
>> >
>> > Yeah, my recollection is that they were purposefully disallowed
>> > (mainly because I didn't want to research how to fully make them
>> > work when adding local partitioned indexes), and that we needed to
>> > do more work if we wanted to let them through.  I suspect commit
>> > 8c852ba9a4 was mistaken to allow that case without looking for
>> > further implications.
>>
>> Sorry, I’m unclear on “more work.” Can you explain further?
>
> Well, there are no tests in the patch.  8c852ba9a434 added some, but
> it's now clear that something was overlooked.  I think this patch should
> make more of an effort to cover all interesting cases in regression
> tests if there are holes in coverage; and also add something to verify
> that pg_dump and pg_upgrade work correctly for these constraints.
>

My understanding, based on the src/bin/pg_dump tests, is that they don't
involve a genuine restore of the dumped data to a database.  Instead, it
dumps to a file using pg_restore.   Is that correct?

I doubt whether I can add a test to pg_dump that would cover this issue.

--
Regrads,
Japin Li

On 2025-Apr-29, Japin Li wrote:

> My understanding, based on the src/bin/pg_dump tests, is that they don't
> involve a genuine restore of the dumped data to a database.  Instead, it
> dumps to a file using pg_restore.   Is that correct?

Yes.

> I doubt whether I can add a test to pg_dump that would cover this issue.

The pg_upgrade now includes a roundtrip dump/restore which you can take
advantage of.  You just need to ensure some of the src/test/regress/sql
files leave an object in the state you need, and the pg_upgrade test
will run those tests and try to dump and restore the resulting database.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
 algunas personas nos parecen brillantes un minuto antes
 de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

On Tue, 29 Apr 2025 at 16:53, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-Apr-29, Japin Li wrote:
>
>> My understanding, based on the src/bin/pg_dump tests, is that they don't
>> involve a genuine restore of the dumped data to a database.  Instead, it
>> dumps to a file using pg_restore.   Is that correct?
>
> Yes.
>
>> I doubt whether I can add a test to pg_dump that would cover this issue.
>
> The pg_upgrade now includes a roundtrip dump/restore which you can take
> advantage of.  You just need to ensure some of the src/test/regress/sql
> files leave an object in the state you need, and the pg_upgrade test
> will run those tests and try to dump and restore the resulting database.
>

Thank you for the explanation. A test case has been added to create_index.sql.
Could you please take a look?

--
Regrads,
Japin Li

On 2025-Apr-30, Japin Li wrote:

> Thank you for the explanation. A test case has been added to create_index.sql.
> Could you please take a look?

Well, it seems a bit minimalistic -- I would try to be more adversarial
about it maybe, because details are where devil(s) lie.

You need to add comments in CompareIndexInfo about your new code.  Why
is it okay to ignore ii_ExclusionProcs and ii_ExclusionStrats?  Why is
it okay to not have tests that set up tables with those things as
different so that this function returns false in these cases?  Why do
you have a test for a table set up where the positive case is handled,
but no case for the negative case?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/