public inbox for [email protected]  

help / color / mirror / Atom feed

From: vignesh C <[email protected]>
To: Amul Sul <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
Date: Sat, 20 Jan 2024 07:54:52 +0530
Message-ID: <CALDaNm0ELv1v+DUw3iOfbOpCHqW49kFmaU2QN3APFeFp_ge2dg@mail.gmail.com> (raw)
In-Reply-To: <CAAJ_b94vRvR8AymA08Dz9Cs_3rHq+dRfY_-tnuJU48jkEuojwg@mail.gmail.com>
References: <CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com>
	<[email protected]>
	<CAAJ_b94vRvR8AymA08Dz9Cs_3rHq+dRfY_-tnuJU48jkEuojwg@mail.gmail.com>

On Fri, 22 Sept 2023 at 18:45, Amul Sul <[email protected]> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <[email protected]> wrote:
>>
>> On 2023-Sep-20, Amul Sul wrote:
>>
>> > On the latest master head, I can see a $subject bug that seems to be related
>> > commit #b0e96f311985:
>> >
>> > Here is the table definition:
>> > create table foo(i int, j int, CONSTRAINT pk PRIMARY KEY(i) DEFERRABLE);
>>
>> Interesting, thanks for the report.  Your attribution to that commit is
>> correct.  The table is dumped like this:
>>
>> CREATE TABLE public.foo (
>>     i integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT,
>>     j integer
>> );
>> ALTER TABLE ONLY public.foo
>>     ADD CONSTRAINT pk PRIMARY KEY (i) DEFERRABLE;
>> ALTER TABLE ONLY public.foo DROP CONSTRAINT pgdump_throwaway_notnull_0;
>>
>> so the problem here is that the deferrable PK is not considered a reason
>> to keep attnotnull set, so we produce a throwaway constraint that we
>> then drop.  This is already bogus, but what is more bogus is the fact
>> that the backend accepts the DROP CONSTRAINT at all.
>>
>> The pg_dump failing should be easy to fix, but fixing the backend to
>> error out sounds more critical.  So, the reason for this behavior is
>> that RelationGetIndexList doesn't want to list an index that isn't
>> marked indimmediate as a primary key.  I can easily hack around that by
>> doing
>>
>>         diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
>>         index 7234cb3da6..971d9c8738 100644
>>         --- a/src/backend/utils/cache/relcache.c
>>         +++ b/src/backend/utils/cache/relcache.c
>>         @@ -4794,7 +4794,6 @@ RelationGetIndexList(Relation relation)
>>                          * check them.
>>                          */
>>                         if (!index->indisunique ||
>>         -                       !index->indimmediate ||
>>                                 !heap_attisnull(htup, Anum_pg_index_indpred, NULL))
>>                                 continue;
>>
>>         @@ -4821,6 +4820,9 @@ RelationGetIndexList(Relation relation)
>>                                  relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
>>                                 pkeyIndex = index->indexrelid;
>>
>>         +               if (!index->indimmediate)
>>         +                       continue;
>>         +
>>                         if (!index->indisvalid)
>>                                 continue;
>>
>>
>> But of course this is not great, since it impacts unrelated bits of code
>> that are relying on relation->pkindex or RelationGetIndexAttrBitmap
>> having their current behavior with non-immediate index.
>
>
> True, but still wondering why would relation->rd_pkattr skipped for a
> deferrable primary key, which seems to be a bit incorrect to me since it
> sensing that relation doesn't have PK at all.  Anyway, that is unrelated.
>
>>
>> I think a real solution is to stop relying on RelationGetIndexAttrBitmap
>> in ATExecDropNotNull().  (And, again, pg_dump needs some patch as well
>> to avoid printing a throwaway NOT NULL constraint at this point.)
>
>
> I might not have understood this, but I think, if it is ok to skip throwaway NOT
> NULL for deferrable PK then that would be enough for the reported issue
> to be fixed.  I quickly tried with the attached patch which looks sufficient
> to skip that, but, TBH, I haven't thought carefully about this change.

I did not see any test addition for this, can we add it?

Regards,
Vignesh

view thread (8+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).
  In-Reply-To: <CALDaNm0ELv1v+DUw3iOfbOpCHqW49kFmaU2QN3APFeFp_ge2dg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox