Thread: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).

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.

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

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

On Fri, 22 Sept 2023 at 18:45, Amul Sul <sulamul@gmail.com> wrote:
>
>
>
> On Wed, Sep 20, 2023 at 8:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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

Hi hackers,

>> I did not see any test addition for this, can we add it?
>
>
> Ok, added it in the attached version.
>
> This was an experimental patch, I was looking for the comment on the proposed
> approach -- whether we could simply skip the throwaway NOT NULL constraint for
> deferred PK constraint.  Moreover,  skip that for any PK constraint.

I confirm that the patch fixes the bug. All the tests pass. Looks like
RfC to me.

-- 
Best regards,
Aleksander Alekseev

On Mon, 4 Mar 2024 at 12:34, Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> > This was an experimental patch, I was looking for the comment on the proposed
> > approach -- whether we could simply skip the throwaway NOT NULL constraint for
> > deferred PK constraint.  Moreover,  skip that for any PK constraint.
>
> I confirm that the patch fixes the bug. All the tests pass. Looks like
> RfC to me.
>

I don't think that this is the right fix. ISTM that the real issue is
that dropping a NOT NULL constraint should not mark the column as
nullable if it is part of a PK, whether or not that PK is deferrable
-- a deferrable PK still marks a  column as not nullable.

The reason pg_dump creates these throwaway NOT NULL constraints is to
avoid a table scan to check for NULLs when the PK is later created.
That rationale still applies to deferrable PKs, so we still want the
throwaway NOT NULL constraints in that case, otherwise we'd be hurting
performance of restore.

Regards,
Dean

On 2024-Mar-04, Dean Rasheed wrote:

> I don't think that this is the right fix. ISTM that the real issue is
> that dropping a NOT NULL constraint should not mark the column as
> nullable if it is part of a PK, whether or not that PK is deferrable
> -- a deferrable PK still marks a  column as not nullable.

Yeah.  As I said upthread, a good fix seems to require no longer relying
on RelationGetIndexAttrBitmap to obtain the columns in the primary key,
because that function does not include deferred primary keys.  I came up
with the attached POC, which seems to fix the reported problem, but of
course it needs more polish, a working test case, and verifying whether
the new function should be used in more places -- in particular, whether
it can be used to revert the changes to RelationGetIndexList that
b0e96f311985 did.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas    / desprovistas, por cierto
 de blandos atenuantes"                          (Patricio Vogel)

On Tue, 5 Mar 2024 at 12:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Yeah.  As I said upthread, a good fix seems to require no longer relying
> on RelationGetIndexAttrBitmap to obtain the columns in the primary key,
> because that function does not include deferred primary keys.  I came up
> with the attached POC, which seems to fix the reported problem, but of
> course it needs more polish, a working test case, and verifying whether
> the new function should be used in more places -- in particular, whether
> it can be used to revert the changes to RelationGetIndexList that
> b0e96f311985 did.
>

Looking at the other places that call RelationGetIndexAttrBitmap()
with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include
deferrable PKs, since they are relying on the result to see which
columns are not nullable.

So there are other bugs here. For example:

CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text);
CREATE TABLE bar (LIKE foo);

now fails to mark bar.id as not nullable, whereas prior to
b0e96f311985 it would have been.

So I think RelationGetIndexAttrBitmap() should include deferrable PKs,
but not all the changes made to RelationGetIndexList() by b0e96f311985
need reverting.

Regards,
Dean

On 2024-Mar-05, Dean Rasheed wrote:

> Looking at the other places that call RelationGetIndexAttrBitmap()
> with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include
> deferrable PKs, since they are relying on the result to see which
> columns are not nullable.

Hmm, I find this pretty surprising, but you are right.  Somehow I had
the idea that INDEX_ATTR_BITMAP_PRIMARY_KEY was used for planning
activities so I didn't want to modify its behavior ... but clearly
that's not at all the case.  It's only used for DDL, and one check in
logical replication.

> So there are other bugs here. For example:
> 
> CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text);
> CREATE TABLE bar (LIKE foo);
> 
> now fails to mark bar.id as not nullable, whereas prior to
> b0e96f311985 it would have been.

Fun.  (Thankfully, easy to fix. But I'll add this as a test too.)

> So I think RelationGetIndexAttrBitmap() should include deferrable PKs,

Yeah, I'll go make it so.  I think I'll add a test for the case that
changes behavior in logical replication first (which is that the target
relation of logical replication is currently not marked as updatable,
when its PK is deferrable).

> but not all the changes made to RelationGetIndexList() by b0e96f311985
> need reverting.

I'll give this a look too.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

On 2024-Mar-05, Dean Rasheed wrote:

> So I think RelationGetIndexAttrBitmap() should include deferrable PKs,

I tried this, but it doesn't actually lead to a good place, because if
we allow deferrable PKs to identify rows, then they are not useful to
find the tuple to update when replicating.  Consider the following case:

$node_publisher->safe_psql('postgres',
    'create table deferred_pk (id int primary key initially deferred, hidden int, value text)');
$node_subscriber->safe_psql('postgres',
    'create table deferred_pk (id int primary key initially deferred, hidden int, value text)');
$node_subscriber->safe_psql('postgres',
    'alter subscription tap_sub refresh publication');

$node_publisher->safe_psql('postgres',
    "insert into deferred_pk (id, hidden, value) values (1, 1, 'first')");
$node_publisher->wait_for_catchup('tap_sub');
$node_publisher->safe_psql('postgres',
    qq{
    begin;
    insert into deferred_pk values (1, 2, 'conflicting');
    update deferred_pk set value = value || ', updated' where id = 1 and hidden = 2;
    update deferred_pk set id = 3, value = value || ', updated' where hidden = 2;
    commit});
$node_publisher->wait_for_catchup('tap_sub');
my $pubdata = $node_publisher->safe_psql('postgres',
    'select * from deferred_pk order by id');
my $subsdata = $node_subscriber->safe_psql('postgres',
    'select * from deferred_pk order by id');
is($subsdata, $pubdata, "data is equal");

Here, the publisher's transaction first creates a new record with the
same PK, which only works because the PK is deferred; then we update its
payload column.  When this is replicated, the row is identified by the
PK ... but replication actually updates the other row, because it's
found first:

#   Failed test 'data is equal'
#   at t/003_constraints.pl line 163.
#          got: '1|2|conflicting
# 3|2|conflicting, updated, updated'
#     expected: '1|1|first
# 3|2|conflicting, updated, updated'

Actually, is that what happened here?  I'm not sure, but clearly this is
bogus.

So I think the original developers of REPLICA IDENTITY had the right
idea here (commit 07cacba983ef), and we mustn't change this aspect,
because it'll lead to data corruption in replication.  Using a deferred
PK for DDL considerations seems OK, but it seems certain that for actual
data replication it's going to be a disaster.

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

On Thu, 7 Mar 2024 at 13:00, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> So I think the original developers of REPLICA IDENTITY had the right
> idea here (commit 07cacba983ef), and we mustn't change this aspect,
> because it'll lead to data corruption in replication.  Using a deferred
> PK for DDL considerations seems OK, but it seems certain that for actual
> data replication it's going to be a disaster.
>

Yes, that makes sense. If I understand correctly though, the
replication code uses relation->rd_replidindex (not
relation->rd_pkindex), although sometimes it's the same thing. So can
we get away with making sure that RelationGetIndexList() doesn't set
relation->rd_replidindex to a deferrable PK, while still allowing
relation->rd_pkindex to be one?

Regards,
Dean

On 2024-Mar-07, Dean Rasheed wrote:

> On Thu, 7 Mar 2024 at 13:00, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > So I think the original developers of REPLICA IDENTITY had the right
> > idea here (commit 07cacba983ef), and we mustn't change this aspect,
> > because it'll lead to data corruption in replication.  Using a deferred
> > PK for DDL considerations seems OK, but it seems certain that for actual
> > data replication it's going to be a disaster.
> 
> Yes, that makes sense. If I understand correctly though, the
> replication code uses relation->rd_replidindex (not
> relation->rd_pkindex), although sometimes it's the same thing. So can
> we get away with making sure that RelationGetIndexList() doesn't set
> relation->rd_replidindex to a deferrable PK, while still allowing
> relation->rd_pkindex to be one?

Well, not really, because the logical replication code for some reason
uses GetRelationIdentityOrPK(), which falls back to rd->pk_index (via
RelationGetPrimaryKeyIndex) if rd_replindex is not set.

Maybe we can add a flag RelationData->rd_ispkdeferred, so that
RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
logical replication would continue to not know about this PK, which
perhaps is what we want.  I'll do some testing with this.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 2024-Mar-07, Alvaro Herrera wrote:

> Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> logical replication would continue to not know about this PK, which
> perhaps is what we want.  I'll do some testing with this.

This seems to work okay.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

On Thu, 7 Mar 2024 at 17:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> This seems to work okay.
>

Yes, this looks good. I tested it against CREATE TABLE ... LIKE, and
it worked as expected. It might be worth adding a test case for that,
to ensure that it doesn't get broken in the future. Do we also want a
test case that does what pg_dump would do:

 - Add a NOT NULL constraint
 - Add a deferrable PK constraint
 - Drop the NOT NULL constraint
 - Check the column is still not nullable

Looking at rel.h, I think that the new field should probably come
after rd_pkindex, under the comment "data managed by
RelationGetIndexList", and have its own comment.

Also, if I'm nitpicking, the new field and local variables should use
the term "deferrable" rather than "deferred". A DEFERRABLE constraint
can be set to be either DEFERRED or IMMEDIATE within a transaction,
but "deferrable" is the right term to use to describe the persistent
property of an index/constraint that can be deferred. (The same
objection applies to the field name "indimmediate", but it's too late
to change that.)

Also, for neatness/consistency, the new field should probably be reset
in load_relcache_init_file(), alongside rd_pkindex, though I don't
think it can matter in practice.

Regards,
Dean

On Thu, Mar 7, 2024 at 12:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Mar-07, Alvaro Herrera wrote:
> > Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> > RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> > logical replication would continue to not know about this PK, which
> > perhaps is what we want.  I'll do some testing with this.
>
> This seems to work okay.

There is a CommitFest entry for this patch. Should that entry be
closed in view of the not-NULL revert
(6f8bb7c1e9610dd7af20cdaf74c4ff6e6d678d44)?

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-14, Robert Haas wrote:

> On Thu, Mar 7, 2024 at 12:32 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2024-Mar-07, Alvaro Herrera wrote:
> > > Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> > > RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> > > logical replication would continue to not know about this PK, which
> > > perhaps is what we want.  I'll do some testing with this.
> >
> > This seems to work okay.
> 
> There is a CommitFest entry for this patch. Should that entry be
> closed in view of the not-NULL revert
> (6f8bb7c1e9610dd7af20cdaf74c4ff6e6d678d44)?

Uhmm, I didn't realize there was a CF entry.  I don't know why it was
there; this should have been an open item, not a bugfix CF entry.

This had already been committed as 270af6f0df76 (the day before it was
sent to the next commitfest).  This commit wasn't included in the
reverted set, though, so you still get deferrable PKs from
RelationGetIndexList.  I don't think this is necessarily a bad thing,
though these don't have any usefulness as things stand (and if we deal
with PKs by forcing not-null constraints to be underneath, then we won't
need them either).

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

On Tue, May 14, 2024 at 10:42 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> This had already been committed as 270af6f0df76 (the day before it was
> sent to the next commitfest).  This commit wasn't included in the
> reverted set, though, so you still get deferrable PKs from
> RelationGetIndexList.  I don't think this is necessarily a bad thing,
> though these don't have any usefulness as things stand (and if we deal
> with PKs by forcing not-null constraints to be underneath, then we won't
> need them either).

So, are you saying this should be marked Committed in the commitfest?

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-14, Robert Haas wrote:

> On Tue, May 14, 2024 at 10:42 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > This had already been committed as 270af6f0df76 (the day before it was
> > sent to the next commitfest).  This commit wasn't included in the
> > reverted set, though, so you still get deferrable PKs from
> > RelationGetIndexList.  I don't think this is necessarily a bad thing,
> > though these don't have any usefulness as things stand (and if we deal
> > with PKs by forcing not-null constraints to be underneath, then we won't
> > need them either).
> 
> So, are you saying this should be marked Committed in the commitfest?

Yeah.  I've done so.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Pero la cosa no es muy grave ..."     (le petit Nicolas -- René Goscinny)

On Tue, May 14, 2024 at 11:11 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > So, are you saying this should be marked Committed in the commitfest?
>
> Yeah.  I've done so.

Great, thanks.

--
Robert Haas
EDB: http://www.enterprisedb.com